Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
On Tue, 12 Mar 2024 19:22:25 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request incrementally with two additional > commits since the last revision: > > - Print to stdout instead of stderr > - Compare version using VERSION_STRING make/modules/java.base/Launcher.gmk line 81: > 79: SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \ > 80: OPTIMIZATION := LOW, \ > 81: CFLAGS := $(CFLAGS_JDKEXE) \ There is no need to introduce a break after `$(CFLAGS_JDKEXE)`, for thing like flags we try to fill the line. Also, the indentation rules are that a broken line should be indented with four spaces. See e.g. the CFLAGS line below in CoreLibraries.gmk. - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1522267500
Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) > > Updates jspawnhelper to check that JDK version and jspawnhelper version are > the same. Updates test to include check for version. Also tested manually by > replacing jspawnhelper with incorrect version to confirm that check works. Chad Rakoczy has updated the pull request incrementally with two additional commits since the last revision: - Print to stdout instead of stderr - Compare version using VERSION_STRING - Changes: - all: https://git.openjdk.org/jdk/pull/18204/files - new: https://git.openjdk.org/jdk/pull/18204/files/7bbcc08f..62529391 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18204=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18204=01-02 Stats: 71 lines in 5 files changed: 12 ins; 41 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/18204.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18204/head:pull/18204 PR: https://git.openjdk.org/jdk/pull/18204
Re: RFR: 8315113: Print request Chromaticity.MONOCHROME attribute does not work on macOS
On Tue, 12 Mar 2024 17:28:57 GMT, Alexander Scherbatiy wrote: > > But I am really surprised to see a PR rather than a discussion first as it > > is making a lot of policy decisions > > and implies we need to support something I am not sure we want to support. > > There was an email with the issue discussion on the client-libs-dev alias: > https://mail.openjdk.org/pipermail/client-libs-dev/2023-August/014497.html > > ``` > Setting Chromaticity.MONOCHROME print request attribute on macOS > > What is the right way to use different key/values (ColorModel: Gray, > HPColorMode: grayscale, ...) in code to set Black & White settings for > NSPrintInfo? > Could it be a configuration file which contains all color model > key/values for all known printers? > ``` No replies to that. Clearly it was not noticed. A problem with all the PR notifications to the mailing lists is that real emails get overlooked. You should have re-sent it. I don't know what the right answer is for this. Not off the top of my head. I just know that this smells like the Linux font configuration files which were always wrong and/or late as well as being a serious maintenance burden. - PR Comment: https://git.openjdk.org/jdk/pull/18195#issuecomment-1992334610
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request incrementally with one additional > commit since the last revision: > > Code cleanup I'm fine with just using VERSION_FEATURE. I think it's a simple and straightforward enough simplification/approximation. If we think we need a more exact version comparison, then I think we should use one of the version strings instead of arbitrarily picking a set of version variables. - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1992318595
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request incrementally with one additional > commit since the last revision: > > Code cleanup Thanks for all this feedback! I am going to try and see if we can pass the full version string. - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1992241562
Re: RFR: 8315113: Print request Chromaticity.MONOCHROME attribute does not work on macOS
On Tue, 12 Mar 2024 16:41:12 GMT, Phil Race wrote: > But I am really surprised to see a PR rather than a discussion first as it is > making a lot of policy decisions and implies we need to support something I am not sure we want to support. There was an email with the issue discussion on the client-libs-dev alias: https://mail.openjdk.org/pipermail/client-libs-dev/2023-August/014497.html Setting Chromaticity.MONOCHROME print request attribute on macOS What is the right way to use different key/values (ColorModel: Gray, HPColorMode: grayscale, ...) in code to set Black & White settings for NSPrintInfo? Could it be a configuration file which contains all color model key/values for all known printers? - PR Comment: https://git.openjdk.org/jdk/pull/18195#issuecomment-1992189765
Re: RFR: 8315113: Print request Chromaticity.MONOCHROME attribute does not work on macOS
On Mon, 11 Mar 2024 13:54:02 GMT, Alexander Scherbatiy
wrote:
> The fix provides ability to print Black & White pages on macOS.
>
> Cocoa API has
> [PMSetColorMode](https://developer.apple.com/documentation/applicationservices/core_printing/1805783-pmsetcolormode)
> function but it is marked as deprecated and really does nothing.
>
> There is no replacement; this function was included to facilitate porting
> legacy applications to macOS,
> but it serves no useful purpose.
>
> Dumping `NSPrintInfo` print settings which were set by the native print
> dialog on macOS shows that the keys and values used for Black & White
> printing depend on the used printer type.
> For example, the tested
> `HP Color LaserJet MFP M180n` printer uses `ColorModel` key and`Gray` value,
> and
> `HP Ink Tank 110 series` uses `HPColorMode` key and `grayscale` value.
>
> Printing all `NSPrintInfo` presets shows that they do not contain key/value
> pairs for Black settings.
> This is the code snippet used to print `NSPrintInfo` presets:
>
> PMPrinter pr;
> PMPrintSession printSession = (PMPrintSession)[printInfo PMPrintSession];
> OSStatus status = PMSessionGetCurrentPrinter(printSession, );
> CFArrayRef presetsList = nil;
> status = PMPrinterCopyPresets(pr, );
> CFIndex arrayCount = CFArrayGetCount(presetsList);
>
> for (CFIndex index = 0; index < arrayCount; index++) {
> PMPreset preset = (PMPreset)CFArrayGetValueAtIndex(presetsList,
> index);
> CFStringRef presetName = nil;
> if (PMPresetCopyName(preset, ) == noErr &&
> CFStringGetLength(presetName) > 0) {
> NSLog(@" presetName: '%@'", presetName);
> NSDictionary* dict = nil;
> if (PMPresetGetAttributes(preset, (CFDictionaryRef*)()) ==
> noErr) {
>// print preset dict
>
>
> The idea of the proposed fix is to store printer dependent key/value pairs in
> the `/conf/printer.properties` property file.
>
> The property file has the format:
>
> print-attribute.print-attribute-value.key=value
>
> where `print-attribute` is the java print attribute, `print-attribute-value`
> is the corresponding attribute value, and `key` and `value` is the key/value
> pair used by a specific printer.
>
> For example, for `Chromaticity` attribute the property file could look like:
>
> Chromaticity.MONOCHROME.ColorModel=Gray
> Chromaticity.COLOR.ColorModel=CMYK
> Chromaticity.MONOCHROME.HPColorMode=grayscale
> Chromaticity.COLOR.HPColorMode=color
>
> where `Chromaticity.MONOCHROME` key prefix corresponds to
> `Chromaticity.MONOCHOROME` print attribute constant with (...
I wonder how this is supposed to be documented ? And who is supposed to
maintain this file ?
I think this PR should be withdrawn.
src/java.desktop/share/conf/printer.properties line 1:
> 1: Chromaticity.MONOCHROME.BRPrintQuality=Black
Why, oh why would a file needed for CUPS on macOS be in shared code ?
-
PR Comment: https://git.openjdk.org/jdk/pull/18195#issuecomment-1992119844
PR Review Comment: https://git.openjdk.org/jdk/pull/18195#discussion_r1521814087
Re: RFR: 8315113: Print request Chromaticity.MONOCHROME attribute does not work on macOS
On Mon, 11 Mar 2024 13:54:02 GMT, Alexander Scherbatiy
wrote:
> The fix provides ability to print Black & White pages on macOS.
>
> Cocoa API has
> [PMSetColorMode](https://developer.apple.com/documentation/applicationservices/core_printing/1805783-pmsetcolormode)
> function but it is marked as deprecated and really does nothing.
>
> There is no replacement; this function was included to facilitate porting
> legacy applications to macOS,
> but it serves no useful purpose.
>
> Dumping `NSPrintInfo` print settings which were set by the native print
> dialog on macOS shows that the keys and values used for Black & White
> printing depend on the used printer type.
> For example, the tested
> `HP Color LaserJet MFP M180n` printer uses `ColorModel` key and`Gray` value,
> and
> `HP Ink Tank 110 series` uses `HPColorMode` key and `grayscale` value.
>
> Printing all `NSPrintInfo` presets shows that they do not contain key/value
> pairs for Black settings.
> This is the code snippet used to print `NSPrintInfo` presets:
>
> PMPrinter pr;
> PMPrintSession printSession = (PMPrintSession)[printInfo PMPrintSession];
> OSStatus status = PMSessionGetCurrentPrinter(printSession, );
> CFArrayRef presetsList = nil;
> status = PMPrinterCopyPresets(pr, );
> CFIndex arrayCount = CFArrayGetCount(presetsList);
>
> for (CFIndex index = 0; index < arrayCount; index++) {
> PMPreset preset = (PMPreset)CFArrayGetValueAtIndex(presetsList,
> index);
> CFStringRef presetName = nil;
> if (PMPresetCopyName(preset, ) == noErr &&
> CFStringGetLength(presetName) > 0) {
> NSLog(@" presetName: '%@'", presetName);
> NSDictionary* dict = nil;
> if (PMPresetGetAttributes(preset, (CFDictionaryRef*)()) ==
> noErr) {
>// print preset dict
>
>
> The idea of the proposed fix is to store printer dependent key/value pairs in
> the `/conf/printer.properties` property file.
>
> The property file has the format:
>
> print-attribute.print-attribute-value.key=value
>
> where `print-attribute` is the java print attribute, `print-attribute-value`
> is the corresponding attribute value, and `key` and `value` is the key/value
> pair used by a specific printer.
>
> For example, for `Chromaticity` attribute the property file could look like:
>
> Chromaticity.MONOCHROME.ColorModel=Gray
> Chromaticity.COLOR.ColorModel=CMYK
> Chromaticity.MONOCHROME.HPColorMode=grayscale
> Chromaticity.COLOR.HPColorMode=color
>
> where `Chromaticity.MONOCHROME` key prefix corresponds to
> `Chromaticity.MONOCHOROME` print attribute constant with (...
This needs at least two client Capital R reviewers to approve. And a CSR.
But I am really surprised to see a PR rather than a discussion first as it is
making a lot of policy decisions
and implies we need to support something I am not sure we want to support.
Yeah this looks like a complete mis-feature. Meaning it is very unlikely we
will ever agree to integrate this.
-
PR Comment: https://git.openjdk.org/jdk/pull/18195#issuecomment-1992099400
PR Comment: https://git.openjdk.org/jdk/pull/18195#issuecomment-1992103790
Re: RFR: 8315113: Print request Chromaticity.MONOCHROME attribute does not work on macOS
On Mon, 11 Mar 2024 13:54:02 GMT, Alexander Scherbatiy
wrote:
> The fix provides ability to print Black & White pages on macOS.
>
> Cocoa API has
> [PMSetColorMode](https://developer.apple.com/documentation/applicationservices/core_printing/1805783-pmsetcolormode)
> function but it is marked as deprecated and really does nothing.
>
> There is no replacement; this function was included to facilitate porting
> legacy applications to macOS,
> but it serves no useful purpose.
>
> Dumping `NSPrintInfo` print settings which were set by the native print
> dialog on macOS shows that the keys and values used for Black & White
> printing depend on the used printer type.
> For example, the tested
> `HP Color LaserJet MFP M180n` printer uses `ColorModel` key and`Gray` value,
> and
> `HP Ink Tank 110 series` uses `HPColorMode` key and `grayscale` value.
>
> Printing all `NSPrintInfo` presets shows that they do not contain key/value
> pairs for Black settings.
> This is the code snippet used to print `NSPrintInfo` presets:
>
> PMPrinter pr;
> PMPrintSession printSession = (PMPrintSession)[printInfo PMPrintSession];
> OSStatus status = PMSessionGetCurrentPrinter(printSession, );
> CFArrayRef presetsList = nil;
> status = PMPrinterCopyPresets(pr, );
> CFIndex arrayCount = CFArrayGetCount(presetsList);
>
> for (CFIndex index = 0; index < arrayCount; index++) {
> PMPreset preset = (PMPreset)CFArrayGetValueAtIndex(presetsList,
> index);
> CFStringRef presetName = nil;
> if (PMPresetCopyName(preset, ) == noErr &&
> CFStringGetLength(presetName) > 0) {
> NSLog(@" presetName: '%@'", presetName);
> NSDictionary* dict = nil;
> if (PMPresetGetAttributes(preset, (CFDictionaryRef*)()) ==
> noErr) {
>// print preset dict
>
>
> The idea of the proposed fix is to store printer dependent key/value pairs in
> the `/conf/printer.properties` property file.
>
> The property file has the format:
>
> print-attribute.print-attribute-value.key=value
>
> where `print-attribute` is the java print attribute, `print-attribute-value`
> is the corresponding attribute value, and `key` and `value` is the key/value
> pair used by a specific printer.
>
> For example, for `Chromaticity` attribute the property file could look like:
>
> Chromaticity.MONOCHROME.ColorModel=Gray
> Chromaticity.COLOR.ColorModel=CMYK
> Chromaticity.MONOCHROME.HPColorMode=grayscale
> Chromaticity.COLOR.HPColorMode=color
>
> where `Chromaticity.MONOCHROME` key prefix corresponds to
> `Chromaticity.MONOCHOROME` print attribute constant with (...
/Reviewers 2
/Reviewers 4
-
PR Comment: https://git.openjdk.org/jdk/pull/18195#issuecomment-1992093431
PR Comment: https://git.openjdk.org/jdk/pull/18195#issuecomment-1992094712
Re: RFR: 8315113: Print request Chromaticity.MONOCHROME attribute does not work on macOS
On Mon, 11 Mar 2024 13:54:02 GMT, Alexander Scherbatiy
wrote:
> The fix provides ability to print Black & White pages on macOS.
>
> Cocoa API has
> [PMSetColorMode](https://developer.apple.com/documentation/applicationservices/core_printing/1805783-pmsetcolormode)
> function but it is marked as deprecated and really does nothing.
>
> There is no replacement; this function was included to facilitate porting
> legacy applications to macOS,
> but it serves no useful purpose.
>
> Dumping `NSPrintInfo` print settings which were set by the native print
> dialog on macOS shows that the keys and values used for Black & White
> printing depend on the used printer type.
> For example, the tested
> `HP Color LaserJet MFP M180n` printer uses `ColorModel` key and`Gray` value,
> and
> `HP Ink Tank 110 series` uses `HPColorMode` key and `grayscale` value.
>
> Printing all `NSPrintInfo` presets shows that they do not contain key/value
> pairs for Black settings.
> This is the code snippet used to print `NSPrintInfo` presets:
>
> PMPrinter pr;
> PMPrintSession printSession = (PMPrintSession)[printInfo PMPrintSession];
> OSStatus status = PMSessionGetCurrentPrinter(printSession, );
> CFArrayRef presetsList = nil;
> status = PMPrinterCopyPresets(pr, );
> CFIndex arrayCount = CFArrayGetCount(presetsList);
>
> for (CFIndex index = 0; index < arrayCount; index++) {
> PMPreset preset = (PMPreset)CFArrayGetValueAtIndex(presetsList,
> index);
> CFStringRef presetName = nil;
> if (PMPresetCopyName(preset, ) == noErr &&
> CFStringGetLength(presetName) > 0) {
> NSLog(@" presetName: '%@'", presetName);
> NSDictionary* dict = nil;
> if (PMPresetGetAttributes(preset, (CFDictionaryRef*)()) ==
> noErr) {
>// print preset dict
>
>
> The idea of the proposed fix is to store printer dependent key/value pairs in
> the `/conf/printer.properties` property file.
>
> The property file has the format:
>
> print-attribute.print-attribute-value.key=value
>
> where `print-attribute` is the java print attribute, `print-attribute-value`
> is the corresponding attribute value, and `key` and `value` is the key/value
> pair used by a specific printer.
>
> For example, for `Chromaticity` attribute the property file could look like:
>
> Chromaticity.MONOCHROME.ColorModel=Gray
> Chromaticity.COLOR.ColorModel=CMYK
> Chromaticity.MONOCHROME.HPColorMode=grayscale
> Chromaticity.COLOR.HPColorMode=color
>
> where `Chromaticity.MONOCHROME` key prefix corresponds to
> `Chromaticity.MONOCHOROME` print attribute constant with (...
> Build changes look good. However, I think there is some kind of movement
> trying to _remove_ stuff from the conf directory, rather than adding new
> files there. @AlanBateman might want to chime in on this.
Hi, as a regular JDK user and a stakeholder to this getting fixed, is there any
insight as to where they're moving to?
-
PR Comment: https://git.openjdk.org/jdk/pull/18195#issuecomment-1992075911
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Tue, 12 Mar 2024 15:24:21 GMT, Magnus Ihse Bursie wrote: > Please re-test. Hi Magnus, thanks for the adjustments. I reenabled your patch in our build/test queue . - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1992009593
Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]
On Tue, 12 Mar 2024 13:51:28 GMT, Magnus Ihse Bursie wrote:
>> test/jdk/java/foreign/CallGeneratorHelper.java line 216:
>>
>>> 214: if (header) {
>>> 215: System.out.println(
>>> 216: "#include \"export.h\"\n"
>>
>> We don't generate these header files any more, so the changes to this file
>> are not really needed.
>
> I still wouldn't like to keep the bad hard-coded defines. Are you okay with
> me pushing these changes, and then you can remove the parts of the test that
> are not actually used anymore? If the code is not used it should not matter
> much to you either way.
>
> (I mean I could back out these changes, but then we'd have the bad code in
> place while waiting for you to remove it, putting pressure on you to actually
> remove it.)
Yes, that's fine. Sorry, I meant to file a JBS issue and come back with another
comment last time, but I got distracted. Filed:
https://bugs.openjdk.org/browse/JDK-8327994 now
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18135#discussion_r1521715617
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Mon, 11 Mar 2024 12:27:08 GMT, Joachim Kern wrote:
>> Magnus Ihse Bursie has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Revert SEARCH_PATH changes
>
> make/autoconf/toolchain.m4 line 940:
>
>> 938: if test "x$OPENJDK_TARGET_OS" = xaix; then
>> 939: # Make sure we have the Open XL version of clang on AIX
>> 940: $ECHO "$CC_VERSION_OUTPUT" | $GREP "IBM Open XL C/C++ for AIX" >
>> /dev/null
>
> This does not work since $CC_VERSION_OUTPUT is unset. We need
> CC_VERSION_OUTPUT=`${XLC_TEST_PATH}ibm-clang++_r --version 2>&1 | $HEAD
> -n 1`
> before, as in the previous code some lines above which you removed.
Yeah, you are right. I confused it with CC_VERSION_STRING.
... duh.
Actually, that is what I should be using instead.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521667916
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Mon, 11 Mar 2024 12:44:55 GMT, Matthias Baesken wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Revert SEARCH_PATH changes > > With this change added, currently configure fails > > > checking for ibm-llvm-cxxfilt... > /opt/IBM/openxlC/17.1.1/tools/ibm-llvm-cxxfilt > configure: error: ibm-clang_r version output check failed, output: > configure exiting with result code > > maybe related to what Joachim pointed out ? @MBaesken Yes, it was the bug Joachim found. It should be fixed now, together will all other comments (except the example version string in the comments). Please re-test. - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1991913029
Re: RFR: 8327701: Remove the xlc toolchain [v4]
> As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building > the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect > clang by another name, and it uses the clang toolchain in the JDK build. Thus > the old xlc toolchain is no longer supported, and should be removed. Magnus Ihse Bursie has updated the pull request incrementally with two additional commits since the last revision: - Replace CC_VERSION_OUTPUT with CC_VERSION_STRING - We need CC_VERSION_OUTPUT before we can check it - Changes: - all: https://git.openjdk.org/jdk/pull/18172/files - new: https://git.openjdk.org/jdk/pull/18172/files/e7949499..577715b3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18172=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18172=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18172.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18172/head:pull/18172 PR: https://git.openjdk.org/jdk/pull/18172
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Tue, 12 Mar 2024 15:15:27 GMT, Magnus Ihse Bursie wrote: >> make/autoconf/toolchain.m4 line 409: >> >>> 407: #Target: powerpc-ibm-aix7.2.0.0 >>> 408: #Thread model: posix >>> 409: #InstalledDir: /opt/IBM/openxlC/17.1.0/bin >> >> Please correct: >> IBM Open XL C/C++ for AIX 17.1.**1** (xxx), clang version >> 1**5**.0.0 >> #Target: **powerpc-ibm-aix7.2.**5**.7** >> #Thread model: posix >> #InstalledDir: /opt/IBM/openxlC/17.1.**1**/bin > > That was just intended as an example code. At one point, this has been > printed by the compiler output (at least according to the source I found on > the net). I tried to find a bunch of different examples from different > platforms and versions. > > But sure, if it makes you happier, I can replace it with a more up to date > example. Actually, your example seems doctored (the `x`) string. I think I'll keep the real world example. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521659238
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Mon, 11 Mar 2024 09:14:27 GMT, Joachim Kern wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Revert SEARCH_PATH changes > > make/autoconf/toolchain.m4 line 409: > >> 407: #Target: powerpc-ibm-aix7.2.0.0 >> 408: #Thread model: posix >> 409: #InstalledDir: /opt/IBM/openxlC/17.1.0/bin > > Please correct: > IBM Open XL C/C++ for AIX 17.1.**1** (xxx), clang version > 1**5**.0.0 > #Target: **powerpc-ibm-aix7.2.**5**.7** > #Thread model: posix > #InstalledDir: /opt/IBM/openxlC/17.1.**1**/bin That was just intended as an example code. At one point, this has been printed by the compiler output (at least according to the source I found on the net). I tried to find a bunch of different examples from different platforms and versions. But sure, if it makes you happier, I can replace it with a more up to date example. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521656819
Re: RFR: 8327701: Remove the xlc toolchain [v3]
On Mon, 11 Mar 2024 13:45:46 GMT, Joachim Kern wrote: >> OK this was a flaw in my introduction of clang toolchain for AIX. The >> intention was to keep the xlc options in form of their clang counterparts. I >> will try with a corrected version for clang on AIX and will come back to you. > > OK, the `-Wl,-bbigtoc` is not a compiler option but a linker option and it is > already set in the linker options. > But the `-fpic -mcmodel=large` should be set to avoid creating a jump to > out-of-order code. > > So we can replace the > > JVM_PICFLAG="$PICFLAG" > JDK_PICFLAG="$PICFLAG" > > > code some lines below by > > > if test "x$TOOLCHAIN_TYPE" = xclang && test "x$OPENJDK_TARGET_OS" = xaix; > then > JVM_PICFLAG="-fpic -mcmodel=large" > else > JVM_PICFLAG="$PICFLAG" > fi > JDK_PICFLAG="$PICFLAG" I'm not entirely happy to do this here, as it is a bug fix, and it will change behavior, in contrast to the rest of the patch which is just about removing unused code. But it's a small fix, and I'm messing with the part of the code anyway, so if you are sure that this is the right thing to do I'll sneak it in here. Please make sure it works. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521652510
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Tue, 12 Mar 2024 15:07:15 GMT, Magnus Ihse Bursie wrote: >>> Is Open XL C/C++ considered a compiler or more akin to a development >>> environment like Xcode is for macOS? Depending on which, we could just say >>> clang is the compiler for AIX without needing to say that Open XL is >>> treated like clang, etc >> >> You are raising a good point. Our current concept of "toolchain" is not >> working as smoothly as it should be. We should probably split it into >> "toolchain environment" (or whatever), like Xcode or Open XL, and "toolchain >> compiler" (like clang), and/or possibly also some concept of "toolchain sdk". >> >> The work you are doing of untangling Windows from Visual Studio would >> probably influence such a redesign, seeing what part of the Windows >> adaptions which arise from cl, and which are constant given that you compile >> for windows (like the Windows SDK). >> >> But this is a more architectural issue that will need to be resolved in >> another PR, and probably discussed quite a bit beforehand. > >> why did this remove the link to the Supported Build Platforms page? > > You make it sound like it is a bad thing, but in fact I promoted the AIX > build to be of the same status as all other platforms. I cowardly did not put > anything specific about the AIX build in the readme before, but just referred > to the wiki. Now I decided we should actually treat this as a proper platform > and describe the compiler version in the readme, since it is checked in > configure, just like all other platforms. > > There is still a link to the Supported Build Platforms wiki page in the > general part of the readme. > It is clang 15 not 13. Clang 13 was in 17.1.0 Thanks! Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521646622
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Tue, 12 Mar 2024 15:04:56 GMT, Magnus Ihse Bursie wrote: >> Is Open XL C/C++ considered a compiler or more akin to a development >> environment like Xcode is for macOS? Depending on which, we could just say >> clang is the compiler for AIX without needing to say that Open XL is treated >> like clang, etc >> >> Also, why did this remove the link to the Supported Build Platforms page? > >> Is Open XL C/C++ considered a compiler or more akin to a development >> environment like Xcode is for macOS? Depending on which, we could just say >> clang is the compiler for AIX without needing to say that Open XL is treated >> like clang, etc > > You are raising a good point. Our current concept of "toolchain" is not > working as smoothly as it should be. We should probably split it into > "toolchain environment" (or whatever), like Xcode or Open XL, and "toolchain > compiler" (like clang), and/or possibly also some concept of "toolchain sdk". > > The work you are doing of untangling Windows from Visual Studio would > probably influence such a redesign, seeing what part of the Windows adaptions > which arise from cl, and which are constant given that you compile for > windows (like the Windows SDK). > > But this is a more architectural issue that will need to be resolved in > another PR, and probably discussed quite a bit beforehand. > why did this remove the link to the Supported Build Platforms page? You make it sound like it is a bad thing, but in fact I promoted the AIX build to be of the same status as all other platforms. I cowardly did not put anything specific about the AIX build in the readme before, but just referred to the wiki. Now I decided we should actually treat this as a proper platform and describe the compiler version in the readme, since it is checked in configure, just like all other platforms. There is still a link to the Supported Build Platforms wiki page in the general part of the readme. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521642995
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Mon, 11 Mar 2024 09:30:20 GMT, Julian Waters wrote: > Is Open XL C/C++ considered a compiler or more akin to a development > environment like Xcode is for macOS? Depending on which, we could just say > clang is the compiler for AIX without needing to say that Open XL is treated > like clang, etc You are raising a good point. Our current concept of "toolchain" is not working as smoothly as it should be. We should probably split it into "toolchain environment" (or whatever), like Xcode or Open XL, and "toolchain compiler" (like clang), and/or possibly also some concept of "toolchain sdk". The work you are doing of untangling Windows from Visual Studio would probably influence such a redesign, seeing what part of the Windows adaptions which arise from cl, and which are constant given that you compile for windows (like the Windows SDK). But this is a more architectural issue that will need to be resolved in another PR, and probably discussed quite a bit beforehand. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521637464
Re: RFR: 8327701: Remove the xlc toolchain [v3]
> As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building > the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect > clang by another name, and it uses the clang toolchain in the JDK build. Thus > the old xlc toolchain is no longer supported, and should be removed. Magnus Ihse Bursie has updated the pull request incrementally with two additional commits since the last revision: - Set JVM_PICFLAG="-fpic -mcmodel=large" for AIX - Open XL 17.1.1.4 is clang 15. - Changes: - all: https://git.openjdk.org/jdk/pull/18172/files - new: https://git.openjdk.org/jdk/pull/18172/files/53a05019..e7949499 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18172=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18172=01-02 Stats: 7 lines in 3 files changed: 4 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18172.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18172/head:pull/18172 PR: https://git.openjdk.org/jdk/pull/18172
Re: RFR: 8323672: Suppress unwanted autoconf added flags in CC and CXX [v7]
On Mon, 12 Feb 2024 14:09:49 GMT, Julian Waters wrote: >> I have verified that this works fine in the Oracle internal CI. >> >> Erik's point still stands: >>> I still think it would be prudent to verify this patch with all the >>> currently accepted versions of autoconf (2.69, 2.70, 2.71, 2.72). >> >> I think the easiest way to achieve this is to checkout the autoconf at these >> versions, and build it yourself. Iirc it was quite easy to build autoconf >> (anything else would be a shame and very bad PR for the project! :-o). >> >> I'm hoping you are willing to do this, since I believe this is a superior >> solution and I'd like to see it in mainline. (Otherwise, let me know and >> I'll try to squeeze in doing it.) > >> I have verified that this works fine in the Oracle internal CI. >> >> Erik's point still stands: >> >> > I still think it would be prudent to verify this patch with all the >> > currently accepted versions of autoconf (2.69, 2.70, 2.71, 2.72). >> >> I think the easiest way to achieve this is to checkout the autoconf at these >> versions, and build it yourself. Iirc it was quite easy to build autoconf >> (anything else would be a shame and very bad PR for the project! :-o). >> >> I'm hoping you are willing to do this, since I believe this is a superior >> solution and I'd like to see it in mainline. (Otherwise, let me know and >> I'll try to squeeze in doing it.) > > It's actually even easier to do on Windows than it might appear, since > MinGW's pacman system has caches that contain past package versions. I > wouldn't have to build it at all in fact, all I'd have to do is downgrade my > autoconf to past versions (I think I'm currently on 2.71). I know you're > probably very busy right now, so I'll spare you the chore of having to do > that by testing it on my end. That said, should I test autoconf on all > platforms too? That aside, I'm still a little unhappy that there is no formal > way to unregister an AC_DEFUN macro though :( > > By the way, I've left you something in the mail. Hope you have some time to > check it out! > @TheShermanTanker I think this is essentially done. All that needs are the > testing. Can you do it or do you want help with it? I'm able to do so myself, I've just been busy with university so far. I've only tested 2.71 and 2.72 though, 2.69 and 2.70 might still need testing. I'm still unhappy that I can't properly undefine the macros, as a side tangent - PR Comment: https://git.openjdk.org/jdk/pull/17401#issuecomment-1991874882
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
On Tue, 27 Feb 2024 15:23:09 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Only show runtime image suffix for JDK modules Thanks for all the descriptions! They were all very clear and well-written. I'm going to paste some of them into the JBS issue for future record keeping. With this background, I fully agree with the chosen solution. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1991815728
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
On Mon, 11 Mar 2024 13:06:55 GMT, Erik Joelsson wrote: >> Why the rm? Because jlink refuses to run if the output dir already exists. > > I don't see a race. The `rm` was there in the original code and is no scarier > in the modified version. The jdk image is constructed by a combination of > targets and recipes. The first one to run has to be jlink, then we overlay > more stuff on top. If the makefile dependency resolution concludes that we > need to rerun the jlink step, we clear out the directory completely to make > sure we aren't leaving anything in there that's no longer valid. This is > basically a precaution to guarantee a correct incremental build. It's not > incurring a big build time penalty as jlink would have overwritten all files > it would have created anyway. However, if a module was removed, or a file was > removed from a module, the `rm` helps guarantee that we don't include such a > removed file from a previous build attempt in the final image. > > Or it may even be as Severin says, that jlink refuses to run, I don't > remember. Ok, good, thanks. I did not see the whole picture there. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1521535618
Re: RFR: 8315113: Print request Chromaticity.MONOCHROME attribute does not work on macOS
On Mon, 11 Mar 2024 13:54:02 GMT, Alexander Scherbatiy
wrote:
> The fix provides ability to print Black & White pages on macOS.
>
> Cocoa API has
> [PMSetColorMode](https://developer.apple.com/documentation/applicationservices/core_printing/1805783-pmsetcolormode)
> function but it is marked as deprecated and really does nothing.
>
> There is no replacement; this function was included to facilitate porting
> legacy applications to macOS,
> but it serves no useful purpose.
>
> Dumping `NSPrintInfo` print settings which were set by the native print
> dialog on macOS shows that the keys and values used for Black & White
> printing depend on the used printer type.
> For example, the tested
> `HP Color LaserJet MFP M180n` printer uses `ColorModel` key and`Gray` value,
> and
> `HP Ink Tank 110 series` uses `HPColorMode` key and `grayscale` value.
>
> Printing all `NSPrintInfo` presets shows that they do not contain key/value
> pairs for Black settings.
> This is the code snippet used to print `NSPrintInfo` presets:
>
> PMPrinter pr;
> PMPrintSession printSession = (PMPrintSession)[printInfo PMPrintSession];
> OSStatus status = PMSessionGetCurrentPrinter(printSession, );
> CFArrayRef presetsList = nil;
> status = PMPrinterCopyPresets(pr, );
> CFIndex arrayCount = CFArrayGetCount(presetsList);
>
> for (CFIndex index = 0; index < arrayCount; index++) {
> PMPreset preset = (PMPreset)CFArrayGetValueAtIndex(presetsList,
> index);
> CFStringRef presetName = nil;
> if (PMPresetCopyName(preset, ) == noErr &&
> CFStringGetLength(presetName) > 0) {
> NSLog(@" presetName: '%@'", presetName);
> NSDictionary* dict = nil;
> if (PMPresetGetAttributes(preset, (CFDictionaryRef*)()) ==
> noErr) {
>// print preset dict
>
>
> The idea of the proposed fix is to store printer dependent key/value pairs in
> the `/conf/printer.properties` property file.
>
> The property file has the format:
>
> print-attribute.print-attribute-value.key=value
>
> where `print-attribute` is the java print attribute, `print-attribute-value`
> is the corresponding attribute value, and `key` and `value` is the key/value
> pair used by a specific printer.
>
> For example, for `Chromaticity` attribute the property file could look like:
>
> Chromaticity.MONOCHROME.ColorModel=Gray
> Chromaticity.COLOR.ColorModel=CMYK
> Chromaticity.MONOCHROME.HPColorMode=grayscale
> Chromaticity.COLOR.HPColorMode=color
>
> where `Chromaticity.MONOCHROME` key prefix corresponds to
> `Chromaticity.MONOCHOROME` print attribute constant with (...
Build changes look good. However, I think there is some kind of movement trying
to *remove* stuff from the conf directory, rather than adding new files there.
@AlanBateman might want to chime in on this.
-
Marked as reviewed by ihse (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18195#pullrequestreview-1931171225
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 20:04:48 GMT, Roger Riggs wrote: >> Chad Rakoczy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Code cleanup > > make/modules/java.base/Launcher.gmk line 85: > >> 83: -DVERSION_INTERIM=$(VERSION_INTERIM) \ >> 84: -DVERSION_UPDATE=$(VERSION_UPDATE) \ >> 85: -DVERSION_PATCH=$(VERSION_PATCH), \ > > Using all 4 is way overkill for the problem at hand. Just the > FEATURE_VERSION is sufficient. > We all know better than to make incompatible changes in minor versions let > alone update or patch version. There is already a `$(VERSION_CFLAGS)` variable defined. It will set all those (and some more). Please use it instead. But then, as Roger says, it is probably overkill to *check* anything but the feature version. - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1521525658
Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]
On Mon, 11 Mar 2024 02:31:02 GMT, David Holmes wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update line number for dereference_null in TestDwarf > > test/hotspot/jtreg/runtime/ErrorHandling/libTestDwarfHelper.h line 24: > >> 22: */ >> 23: >> 24: #include > > Seems unneeded. So what I did here was change: #include "jni.h" #include into #include #include "export.h" #include "jni.h" The reordering was strictly not needed, but putting user includes in front of system ones looked like bad coding to me, and put me in a difficult spot in where to add the new `#include "export.h"` -- next to the current user include even if I thought that was wrong, or have the system includes "sandwitched" between two user includes. Or do you mean that it seems unneeded to include `jni.h` at all? Yes, I agree, but it was there before, and I don't want to make any other changes to the tests. This change is scary enough as it is :-). If you want, I can file a follow-up to remove this include instead. - PR Review Comment: https://git.openjdk.org/jdk/pull/18135#discussion_r1521510510
Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]
On Mon, 11 Mar 2024 06:57:58 GMT, Alan Bateman wrote: > Good cleanup. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/18135#issuecomment-1991712859
Re: RFR: 8327460: Compile tests with the same visibility rules as product code
On Mon, 11 Mar 2024 06:57:42 GMT, Alan Bateman wrote: > > test/jdk/java/lang/Thread/jni/AttachCurrentThread/libImplicitAttach.c was > > missing an export. This had not been discovered before since that file was > > not compiled on Windows. > It's a Linux/macOS specific test so it wasn't needed. Yeah. I understand. I just wanted to give an explanation for why this particular test needed changes that were not present elsewhere (since other exported methods all were tested on Windows). - PR Comment: https://git.openjdk.org/jdk/pull/18135#issuecomment-1991712352
Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]
On Fri, 8 Mar 2024 18:12:40 GMT, Jorn Vernee wrote:
>> Magnus Ihse Bursie has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update line number for dereference_null in TestDwarf
>
> test/jdk/java/foreign/CallGeneratorHelper.java line 216:
>
>> 214: if (header) {
>> 215: System.out.println(
>> 216: "#include \"export.h\"\n"
>
> We don't generate these header files any more, so the changes to this file
> are not really needed.
I still wouldn't like to keep the bad hard-coded defines. Are you okay with me
pushing these changes, and then you can remove the parts of the test that are
not actually used anymore? If the code is not used it should not matter much to
you either way.
(I mean I could back out these changes, but then we'd have the bad code in
place while waiting for you to remove it, putting pressure on you to actually
remove it.)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18135#discussion_r1521503942
