Re: RFR: JDK-8298405: Implement JEP 467: Markdown Documentation Comments [v49]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 68 commits: - Merge with upstream/master - improve first-sentence break in Markdown comments - add test cases for links to anchors - fix `textOf` method to accommodate Markdown content. - avoid relying on unspecified behavior `List.toString` - fix test after merge - Merge remote-tracking branch 'upstream/master' into 8298405.doclet-markdown-v3 - Revise `Markdown.update` to better handle container blocks. - Refactor most of TestMarkdown.java into separate tests, grouped by functionality - add support for (top-level) trailing code blocks add simple test case with tabs add TestMarkdownCodeBlocks.testTypical - ... and 58 more: https://git.openjdk.org/jdk/compare/586396cb...76eccbf4 - Changes: https://git.openjdk.org/jdk/pull/16388/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=48 Stats: 23757 lines in 208 files changed: 23030 ins; 281 del; 446 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: RFR: JDK-8298405: Implement JEP 467: Markdown Documentation Comments [v48]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: improve first-sentence break in Markdown comments - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/8dfd9e08..81279a74 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=47 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=46-47 Stats: 50 lines in 2 files changed: 45 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
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 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1520364358
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 18:36:57 GMT, Aleksey Shipilev wrote: > > If you really want to require an exact match for jspawnhelper, then these 4 > > numbers aren't necessarily enough, but a rather arbitrarily chosen > > approximation. > > I think for our purposes, a version number quadruplet is enough to provide > basic level of safety for jspawnhelper protocol updates across JDK versions, > without version checking code being a safety problem itself. > > Putting in the full version string would raise more questions, at least for > me. For example, are we guaranteed that version string always fits the > argument line? Probably so. Would we get some interesting (Unicode?) symbols > in version string that would break somewhere along the way? Would we need to > dynamically allocate the buffer for arguments, instead of allocating enough > to hold the version _integers_? Would we make a mistake while doing so? Etc. > > All in all, it feels better to silently accept some version mismatches not > captured by the version quadruplet, rather than fail trying to do a perfect > match. Legal characters for the version string are defined in https://openjdk.org/jeps/223 and verified by configure. There is no risk of weird unicode being included. If the full version string is too long, you could consider using `VERSION_SHORT`, which drops the $OPT, and the $BUILD part. I don't think we need to dynamically allocate the buffer. The required buffer length is known at compile time as the length of the expected version string (which we get as a command line macro) +1. If the user supplies more, we don't need to compare the rest of the string as we already know it's not equal. To me, hardcoding of 4 version digits raises more questions. We spend unnecessary time and effort specifically serializing and de-serializing 4 numbers, always maintaining the correct order. To me that's brittle code. If the version elements, or how we use them, were to change in the future, then this code needs to be carefully revisited, whereas if we just used a standard version string, it would continue to work. - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1989320103
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
src/java.base/unix/native/jspawnhelper/jspawnhelper.c line 173:
> 171: if (jdk_feature != VERSION_FEATURE || jdk_interim !=
> VERSION_INTERIM || jdk_update != VERSION_UPDATE || jdk_patch !=
> VERSION_PATCH) {
> 172: fprintf(stderr, "Expected jspawnhelper for Java %d.%d.%d.%d,
> ", jdk_feature, jdk_interim, jdk_update, jdk_patch);
> 173: fprintf(stderr, "but jspawnhelper for Java %d.%d.%d.%d was
> found.\n", VERSION_FEATURE, VERSION_INTERIM, VERSION_UPDATE, VERSION_PATCH);
Minor: It is a bit odd to see that jspawnhelper found its own version odd. I'd
say:
fprintf(stderr, "jspawnhelper version check failed. jspawnhelper version:
%d.%d.%d.%d, JDK version: %d.%d.%d.%d\n",
VERSION_FEATURE, VERSION_INTERIM, VERSION_UPDATE, VERSION_PATCH,
jdk_feature, jdk_interim, jdk_update, jdk_patch);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1520296858
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/18204/files - new: https://git.openjdk.org/jdk/pull/18204/files/cf3c20fb..7bbcc08f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18204&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18204&range=00-01 Stats: 48 lines in 4 files changed: 16 ins; 24 del; 8 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: 8325621: Improve jspawnhelper version checks
On Mon, 11 Mar 2024 18:56:21 GMT, Bernd wrote: > with a protocol version you don’t have to care about micro versions and also > it is more tolerant about the usual cpu updates which do not introduce > incompatibilities most of the time. I think we can only reasonably guarantee that jspawnhelper built for a particular JDK is compatible. Protocol version does not exactly capture that: there might be a slight change in JDK that jspawnhelper needs to be aware about, but which does not readily manifest in handshake protocol. Version quadruplet seems to get us what we want automatically. Plus, as you said, protocol number would probably communicate a wrong message that there is some guarantee about the protocol. > btw just as a datapoint: we run into this issue with a longrunning Gerrit > server which could no longer invoke external ssh client for incoming hooks > (ad did not log this). It was not expected to use the system-vm which was > updated on the running system by ubuntu. Ouch! - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1989231670
Re: RFR: 8325621: Improve jspawnhelper version checks
On Mon, 11 Mar 2024 17:46:14 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. Since incompatible changes here are seldom, another option would be to set/send a protocol version. Because if you reject an execute() on each mismatch or if only a incompatible execute() fails is both undesireable, but much more often with version compare (of course third behavior crash/corruption would be bad, but the bugfix should avoid that). with a protocol version you don’t have to care about micro versions and also it is more tolerant about the usual cpu updates which do not introduce incompatibilities most of the time. having said that, if you don’t want to introduce a protocol version and don’t want to gurantee this interface - the version quadruple would be fine for the most common cases of quarterly security updates. btw just as a datapoint: we run into this issue with a longrunning Gerrit server which could no longer invoke external ssh client for incoming hooks (ad did not log this). It was not expected to use the system-vm which was updated on the running system by ubuntu. - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1989211732
Re: RFR: 8325621: Improve jspawnhelper version checks
On Mon, 11 Mar 2024 18:16:53 GMT, Erik Joelsson wrote: > If you really want to require an exact match for jspawnhelper, then these 4 > numbers aren't necessarily enough, but a rather arbitrarily chosen > approximation. I think for our purposes, a version number quadruplet is enough to provide basic level of safety for jspawnhelper protocol updates across JDK versions, without version checking code being a safety problem itself. Putting in the full version string would raise more questions, at least for me. For example, are we guaranteed that version string always fits the argument line? Probably so. Would we get some interesting (Unicode?) symbols in version string that would break somewhere along the way? Would we need to dynamically allocate the buffer for arguments, instead of allocating enough to hold the version _integers_? Would we make a mistake while doing so? Etc. All in all, it feels better to silently accept some version mismatches not captured by the version quadruplet, rather than fail trying to do a perfect match. - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1989179598
RFR: 8325621: Improve jspawnhelper version checks
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. - Commit messages: - Remove string.h include - Update test - Pass version as integer instead of string - Read in version using int instead of string - 8325621: Improve jspawnhelper version checks Changes: https://git.openjdk.org/jdk/pull/18204/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18204&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325621 Stats: 76 lines in 6 files changed: 70 ins; 0 del; 6 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: 8325621: Improve jspawnhelper version checks
On Mon, 11 Mar 2024 17:46:14 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.
If you really want to require an exact match for jspawnhelper, then these 4
numbers aren't necessarily enough, but a rather arbitrarily chosen
approximation. We have 3 more potential dotted numbers (extra1-3) as well as
the OPT string that could potentially describe a different build.
I saw someone argued for using separated version variables for comparison in an
issue comment, but I'm not sure I agree. We have a pretty well defined version
string so comparing that would be much easier, and basically guaranteed to
accomplish what I understand to be the goal of this change. Comparing any
subset of other version variables will always just be an approximation, and
while an approximation may be fine, it's difficult to predict exactly how that
will play out in the future.
src/java.base/unix/native/jspawnhelper/jspawnhelper.c line 172:
> 170: // Check that JDK version and jspawnhelper version are the same
> 171: if (jdk_feature != VERSION_FEATURE || jdk_interim !=
> VERSION_INTERIM || jdk_update != VERSION_UPDATE || jdk_patch !=
> VERSION_PATCH) {
> 172: fprintf(stderr, "Expected jspawnhelper for Java %d.%d.%d+%d,
> ", jdk_feature, jdk_interim, jdk_update, jdk_patch);
This isn't correct. All of feature, interim, update and patch are always dot
separated. The `+` is the separator for the build number.
-
PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1928706835
PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1520217129
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&White settings.
> This is the code snippet used to print `NSPrintInfo` presets:
>
> PMPrinter pr;
> PMPrintSession printSession = (PMPrintSession)[printInfo PMPrintSession];
> OSStatus status = PMSessionGetCurrentPrinter(printSession, &pr);
> CFArrayRef presetsList = nil;
> status = PMPrinterCopyPresets(pr, &presetsList);
> CFIndex arrayCount = CFArrayGetCount(presetsList);
>
> for (CFIndex index = 0; index < arrayCount; index++) {
> PMPreset preset = (PMPreset)CFArrayGetValueAtIndex(presetsList,
> index);
> CFStringRef presetName = nil;
> if (PMPresetCopyName(preset, &presetName) == noErr &&
> CFStringGetLength(presetName) > 0) {
> NSLog(@" presetName: '%@'", presetName);
> NSDictionary* dict = nil;
> if (PMPresetGetAttributes(preset, (CFDictionaryRef*)(&dict)) ==
> 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 change looks ok.
(The copy logic follows the existing pattern in that file so it's fine, even
though we have generally moved to different patterns using the SetupCopyFile
macro.)
-
Marked as reviewed by erikj (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18195#pullrequestreview-1928548932
RFR: 8315113: Print request Chromaticity.MONOCHROME attribute does not work on macOS
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&White settings.
This is the code snippet used to print `NSPrintInfo` presets:
PMPrinter pr;
PMPrintSession printSession = (PMPrintSession)[printInfo PMPrintSession];
OSStatus status = PMSessionGetCurrentPrinter(printSession, &pr);
CFArrayRef presetsList = nil;
status = PMPrinterCopyPresets(pr, &presetsList);
CFIndex arrayCount = CFArrayGetCount(presetsList);
for (CFIndex index = 0; index < arrayCount; index++) {
PMPreset preset = (PMPreset)CFArrayGetValueAtIndex(presetsList, index);
CFStringRef presetName = nil;
if (PMPresetCopyName(preset, &presetName) == noErr &&
CFStringGetLength(presetName) > 0) {
NSLog(@" presetName: '%@'", presetName);
NSDictionary* dict = nil;
if (PMPresetGetAttributes(preset, (CFDictionaryRef*)(&dict)) ==
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 (`ColorModel`,
`Gray`) and (`HPColorMode`, `grayscale`) key/value pairs.
Similarly `Chromaticity.COLOR` key prefix with (`ColorModel`, `CMYK`) and
(`HPColorMode`, `color`) key/value pairs correspond to `Chromaticity.COLOR`
print attribute.
https://www.openprinting.org/download/PPD/ site has been used to collect
`monochrome` and `color` constants from ppd files and save them in
`/conf/printer.properties` file.
For example, for the Brother printer the
https://www.openprinting.org/download/PPD/Brother/BR9440_2_GPL.ppd ppd file
defines `BRPrintQuality` option:
*OpenUI *BRPrintQuality/Color/Mono: PickOne
*OrderDependency: 15 AnySetup *BRPrintQuality
*DefaultBRPrintQuality: Auto
*BRPrintQuality Auto/Auto: "
<>setpagedevice
<>setpagedevice
<>setpagedevice"
*BRPrintQuality Color/Color: "
<>setpagedevice
<>setpagedevice
<>setpagedevice"
*BRPrintQuality Black/Mono: "
<>setpagedevice
<>setpagedevice
<>setpagedevice"
...
*CloseUI: *BRPrintQuality
which is listed in the `printer.properties` file as:
Chromaticity.MONOCHROME.BRPrintQuality=Black
Chromaticity.COLOR.BRPrintQuality=Color
and for the Epson printer the
https://www.openprinting.org/download/PPD/Epson/epal2600.ppd ppd file defines
`EPRendering` option:
*OpenUI *EPRendering/Color Mode: PickOne
*% EPRendering must be after EPColorModel
*OrderDependency: 50 AnySetup *EPRendering
*DefaultEPRendering: None
*EPRendering None/Mono: "
<> >>
setpagedevice"
*End
*% ProcessColorModel is already set in EPColorModel
*EPRendering RGB/Color: "
<< /ProcessColorModel (DeviceRGB) >> setpagedevice
<> >>
setpagedevice"
...
*CloseUI: *EPRendering
which is listed in the `printer.properties` file as:
Chromaticity.MONOCHROME.EPRendering=None
Chromaticity.COLOR.EPRendering=RGB
The full list of constants from https://www.openprinting.org/download/PPD/ used
in `printer.properties` is
# Brother
Chromaticity.MONOCHROME.BRPrintQuality=Black
Chromaticity.COLOR.BRPrintQuality=Color
# Dell
Chromaticity.MONOCHROME.DLColorMode=Black
Chromaticity.COLOR.DLColorMode=Color
# Epson
Chromaticity.MONOCHROME.EPRendering=None
Chromaticity.COLOR.EPRendering=RGB
# Gestener
# Kyocera
# Lanier
# NRG
# Ricoh
# Savin
# Utax
Chromaticity.MONOCHROME.ColorModel=Gray
Chromaticity.COLOR.ColorModel=CMYK
# HP
Chromaticity.MONOCHROME.HPColorAsGray=Yes
Chromaticity.COLOR.HPColorAsGray=No
# KONICA_MINOLTA
Chromaticity.MONOCHROM
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
On Mon, 11 Mar 2024 13:24:54 GMT, Erik Joelsson wrote: > Based on that I agree with the choice of using a configure argument. Thanks. The intention is that without the extra configure argument you'd get `jdk-image` as is today. Not modified. *With* `--enable-runtime-link-image` the result of what ends up in `images/jdk/lib/modules` is changed in a way that it allows linking without needing the packaged modules. Everything else should remain the same. That includes generated CDS archives, `src.zip` and so on. That is why it currently ends up in the same output directory. I hope that is acceptable as requiring downstream distributors to assemble the artefacts by hand (using the linkable jimage from some other location) would be more error prone. My mental model for this would be similar to JVM features (e.g. `--with-jvm-features`). That also keeps the original location of `libjvm` but adds/removes features. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1988501066
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Fri, 8 Mar 2024 15:48:08 GMT, Magnus Ihse Bursie wrote: >> 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 one > additional commit since the last revision: > > Revert SEARCH_PATH changes Changes requested by jkern (Author). - PR Review: https://git.openjdk.org/jdk/pull/18172#pullrequestreview-1927875301
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Mon, 11 Mar 2024 08:59:03 GMT, Joachim Kern wrote: >> make/autoconf/flags-cflags.m4 line 687: >> >>> 685: PICFLAG="-fPIC" >>> 686: PIEFLAG="-fPIE" >>> 687: elif test "x$TOOLCHAIN_TYPE" = xclang && test "x$OPENJDK_TARGET_OS" >>> = xaix; then >> >> Just a remark: This code has never been executed, since running with clang >> on any OS would hit the branch above. Also, the code is syntactically >> incorrect, missing a trailing `"`. > > 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" - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1519747481
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 > > What is the rationale for introducing a special configure flag for this > > functionality? I've tried to look though all comments in this PR, and read > > the JBS issue and CSR, but I have not found any motivation for this. I'm > > sorry if it's there and I missed it, but this is a huge PR with a lot of > > discussion. > > In general, it is better not to introduce variants of the build like this. > > The testing matrix just explodes a bit more. And my understanding from the > > discussion is that this functionality is likely to be turned on anyway, > > otherwise you'll build a crippled jlink without full functionality. > > I have no opinion on whether the opt-in is at configure time or its just make > target (like "make legacy-jre-image"). In the discussions it wasn't meant to > be built by default. If distributions choose to distribute this image then > this will likely influence what they test. Once experience builds up with > using these run-time images then it may be that there is a proposal (and JEP) > to make it the default, meaning images/jdk would not include .jmod files and > multi-hop restriction is removed from jlink. That may be something for much > further down the road. The choice between a configure option to modify the meaning of the `jdk-image` target (and subsequently the `product-bundles` target), and adding an additional `foo-image` target (and bundle definition) mainly comes down to if there is ever a usecase for producing both the current/default jdk image and this new image side by side. Back when we still had a JRE, producing both the JDK and JRE from the same build was required as they were two different supported distributions that users were expected to pick between depending on usecase. This option however sounds much more like an either or choice for the OpenJDK distributor. Unless RedHat is planning on distributing both variants side by side, the configure option makes the most sense. It minimizes the need for further modifications of the makefiles (for defining a new bundle for this new image), the test makefiles (for defining a way to run te
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
On Fri, 8 Mar 2024 17:36:33 GMT, Severin Gehwolf wrote: >> That was modelled similar to `jdk_jlink` target. It also does the removal. >> When building with `--enable-runtime-link-image`, the flow is: >> >> >> 1. Link the initial jdk image (current `images/jdk`). Output is >> $(RL_INTERMEDIATE_IMAGE_DIR) >> 2. Generate the diff to the packaged modules (target `generate_jimage_diff`) >> 3. Do the final link creating a runtime linkable jimage with >> `--create-linkabel-runtime` into `JDK_IMAGE_DIR`. >> >> >> All three steps should have appropriate dependencies on each other. So I >> don't think there is a race. If you see one please let me know! Thanks. > > 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1519686906
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Fri, 8 Mar 2024 15:48:08 GMT, Magnus Ihse Bursie wrote: >> 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 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 ? - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1988358518
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Fri, 8 Mar 2024 15:48:08 GMT, Magnus Ihse Bursie wrote:
>> 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 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1519634810
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Mon, 11 Mar 2024 08:38:53 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 > > doc/building.html line 679: > >> 677: IBM Open XL C/C++ >> 678: The minimum accepted version of Open XL is 17.1.1.4. This is in >> 679: essence clang 13, and will be treated as such by the OpenJDK build > > It is clang 15 not 13. Clang 13 was in 17.1.0 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? - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1519394251
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Fri, 8 Mar 2024 15:31:18 GMT, Magnus Ihse Bursie wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Revert SEARCH_PATH changes > > make/autoconf/flags-cflags.m4 line 687: > >> 685: PICFLAG="-fPIC" >> 686: PIEFLAG="-fPIE" >> 687: elif test "x$TOOLCHAIN_TYPE" = xclang && test "x$OPENJDK_TARGET_OS" = >> xaix; then > > Just a remark: This code has never been executed, since running with clang on > any OS would hit the branch above. Also, the code is syntactically incorrect, > missing a trailing `"`. 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1519347031
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Fri, 8 Mar 2024 15:48:08 GMT, Magnus Ihse Bursie wrote:
>> 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 one
> additional commit since the last revision:
>
> Revert SEARCH_PATH changes
Changes requested by jkern (Author).
doc/building.html line 679:
> 677: IBM Open XL C/C++
> 678: The minimum accepted version of Open XL is 17.1.1.4. This is in
> 679: essence clang 13, and will be treated as such by the OpenJDK build
It is clang 15 not 13. Clang 13 was in 17.1.0
doc/building.md line 493:
> 491:
> 492: The minimum accepted version of Open XL is 17.1.1.4. This is in essence
> clang
> 493: 13, and will be treated as such by the OpenJDK build system.
clang 15 not 13
make/autoconf/toolchain.m4 line 285:
> 283: XLC_TEST_PATH=${TOOLCHAIN_PATH}/
> 284: fi
> 285: if test "x$TOOLCHAIN_TYPE" = xclang; then
Why did you also remove the test for the clang compiler? Ah I see, you moved
the clang compiler check down below
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
-
PR Review: https://git.openjdk.org/jdk/pull/18172#pullrequestreview-1927198641
PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1519321337
PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1519321980
PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1519358938
PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1519365934
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Fri, 8 Mar 2024 15:48:08 GMT, Magnus Ihse Bursie wrote: >> 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 one > additional commit since the last revision: > > Revert SEARCH_PATH changes Hi, thanks for doing this cleanup change. I put it into our build/test queue to see the results on AIX. - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1987807579
Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]
On Wed, 6 Mar 2024 13:43:00 GMT, Magnus Ihse Bursie wrote: >> Currently, our symbol visibility handling for tests are sloppy; we only >> handle it properly on Windows. We need to bring it up to the same levels as >> product code. This is a prerequisite for >> [JDK-8327045](https://bugs.openjdk.org/browse/JDK-8327045), which in turn is >> a building block for Hermetic Java. > > 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 Good cleanup. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18135#pullrequestreview-1927041103
Re: RFR: 8327460: Compile tests with the same visibility rules as product code
On Wed, 6 Mar 2024 12:14:10 GMT, Magnus Ihse Bursie 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. - PR Comment: https://git.openjdk.org/jdk/pull/18135#issuecomment-1987743188
