Re: RFR: 8240567: MethodTooLargeException thrown while creating a jlink image [v6]
On Thu, 8 Dec 2022 07:41:22 GMT, Oliver Kopp wrote: >> Would it be possible to paste in a summary on the VerifyError with the >> previous iteration? If I read the latest update then the limit per helper >> method has been bump to avoid it, is that right? > >> Would it be possible to paste in a summary on the VerifyError with the >> previous iteration? > > Isn't this https://github.com/openjdk/jdk/pull/10704#issuecomment-1286106503? > > Type top (current frame, locals[15]) is not assignable to reference type > >> If I read the latest update then the limit per helper method has been bump >> to avoid it, is that right? > > Yes. Then, the compiler still works - and we can try to debug using the test > case (yet to be finalized). Yes, we are currently trying to set up the test, so it results in the original method too large error. Then we will re add the module splitting. (Working on this together with @koppor ) - PR: https://git.openjdk.org/jdk/pull/10704
Re: RFR: 8240567: MethodTooLargeException thrown while creating a jlink image [v6]
On Tue, 3 Jan 2023 16:17:41 GMT, Alan Bateman wrote: >>> Would it be possible to paste in a summary on the VerifyError with the >>> previous iteration? >> >> Isn't this https://github.com/openjdk/jdk/pull/10704#issuecomment-1286106503? >> >> Type top (current frame, locals[15]) is not assignable to reference type >> >>> If I read the latest update then the limit per helper method has been bump >>> to avoid it, is that right? >> >> Yes. Then, the compiler still works - and we can try to debug using the test >> case (yet to be finalized). > > @koppor Should we continue to just ignore this PR for now? The current patch > is test only, I don't know if that is deliberate or not. @AlanBateman We now have a reproducible JTREG test case that throws the MethodTooLargeException. We will now readd the module splitting handling. - PR: https://git.openjdk.org/jdk/pull/10704
Re: RFR: 8240567: MethodTooLargeException thrown while creating a jlink image [v14]
On Thu, 26 Jan 2023 19:18:52 GMT, Oliver Kopp wrote: >> Fix for [JDK-8240567](https://bugs.openjdk.org/browse/JDK-8240567): >> "MethodTooLargeException thrown while creating a jlink image". >> >> Java still has a 64kb limit: A method may not be longer than 64kb. The idea >> of the fix is to split up the generated methods in several smaller methods > > Oliver Kopp has updated the pull request incrementally with one additional > commit since the last revision: > > remove whitespace We readded the splitting code and the test is passing. In the test we could make it work with up to 130 modules where each module _n_ requires all modules from _0...n_ Any further optimization ideas? - PR: https://git.openjdk.org/jdk/pull/10704
Re: RFR: 8240567: MethodTooLargeException thrown while creating a jlink image
On Sun, 11 Jun 2023 21:01:54 GMT, Oliver Kopp wrote: > Fix for [JDK-8240567](https://bugs.openjdk.org/browse/JDK-8240567): > "MethodTooLargeException thrown while creating a jlink image". > > Java still has a 64kb limit: A method may not be longer than 64kb. The idea > of the fix is to split up the generated methods in several smaller methods > > This is a follow-up to https://github.com/openjdk/jdk/pull/10704. GitHub did > not allow me to re-open the PR, because I did a force-push to have one commit. We validated this by compiling JabRef against it and a) writing out the generated class file for the generated bytecode and b) verifying with -Xlog:init=debug -XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal Is there a way to "parse" or call the verifyer after generation? - PR Comment: https://git.openjdk.org/jdk/pull/14408#issuecomment-1586339612
Re: RFR: 8240567: MethodTooLargeException thrown while creating a jlink image [v2]
On Mon, 12 Jun 2023 05:41:08 GMT, Oliver Kopp wrote:
>> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
>> line 754:
>>
>>> 752: // Restore all (!) sets from parameter to
>>> local variables
>>> 753: if (nextLocalVar > firstVariableForDedup) {
>>> 754: // We need to go from the end to the
>>> beginning as we will probably overwrite position 2 (which holds the list at
>>> the beginning)
>>
>> Do you mind fixing all the comments as these 160+ lines make it impossible
>> to look at the changes side-by-side again. You had fixed that in the
>> original PR but it looks like they have come back with this PR.
>
>> Do you mind fixing all the comments as these 160+ lines
>
> Done. I copied the (for me) important comments to the GitHub reply
> https://github.com/openjdk/jdk/pull/14408#issuecomment-1586615579.
>
>> You had fixed that in the original PR but it looks like they have come back
>> with this PR.
>
> This is my job training 😇: Comment the core ideas (and do not verbatim repeat
> what is said by the code directly). - For me, the good thing is, the diff now
> has all the comments
> (https://github.com/openjdk/jdk/pull/14408/commits/23bbc0ce0c8fd8a4cd689c0260c5fbcb91b20046).
> OK, at least one has go to nirvana in all cases.
Yes, the verify errors are gone. Application (JabRef) starts sucessfully
against the compilation. Is there a way to verify in a tesr the creation of the
helper methos in the bytecode? e.g. something like getDeclaredMethods?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1226170715
RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder
Add new test case with sample modules that contains some requires/exports/uses/provides. We are just unsure if and how we should add some last step of verificaiton with the extracted and decompiled class. Follow up task from https://github.com/openjdk/jdk/pull/14408 - Commit messages: - cleanup - rename and cleanup - revert back to default size of 75 for module desriptors - add decompilation via javap - extract jimage - add some more opens and exports - Merge remote-tracking branch 'origin/fix-8311591' into fix-8311591 - Set module split size to 1 - fix test - test - ... and 2 more: https://git.openjdk.org/jdk/compare/509f80bb...9577e7e8 Changes: https://git.openjdk.org/jdk/pull/15234/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8311591 Stats: 240 lines in 10 files changed: 240 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15234/head:pull/15234 PR: https://git.openjdk.org/jdk/pull/15234
Re: RFR: 8240567: MethodTooLargeException thrown while creating a jlink image [v19]
On Fri, 14 Jul 2023 16:08:12 GMT, Mandy Chung wrote: >>> It's looking pretty good. >> >> Thank you! >> >>> About the test, I don't see `ArrayList::add` in the generated bytecode of >>> `sub2-13`. The dedup string set is used for the targets of qualified >>> exports and opens and uses. The modifiers set of `requires` is deduplicated >>> but not the required module names. >> >> Thank you for the hint. >> >>> I assume you want this be backported. If so, we should give it some baked >>> time after it's integrated to the main line. >> >> Sounds great! >> >>> I'm okay if you want to extend the test in a follow up. >> >> That would be great. Will take time to craft a proper setting. > > @koppor do you have any update on a new test for > [JDK-8311591](https://bugs.openjdk.org/browse/JDK-8311591)? @mlchung Pardon for the delay, we now created a PR with a test case https://github.com/openjdk/jdk/pull/15234 - PR Comment: https://git.openjdk.org/jdk/pull/14408#issuecomment-1673959119
Re: RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v2]
> Add new test case with sample modules that contains some > requires/exports/uses/provides. > > We are just unsure if and how we should add some last step of verificaiton > with the extracted and decompiled class. > > Follow up task from https://github.com/openjdk/jdk/pull/14408 Christoph has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder Co-authored-by: Oliver Kopp - Changes: https://git.openjdk.org/jdk/pull/15234/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=01 Stats: 239 lines in 9 files changed: 239 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15234/head:pull/15234 PR: https://git.openjdk.org/jdk/pull/15234
Re: RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v3]
> Add new test case with sample modules that contains some > requires/exports/uses/provides. > > We are just unsure if and how we should add some last step of verificaiton > with the extracted and decompiled class. > > Follow up task from https://github.com/openjdk/jdk/pull/14408 Christoph has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 15 additional commits since the last revision: - Merge remote-tracking branch 'origin/fix-8311591' into fix-8311591 * origin/fix-8311591: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder - 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder Co-authored-by: Oliver Kopp - Merge remote-tracking branch 'upstream/master' into fix-8311591 * upstream/master: (49 commits) 8313904: [macos] All signing tests which verifies unsigned app images are failing 8314139: TEST_BUG: runtime/os/THPsInThreadStackPreventionTest.java could fail on machine with large number of cores 8313798: [aarch64] sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java sometimes times out on aarch64 8313244: NM flags handling in configure process 8314113: G1: Remove unused G1CardSetInlinePtr::card_at 8311648: Refactor the Arena/Chunk/ChunkPool interface 8313224: Avoid calling JavaThread::current() in MemAllocator::Allocation constructor 8312461: JNI warnings in SunMSCApi provider 8312882: Update the CONTRIBUTING.md with pointers to lifecycle of a PR 8304292: Memory leak related to ClassLoader::update_class_path_entry_list 8313899: JVMCI exception Translation can fail in TranslatedException. 8313633: [macOS] java/awt/dnd/NextDropActionTest/NextDropActionTest.java fails with java.lang.RuntimeException: wrong next drop action! 8312259: StatusResponseManager unused code clean up 8314061: [JVMCI] DeoptimizeALot stress logic breaks deferred barriers 8313905: Checked_cast assert in CDS compare_by_loader 8313654: Test WaitNotifySuspendedVThreadTest.java timed out 8312194: test/hotspot/jtreg/applications/ctw/modules/jdk_crypto_ec.java cannot handle empty modules 8313616: support loading library members on AIX in os::dll_load 8313882: Fix -Wconversion warnings in runtime code 8313239: InetAddress.getCanonicalHostName may return ip address if reverse lookup fails ... - cleanup - rename and cleanup - revert back to default size of 75 for module desriptors use parameter for jlink - add decompilation via javap - extract jimage - add some more opens and exports prepare methods for verifying - Merge remote-tracking branch 'origin/fix-8311591' into fix-8311591 * origin/fix-8311591: Set module split size to 1 - ... and 5 more: https://git.openjdk.org/jdk/compare/406ae9ec...996c187c - Changes: - all: https://git.openjdk.org/jdk/pull/15234/files - new: https://git.openjdk.org/jdk/pull/15234/files/9b9b0bb2..996c187c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=01-02 Stats: 334 lines in 30 files changed: 137 ins; 78 del; 119 mod Patch: https://git.openjdk.org/jdk/pull/15234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15234/head:pull/15234 PR: https://git.openjdk.org/jdk/pull/15234
Re: RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v4]
> Add new test case with sample modules that contains some > requires/exports/uses/provides. > > We are just unsure if and how we should add some last step of verificaiton > with the extracted and decompiled class. > > Follow up task from https://github.com/openjdk/jdk/pull/14408 Christoph has updated the pull request incrementally with two additional commits since the last revision: - Add another required transitive desktop Assert number of module description generated sub modules Co-authored-by: Oliver Kopp - Add copyright header and apply some formatting Revert unnecesary CompilerUtil - Changes: - all: https://git.openjdk.org/jdk/pull/15234/files - new: https://git.openjdk.org/jdk/pull/15234/files/996c187c..1a2e463e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=02-03 Stats: 189 lines in 9 files changed: 171 ins; 5 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/15234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15234/head:pull/15234 PR: https://git.openjdk.org/jdk/pull/15234
Re: RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v3]
On Mon, 14 Aug 2023 22:07:32 GMT, Mandy Chung wrote: > Since the batch size is 1, I would suggest that `p4.Main` can also load > `jdk.internal.module.SystemModules$all` and verify the expected numbers of > `subX` methods (one per each module). To find all modules in the image, you > can simply do `ModuleFinder.ofSystem().findAll()`. Thanks for the suggestion. We added now a check in the main class for the number of generated sub methods. We put a fixed number, as we did not get the relation between the number of "require modules" statements and the total number of modules. - PR Comment: https://git.openjdk.org/jdk/pull/15234#issuecomment-1679458565
Re: RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v5]
> Add new test case with sample modules that contains some > requires/exports/uses/provides. > > We are just unsure if and how we should add some last step of verificaiton > with the extracted and decompiled class. > > Follow up task from https://github.com/openjdk/jdk/pull/14408 Christoph has updated the pull request incrementally with four additional commits since the last revision: - Merge remote-tracking branch 'origin/fix-8311591' into fix-8311591 * origin/fix-8311591: Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java - Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java Co-authored-by: Mandy Chung - Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java Co-authored-by: Mandy Chung - Reformat, add missing copyright header Remove duplicated module in add mods call Co-authored-by: Oliver Kopp - Changes: - all: https://git.openjdk.org/jdk/pull/15234/files - new: https://git.openjdk.org/jdk/pull/15234/files/1a2e463e..015a3c2e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=03-04 Stats: 35 lines in 3 files changed: 30 ins; 1 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/15234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15234/head:pull/15234 PR: https://git.openjdk.org/jdk/pull/15234
Re: RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v6]
> Add new test case with sample modules that contains some > requires/exports/uses/provides. > > We are just unsure if and how we should add some last step of verificaiton > with the extracted and decompiled class. > > Follow up task from https://github.com/openjdk/jdk/pull/14408 Christoph has updated the pull request incrementally with one additional commit since the last revision: Fix missing import, use size instead of count - Changes: - all: https://git.openjdk.org/jdk/pull/15234/files - new: https://git.openjdk.org/jdk/pull/15234/files/015a3c2e..2ebc0592 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=04-05 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15234/head:pull/15234 PR: https://git.openjdk.org/jdk/pull/15234
Re: RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v5]
On Tue, 15 Aug 2023 19:51:44 GMT, Christoph wrote: >> Add new test case with sample modules that contains some >> requires/exports/uses/provides. >> >> We are just unsure if and how we should add some last step of verificaiton >> with the extracted and decompiled class. >> >> Follow up task from https://github.com/openjdk/jdk/pull/14408 > > Christoph has updated the pull request incrementally with four additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/fix-8311591' into fix-8311591 > >* origin/fix-8311591: > Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java > Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java > - Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java > >Co-authored-by: Mandy Chung > - Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java > >Co-authored-by: Mandy Chung > - Reformat, add missing copyright header > >Remove duplicated module in add mods call > >Co-authored-by: Oliver Kopp Thanks a lot for your explanation and suggestions! Now we understood this :) - PR Comment: https://git.openjdk.org/jdk/pull/15234#issuecomment-1679515563
Re: RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v7]
> Add new test case with sample modules that contains some > requires/exports/uses/provides. > > We are just unsure if and how we should add some last step of verificaiton > with the extracted and decompiled class. > > Follow up task from https://github.com/openjdk/jdk/pull/14408 Christoph has updated the pull request incrementally with one additional commit since the last revision: remove obsolete jimage and decompile methods - Changes: - all: https://git.openjdk.org/jdk/pull/15234/files - new: https://git.openjdk.org/jdk/pull/15234/files/2ebc0592..2bcdd2fc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=05-06 Stats: 30 lines in 1 file changed: 0 ins; 30 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15234/head:pull/15234 PR: https://git.openjdk.org/jdk/pull/15234
Re: RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v8]
> Add new test case with sample modules that contains some > requires/exports/uses/provides. > > We are just unsure if and how we should add some last step of verificaiton > with the extracted and decompiled class. > > Follow up task from https://github.com/openjdk/jdk/pull/14408 Christoph has updated the pull request incrementally with one additional commit since the last revision: remove obsolete constant - Changes: - all: https://git.openjdk.org/jdk/pull/15234/files - new: https://git.openjdk.org/jdk/pull/15234/files/2bcdd2fc..94fcd8d1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=06-07 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15234/head:pull/15234 PR: https://git.openjdk.org/jdk/pull/15234
Re: RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v7]
On Tue, 15 Aug 2023 20:41:03 GMT, Mandy Chung wrote:
>> Christoph has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove obsolete jimage and decompile methods
>
> test/jdk/tools/jlink/dedup/src/m4/p4/Main.java line 35:
>
>> 33:
>> 34: public class Main {
>> 35: private final static int MODULE_SUB_METHOD_COUNT = 9;
>
> This is leftover and should be deleted
Ah yeah overlooked that!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15234#discussion_r1295092962
Re: RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v9]
> Add new test case with sample modules that contains some > requires/exports/uses/provides. > > We are just unsure if and how we should add some last step of verificaiton > with the extracted and decompiled class. > > Follow up task from https://github.com/openjdk/jdk/pull/14408 Christoph has updated the pull request incrementally with one additional commit since the last revision: Update full name - Changes: - all: https://git.openjdk.org/jdk/pull/15234/files - new: https://git.openjdk.org/jdk/pull/15234/files/94fcd8d1..42df3f5c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15234&range=07-08 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15234/head:pull/15234 PR: https://git.openjdk.org/jdk/pull/15234
Integrated: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder
On Thu, 10 Aug 2023 21:42:41 GMT, Christoph wrote: > Add new test case with sample modules that contains some > requires/exports/uses/provides. > > We are just unsure if and how we should add some last step of verificaiton > with the extracted and decompiled class. > > Follow up task from https://github.com/openjdk/jdk/pull/14408 This pull request has now been integrated. Changeset: bc8e9f44 Author:Christoph Schwentker Committer: Mandy Chung URL: https://git.openjdk.org/jdk/commit/bc8e9f44a39ff59b59b2d1d5d546a148be75a2f2 Stats: 355 lines in 9 files changed: 351 ins; 0 del; 4 mod 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/15234
Re: RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v7]
On Tue, 15 Aug 2023 20:41:13 GMT, Mandy Chung wrote: >> Christoph has updated the pull request incrementally with one additional >> commit since the last revision: >> >> remove obsolete jimage and decompile methods > > Marked as reviewed by mchung (Reviewer). @mlchung Thanks again for your help. What would be needed to get the at https://github.com/openjdk/jdk/commit/ec7da91bd83803b7d91a4de3a01caf0ba256c037 back ported to JDK21? I guess for the initial release it's too late but maybe for one of the jdk21u releases? That would be really a huge step forward for us at JabRef. (Currently we rely on a custom build) - PR Comment: https://git.openjdk.org/jdk/pull/15234#issuecomment-1680929166
Re: RFR: 8315383: jlink SystemModulesPlugin incorrectly parses the options [v2]
On Wed, 30 Aug 2023 19:30:37 GMT, Mandy Chung wrote: >> Oliver Kopp has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove obsolete imports > > Looks good. Thanks for catching this. > > There are a few unused imports in JLinkDedupTestBatchSizeOne.java. Can you > remove them as you are in that file? @mlchung Could you please do the backport for us? We don't have the rights. Thanks in advance! - PR Comment: https://git.openjdk.org/jdk/pull/15495#issuecomment-1702268892
Re: RFR: 8342576: [macos] AppContentTest still fails after JDK-8341443 for same reason on older macOS versions
On Fri, 25 Oct 2024 01:49:01 GMT, Alexander Matveev wrote: > - It is not clear on which macOS versions codesign fails if application > bundle contains additional content. > - As a result test was modified to generate only application image, since PKG > or DMG cannot be generated if signing fails. Exit code of jpackage is > ignored, but generated application image will be checked for additional > content. > - This change is for macOS only. > - Previous implementation of test (forcing expected exist code to 1) was not > doing anything useful, since we never checked if additional content was > copied or not. >From the GitHub actions run with double signing: (mac os 14 runner) jpackage Log starts at `Creating app package: JabRef.app` [signed_image_signed_dmg_macOS (ARM64) installer and portable version.txt](https://github.com/user-attachments/files/20016626/signed_image_signed_dmg_macOS.ARM64.installer.and.portable.version.txt) Run with only signing the pkg/dmg: [only signing dmg_macOS (ARM64) installer and portable version.txt](https://github.com/user-attachments/files/20016647/only.signing.dmg_macOS.ARM64.installer.and.portable.version.txt) - PR Comment: https://git.openjdk.org/jdk/pull/21698#issuecomment-2847804315
Re: RFR: 8342576: [macos] AppContentTest still fails after JDK-8341443 for same reason on older macOS versions
On Fri, 25 Oct 2024 01:49:01 GMT, Alexander Matveev wrote: > - It is not clear on which macOS versions codesign fails if application > bundle contains additional content. > - As a result test was modified to generate only application image, since PKG > or DMG cannot be generated if signing fails. Exit code of jpackage is > ignored, but generated application image will be checked for additional > content. > - This change is for macOS only. > - Previous implementation of test (forcing expected exist code to 1) was not > doing anything useful, since we never checked if additional content was > copied or not. When supplying mac sign to building the image, it does not work either and then applying. The current branch is here in this PR https://github.com/JabRef/jabref/pull/13032/ And now I remember that probably this was the initial reason for using the original approach mentioned earlier with specifcying the app content "severity": "error", "code": null, "path": "JabRef-6.0_arm64.dmg/JabRef.app/Contents/MacOS/JabRef", "message": "The signature of the binary is invalid.", "docUrl": "https://developer.apple.com/documentation/security/notarizing_macos_software_before_distribution/resolving_common_notarization_issues#3087735";, "architecture": "arm64" } - PR Comment: https://git.openjdk.org/jdk/pull/21698#issuecomment-2847757982
Re: RFR: 8342576: [macos] AppContentTest still fails after JDK-8341443 for same reason on older macOS versions
On Fri, 25 Oct 2024 01:49:01 GMT, Alexander Matveev wrote: > - It is not clear on which macOS versions codesign fails if application > bundle contains additional content. > - As a result test was modified to generate only application image, since PKG > or DMG cannot be generated if signing fails. Exit code of jpackage is > ignored, but generated application image will be checked for additional > content. > - This change is for macOS only. > - Previous implementation of test (forcing expected exist code to 1) was not > doing anything useful, since we never checked if additional content was > copied or not. Just tried it with putting the additional files into the Resources folder. Same error as originally reported. Run: https://github.com/JabRef/jabref/actions/runs/14823769155/job/41614205909 Changes from PR: https://github.com/JabRef/jabref/pull/13032/files#diff-5a17873aec4eae6b52b00959d8f9e17672912858f63181d39de8c3a713e90018R135-R144 "codesign" failed and additional application content was supplied via the "--app-content" parameter. Probably the additional content broke the integrity of the application bundle and caused the failure. Ensure content supplied via the "--app-content" parameter does not break the integrity of the application bundle, or add it in the post-processing step. Error: "codesign" failed with following output: /var/folders/hn/k7g0_sh57112t0xtjxcjcm5rgn/T/jdk.jpackage2295206181121069675/images/image-11269735772913219400/JabRef.app: replacing existing signature /var/folders/hn/k7g0_sh57112t0xtjxcjcm5rgn/T/jdk.jpackage2295206181121069675/images/image-11269735772913219400/JabRef.app: code object is not signed at all In subcomponent: /private/var/folders/hn/k7g0_sh57112t0xtjxcjcm5rgn/T/jdk.jpackage2295206181121069675/images/image-11269735772913219400/JabRef.app/Contents/***Host.py [17:58:51.622] java.io.IOException: Command [/usr/bin/codesign, -s, Developer ID Application: JabRef e.V. (6792V39SK3), -, --timestamp, --options, runtime, --prefix, org.***, --entitlements, buildres/mac/***.entitlements, --force, /var/folders/hn/k7g0_sh57112t0xtjxcjcm5rgn/T/jdk.jpackage2295206181121069675/images/image-11269735772913219400/JabRef.app] exited with 1 code at jdk.jpackage/jdk.jpackage.internal.Executor.executeExpectSuccess(Executor.java:90) at jdk.jpackage/jdk.jpackage.internal.IOUtils.exec(IOUtils.java:125) at jdk.jpackage/jdk.jpackage.internal.MacAppImageBuilder.runCodesign(MacAppImageBuilder.java:740) at jdk.jpackage/jdk.jpackage.internal.MacAppImageBuilder.signAppBundle(MacAppImageBuilder.java:907) at jdk.jpackage/jdk.jpackage.internal.MacAppImageBuilder.doSigning(MacAppImageBuilder.java:414) at jdk.jpackage/jdk.jpackage.internal.MacAppImageBuilder.prepareApplicationFiles(MacAppImageBuilder.java:365) at jdk.jpackage/jdk.jpackage.internal.AppImageBundler.createAppBundle(AppImageBundler.java:189) at jdk.jpackage/jdk.jpackage.internal.AppImageBundler.execute(AppImageBundler.java:93) at jdk.jpackage/jdk.jpackage.internal.MacBaseInstallerBundler.prepareAppBundle(MacBaseInstallerBundler.java:201) at jdk.jpackage/jdk.jpackage.internal.MacDmgBundler.bundle(MacDmgBundler.java:83) at jdk.jpackage/jdk.jpackage.internal.MacDmgBundler.execute(MacDmgBundler.java:579) at jdk.jpackage/jdk.jpackage.internal.Arguments.generateBundle(Arguments.java:707) at jdk.jpackage/jdk.jpackage.internal.Arguments.processArguments(Arguments.java:554) at jdk.jpackage/jdk.jpackage.main.Main.execute(Main.java:92) at jdk.jpackage/jdk.jpackage.main.Main.main(Main.java:53) - PR Comment: https://git.openjdk.org/jdk/pull/21698#issuecomment-2849343813
Re: RFR: 8342576: [macos] AppContentTest still fails after JDK-8341443 for same reason on older macOS versions
On Fri, 25 Oct 2024 01:49:01 GMT, Alexander Matveev wrote: > - It is not clear on which macOS versions codesign fails if application > bundle contains additional content. > - As a result test was modified to generate only application image, since PKG > or DMG cannot be generated if signing fails. Exit code of jpackage is > ignored, but generated application image will be checked for additional > content. > - This change is for macOS only. > - Previous implementation of test (forcing expected exist code to 1) was not > doing anything useful, since we never checked if additional content was > copied or not. Thanks all for your support, with a single app-content parameter and the resource directory this worked with codesign and even notarization worked on both macOS13 and macOS14 (arm) It would be great if you document this in the manual/help - PR Comment: https://git.openjdk.org/jdk/pull/21698#issuecomment-2850262585
Re: RFR: 8342576: [macos] AppContentTest still fails after JDK-8341443 for same reason on older macOS versions
On Fri, 25 Oct 2024 01:49:01 GMT, Alexander Matveev wrote: > - It is not clear on which macOS versions codesign fails if application > bundle contains additional content. > - As a result test was modified to generate only application image, since PKG > or DMG cannot be generated if signing fails. Exit code of jpackage is > ignored, but generated application image will be checked for additional > content. > - This change is for macOS only. > - Previous implementation of test (forcing expected exist code to 1) was not > doing anything useful, since we never checked if additional content was > copied or not. This is unfortunate. However, then how do I add content to the app image before? Our current workaround is to build an app-image only and then manually pack it but this misses the dmg background images and install instructions etc https://github.com/JabRef/jabref/blob/97aaab58e3ab543a0f50097ee59e52504d99c786/.github/workflows/deployment.yml#L167-L178 - PR Comment: https://git.openjdk.org/jdk/pull/21698#issuecomment-2839847379
Re: RFR: 8342576: [macos] AppContentTest still fails after JDK-8341443 for same reason on older macOS versions
On Fri, 25 Oct 2024 01:49:01 GMT, Alexander Matveev wrote: > - It is not clear on which macOS versions codesign fails if application > bundle contains additional content. > - As a result test was modified to generate only application image, since PKG > or DMG cannot be generated if signing fails. Exit code of jpackage is > ignored, but generated application image will be checked for additional > content. > - This change is for macOS only. > - Previous implementation of test (forcing expected exist code to 1) was not > doing anything useful, since we never checked if additional content was > copied or not. I managed to get it working now. 1. Build a signed app image with --mac-sign 2. Add the addtional content 3. re-sign the content manually with # Sign the final artifact codesign --force --deep --sign "Developer ID Application: JabRef e.V. (6792V39SK3)" \ --entitlements buildres/mac/jabref.entitlements \ --options runtime --timestamp build/distribution/JabRef.app 4. build a signed dmg/pkg 5. Notarization works now :) - PR Comment: https://git.openjdk.org/jdk/pull/21698#issuecomment-2848520498
Re: RFR: 8342576: [macos] AppContentTest still fails after JDK-8341443 for same reason on older macOS versions
On Fri, 25 Oct 2024 01:49:01 GMT, Alexander Matveev wrote: > - It is not clear on which macOS versions codesign fails if application > bundle contains additional content. > - As a result test was modified to generate only application image, since PKG > or DMG cannot be generated if signing fails. Exit code of jpackage is > ignored, but generated application image will be checked for additional > content. > - This change is for macOS only. > - Previous implementation of test (forcing expected exist code to 1) was not > doing anything useful, since we never checked if additional content was > copied or not. Followed the stepa and built an unsigned image first and then signed the jpackage build and created the pkg/dmg files. However, notarization fails as it says it's not signed at all: e.g.: > "path": "JabRef-6.0_arm64.dmg/JabRef.app/Contents/MacOS/JabRef", "message": "The binary is not signed with a valid Developer ID certificate.", [ notarylog.log](https://github.com/user-attachments/files/20016162/notarylog.log) Testing now with a presigned image. - PR Comment: https://git.openjdk.org/jdk/pull/21698#issuecomment-2847722058
Re: RFR: 8342576: [macos] AppContentTest still fails after JDK-8341443 for same reason on older macOS versions
On Fri, 25 Oct 2024 01:49:01 GMT, Alexander Matveev wrote: > - It is not clear on which macOS versions codesign fails if application > bundle contains additional content. > - As a result test was modified to generate only application image, since PKG > or DMG cannot be generated if signing fails. Exit code of jpackage is > ignored, but generated application image will be checked for additional > content. > - This change is for macOS only. > - Previous implementation of test (forcing expected exist code to 1) was not > doing anything useful, since we never checked if additional content was > copied or not. Jpackage does include the options as well from the logs: 2025-05-02T17:19:25.6907840Z [17:19:12.379] Preparing Info.plist: /Users/runner/work/***/***/jabgui/build/distribution/JabRef.app/Contents/Info.plist. 2025-05-02T17:19:25.6908750Z [17:19:12.383] Using custom package resource [Application Info.plist] (loaded from Info.plist). 2025-05-02T17:19:25.6909590Z [17:19:12.388] Using custom package resource [Java Runtime Info.plist] (loaded from Runtime-Info.plist). 2025-05-02T17:19:25.6910240Z [17:19:23.350] Running /usr/bin/codesign 2025-05-02T17:19:25.6910740Z [17:19:24.530] Command [PID: 6752]: 2025-05-02T17:19:25.6911920Z /usr/bin/codesign -s Developer ID Application: JabRef e.V. (6792V39SK3) - --timestamp --options runtime --prefix org.*** --entitlements buildres/mac/***.entitlements --force build/distribution/JabRef.app/Contents/runtime 2025-05-02T17:19:25.6912950Z [17:19:24.530] Output: 2025-05-02T17:19:25.6914200Z build/distribution/JabRef.app/Contents/runtime: replacing existing signature 2025-05-02T17:19:25.6915140Z build/distribution/JabRef.app/Contents/runtime: signed bundle with Mach-O thin (arm64) [com.oracle.java.JabRef] 2025-05-02T17:19:25.6915990Z [17:19:24.530] Returned: 0 2025-05-02T17:19:25.6916510Z [17:19:24.530] Running /usr/bin/codesign 2025-05-02T17:19:25.6917100Z [17:19:25.655] Command [PID: 6759]: 2025-05-02T17:19:25.6918000Z /usr/bin/codesign -s Developer ID Application: JabRef e.V. (6792V39SK3) - --timestamp --options runtime --prefix org.*** --entitlements buildres/mac/***.entitlements --force build/distribution/JabRef.app 2025-05-02T17:19:25.6918880Z [17:19:25.656] Output: 2025-05-02T17:19:25.6919430Z build/distribution/JabRef.app: replacing existing signature 2025-05-02T17:19:25.6920230Z build/distribution/JabRef.app: signed app bundle with Mach-O thin (arm64) [org.***JabRef] 2025-05-02T17:19:25.6920880Z [17:19:25.656] Returned: 0 2025-05-02T17:19:25.6921160Z 2025-05-02T17:19:25.6921510Z [17:19:25.656] Succeeded in building Mac Application Image package 2025-05-02T17:19:36.0557030Z Warning: Support for per-user configuration of the installed application will not be supported due to missing "build/distribution/JabRef.app/Contents/app/.package" in predefined signed application image. After creating the app image we put additional content in it under Resources. That probably affects the integrity? of the whole stuff **Runtime** options is for Hardened Runtime https://developer.apple.com/documentation/security/hardened-runtime which allows specifying exclusions like jit in the entitlements **Timestamp** is also required https://developer.apple.com/documentation/security/resolving-common-notarization-issues#Include-a-secure-timestamp Otherwise, notarization fails with no timestamp or invalid timestamp. **Deep** is like going recursively through the files. But should be avoided. I will try without as well To upload a macOS app to be notarized, you must enable the Hardened Runtime capability. For more information about notarization, see [Notarizing macOS software before distribution](https://developer.apple.com/documentation/security/notarizing-macos-software-before-distribution). - PR Comment: https://git.openjdk.org/jdk/pull/21698#issuecomment-2848675876
Re: RFR: 8342576: [macos] AppContentTest still fails after JDK-8341443 for same reason on older macOS versions
On Sat, 3 May 2025 20:56:00 GMT, Alexey Semenyuk wrote: > jpackage doesn't produce a notarizable app image if --app-content is used. > Filed [JDK-8356117](https://bugs.openjdk.org/browse/JDK-8356117) Actually, this is a regression as it worked in JDK23 where we just used it recently https://github.com/JabRef/jabref/blob/6d0d78716893cc09577a957d111d62ba2dfbefd0/.github/workflows/deployment-arm64.yml#L115-L126 Just changing from 23 to 24 led to this error as reported by @koppor earlier in this thread https://github.com/openjdk/jdk/pull/21698#issuecomment-2838481133 The binaries of the release were successfully notarized both on macOS13 and macOS14 (About dialog of JabRef shows the jdk version used as well, 23.0.1) https://github.com/JabRef/jabref/releases/tag/v6.0-alpha2 - PR Comment: https://git.openjdk.org/jdk/pull/21698#issuecomment-2848810724
RFR: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
It seems the error is gone meanwhile. So we can reenable the test. - Commit messages: - JDK-8211847 Changes: https://git.openjdk.org/jdk/pull/19691/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19691&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8211847 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19691.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19691/head:pull/19691 PR: https://git.openjdk.org/jdk/pull/19691
Re: RFR: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
On Thu, 13 Jun 2024 09:47:25 GMT, Christoph Langer wrote: > It seems the error is gone meanwhile. So we can reenable the test. Maybe [this one](https://github.com/openjdk/jdk/commit/4d9042043ecade75d50c25574a445e6b8ef43618)? But just guessing... - PR Comment: https://git.openjdk.org/jdk/pull/19691#issuecomment-2165243238
Re: RFR: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
On Thu, 13 Jun 2024 09:47:25 GMT, Christoph Langer wrote: > It seems the error is gone meanwhile. So we can reenable the test. Trivial fix of test listing, so - PR Comment: https://git.openjdk.org/jdk/pull/19691#issuecomment-2165639694
Integrated: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
On Thu, 13 Jun 2024 09:47:25 GMT, Christoph Langer wrote: > It seems the error is gone meanwhile. So we can reenable the test. This pull request has now been integrated. Changeset: f5213671 Author: Christoph Langer URL: https://git.openjdk.org/jdk/commit/f5213671f7b636b32bb93c78e43696a61cd69bae Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected" Reviewed-by: stuefe - PR: https://git.openjdk.org/jdk/pull/19691
[jdk23] RFR: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
Hi all, This pull request contains a backport of [JDK-8211847](https://bugs.openjdk.org/browse/JDK-8211847), commit [f5213671](https://github.com/openjdk/jdk/commit/f5213671f7b636b32bb93c78e43696a61cd69bae) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Christoph Langer on 13 Jun 2024 and was reviewed by Thomas Stuefe. Thanks! - Commit messages: - Backport f5213671f7b636b32bb93c78e43696a61cd69bae Changes: https://git.openjdk.org/jdk/pull/19742/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19742&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8211847 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19742.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19742/head:pull/19742 PR: https://git.openjdk.org/jdk/pull/19742
[jdk23] Integrated: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
On Mon, 17 Jun 2024 08:19:18 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8211847](https://bugs.openjdk.org/browse/JDK-8211847), commit > [f5213671](https://github.com/openjdk/jdk/commit/f5213671f7b636b32bb93c78e43696a61cd69bae) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Christoph Langer on 13 Jun 2024 > and was reviewed by Thomas Stuefe. > > Thanks! This pull request has now been integrated. Changeset: 4e3bfc92 Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/4e3bfc926ebdf512c7b9dbddc8caa7b66e2777f7 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected" Reviewed-by: mbaesken Backport-of: f5213671f7b636b32bb93c78e43696a61cd69bae - PR: https://git.openjdk.org/jdk/pull/19742
Re: RFR: 8334441: Mark tests in jdk_security_infra group as manual
On Thu, 20 Jun 2024 18:35:00 GMT, Rajan Halade wrote: > Updated all the tests that depend on external infrastructure services as > manual. These tests may fail with external reasons, for instance - change in > CA test portal, certificate status updates, or network issues. Looks good, although I don't see why sometimes the directive is @run main/othervm/manual/manual (double occurence of manual). test/jdk/security/infra/java/security/cert/CertPathValidator/certification/CAInterop.java line 30: > 28: * @library /test/lib > 29: * @build jtreg.SkippedException ValidatePathWithURL CAInterop > 30: * @run main/othervm/manual/manual -Djava.security.debug=certpath,ocsp What is the reason why manual is written twice? (Here and in all other places in this file...) - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19814#pullrequestreview-2132623582 PR Review Comment: https://git.openjdk.org/jdk/pull/19814#discussion_r1648943324
Re: [jdk23] RFR: 8334441: Mark tests in jdk_security_infra group as manual
On Sat, 22 Jun 2024 08:07:54 GMT, SendaoYan wrote: > Hi all, > > This pull request contains a backport of commit > [8e1d2b09](https://github.com/openjdk/jdk/commit/8e1d2b091c9a311d98a0b886a803fb18d4405d8a) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Rajan Halade on 21 Jun 2024 and > was reviewed by Christoph Langer and Sean Mullan. > > Thanks! Thanks for doing the backport. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19841#pullrequestreview-2133855195
[jdk23] RFR: 8222884: ConcurrentClassDescLookup.java times out intermittently
Hi all, This pull request contains a backport of [JDK-8222884](https://bugs.openjdk.org/browse/JDK-8222884), commit [bd046d9b](https://github.com/openjdk/jdk/commit/bd046d9b9e79e4eea89c72af358961ef6e98e660) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Jaikiran Pai on 12 Jun 2024 and was reviewed by Roger Riggs and Matthias Baesken. Thanks! - Commit messages: - Backport bd046d9b9e79e4eea89c72af358961ef6e98e660 Changes: https://git.openjdk.org/jdk/pull/19853/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19853&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8222884 Stats: 36 lines in 1 file changed: 7 ins; 7 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/19853.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19853/head:pull/19853 PR: https://git.openjdk.org/jdk/pull/19853
[jdk23] Integrated: 8222884: ConcurrentClassDescLookup.java times out intermittently
On Mon, 24 Jun 2024 09:40:14 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8222884](https://bugs.openjdk.org/browse/JDK-8222884), commit > [bd046d9b](https://github.com/openjdk/jdk/commit/bd046d9b9e79e4eea89c72af358961ef6e98e660) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Jaikiran Pai on 12 Jun 2024 and > was reviewed by Roger Riggs and Matthias Baesken. > > Thanks! This pull request has now been integrated. Changeset: 08c7c383 Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/08c7c38342809c60f5fdea70717362a72b00f6e9 Stats: 36 lines in 1 file changed: 7 ins; 7 del; 22 mod 8222884: ConcurrentClassDescLookup.java times out intermittently Reviewed-by: mdoerr Backport-of: bd046d9b9e79e4eea89c72af358961ef6e98e660 - PR: https://git.openjdk.org/jdk/pull/19853
Re: RFR: 8339364: AIX build fails: various unused variable and function warnings
On Mon, 2 Sep 2024 11:43:20 GMT, Matthias Baesken wrote: > We get a couple of warnings as errors on AIX because of unused variables or > functions , for example : > /priv/jenkins/client-home/workspace/openjdk-jdk-dev-aix_ppc64-opt/jdk/src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c:665:10: > error: unused variable 'exePath' [-Werror,-Wunused-variable] > char exePath[PATH_MAX]; > ^ > /priv/jenkins/client-home/workspace/openjdk-jdk-dev-aix_ppc64-opt/jdk/src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c:668:9: > error: unused variable 'ret' [-Werror,-Wunused-variable] > int ret; > ^ > /priv/jenkins/client-home/workspace/openjdk-jdk-dev-aix_ppc64-opt/jdk/src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c:664:10: > error: unused variable 'fn' [-Werror,-Wunused-variable] > char fn[32]; > ^ > > This seems to be related to the recent make changes > 8339156: Use more fine-granular clang unused warnings > https://bugs.openjdk.org/browse/JDK-8339156 > (we use clang too on AIX) Looks good and seems like the warning helped to clean out coding in a few places. src/java.desktop/unix/native/common/awt/X11Color.c line 1236: > 1234: for (int i = 0; i < num_colors; i++) > 1235: alloc_col (awt_display, awtData->awt_cmap, red (rgbColors [i]), > 1236: green (rgbColors [i]), blue (rgbColors [i]), -1, Seems like indentation here could be improved a bit. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20812#pullrequestreview-2275486151 PR Review Comment: https://git.openjdk.org/jdk/pull/20812#discussion_r1740824222
[jdk20] RFR: 8299439: java/text/Format/NumberFormat/CurrencyFormat.java fails for hr_HR
Hi all, This pull request contains a backport of [JDK-8299439](https://bugs.openjdk.org/browse/JDK-8299439), commit [3b374c01](https://github.com/openjdk/jdk/commit/3b374c0153950ab193f3a188b57d3404b4ce2fe2) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Justin Lu on 4 Jan 2023 and was reviewed by Naoto Sato, Lance Andersen and Jaikiran Pai. Thanks! - Commit messages: - Backport 3b374c0153950ab193f3a188b57d3404b4ce2fe2 Changes: https://git.openjdk.org/jdk20/pull/96/files Webrev: https://webrevs.openjdk.org/?repo=jdk20&pr=96&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299439 Stats: 8 lines in 3 files changed: 1 ins; 2 del; 5 mod Patch: https://git.openjdk.org/jdk20/pull/96.diff Fetch: git fetch https://git.openjdk.org/jdk20 pull/96/head:pull/96 PR: https://git.openjdk.org/jdk20/pull/96
Re: [jdk20] RFR: 8299439: java/text/Format/NumberFormat/CurrencyFormat.java fails for hr_HR
On Wed, 11 Jan 2023 17:01:26 GMT, Naoto Sato wrote: >> Hi all, >> >> This pull request contains a backport of >> [JDK-8299439](https://bugs.openjdk.org/browse/JDK-8299439), commit >> [3b374c01](https://github.com/openjdk/jdk/commit/3b374c0153950ab193f3a188b57d3404b4ce2fe2) >> from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. >> >> The commit being backported was authored by Justin Lu on 4 Jan 2023 and was >> reviewed by Naoto Sato, Lance Andersen and Jaikiran Pai. >> >> Thanks! > > Looks good. Thanks for confirming, @naotoj - PR: https://git.openjdk.org/jdk20/pull/96
[jdk20] Integrated: 8299439: java/text/Format/NumberFormat/CurrencyFormat.java fails for hr_HR
On Wed, 11 Jan 2023 09:21:18 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8299439](https://bugs.openjdk.org/browse/JDK-8299439), commit > [3b374c01](https://github.com/openjdk/jdk/commit/3b374c0153950ab193f3a188b57d3404b4ce2fe2) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Justin Lu on 4 Jan 2023 and was > reviewed by Naoto Sato, Lance Andersen and Jaikiran Pai. > > Thanks! This pull request has now been integrated. Changeset: 752a3701 Author:Christoph Langer URL: https://git.openjdk.org/jdk20/commit/752a37016f49ef8f2405dd74f96f58f80d606cd3 Stats: 8 lines in 3 files changed: 1 ins; 2 del; 5 mod 8299439: java/text/Format/NumberFormat/CurrencyFormat.java fails for hr_HR Reviewed-by: naoto Backport-of: 3b374c0153950ab193f3a188b57d3404b4ce2fe2 - PR: https://git.openjdk.org/jdk20/pull/96
RE: JDK 20 EA builds (archive?)
Hi Chris, SapMachine has all the ea builds in its GitHub Repo: https://github.com/SAP/SapMachine/releases. Should be fine enough for chasing G1 GC behaviour changes. Cheers Christoph > -Original Message- > From: core-libs-dev On Behalf Of Chris > Hegarty > Sent: Freitag, 24. März 2023 17:56 > To: core-libs-dev@openjdk.org > Subject: JDK 20 EA builds (archive?) > > Hi, > > While definitely not the right list, someone here will surely know ... > > Where can I download archive EA builds of JDK 20, so I can perform bisect > experiments to help narrow when something changed between JDK 19 and JDK > 20. ( I have some builds, but not that many ) > > --- > Informational: I'm not expecting anyone to do my work for me ;-) but if you > have ideas or hints! > > I'm try to track down a change in G1 GC behaviour, where the number of GC's > of the Young generation has decreased ( by ~20% ), but the cumulative total > time spent in GC of the Young generation has increased marginally ( ~2% ) for > a > particular workload benchmark ( that runs for a medium amount of time, ~20 > mins ). > > Thanks, > -Chris.
Re: RFR: JDK-8308300: enhance exceptions in MappedMemoryUtils.c [v2]
On Fri, 19 May 2023 09:46:59 GMT, Matthias Baesken wrote: >> MappedMemoryUtils.c can generate exceptions like those : >> java.io.UncheckedIOException: java.io.IOException: Invalid argument >>at >> java.base/java.nio.MappedMemoryUtils.force(MappedMemoryUtils.java:105) >>at java.base/java.nio.Buffer$2.force(Buffer.java:890) >>at >> java.base/jdk.internal.misc.ScopedMemoryAccess.forceInternal(ScopedMemoryAccess.java:317) >>at >> java.base/jdk.internal.misc.ScopedMemoryAccess.force(ScopedMemoryAccess.java:305) >>at >> java.base/jdk.internal.foreign.MappedMemorySegmentImpl.force(MappedMemorySegmentImpl.java:92) >>at >> TestByteBuffer.testMappedSegmentAsByteBuffer(TestByteBuffer.java:327) >> >> (we see this for example on AIX); there is some room for improvement, at >> least the info should be added that msync failed and caused this exception. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Change comments and madvise exception text Marked as reviewed by clanger (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14054#pullrequestreview-1434195594
[jdk19] RFR: 8287672: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails intermittently in nightly run
This pull request contains a backport of [JDK-8287672](https://bugs.openjdk.org/browse/JDK-8287672), commit [7e211d7d](https://github.com/openjdk/jdk/commit/7e211d7daac32dca8f26f408d1a3b2c7805b5a2e) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Rob McKenna on 21 Jun 2022 and was reviewed by Daniel Fuchs and Aleksei Efimov. It has already been brought to jdk19u but as it is a test fix I think it should still go into jdk19 as well. - Commit messages: - Backport 7e211d7daac32dca8f26f408d1a3b2c7805b5a2e Changes: https://git.openjdk.org/jdk19/pull/97/files Webrev: https://webrevs.openjdk.org/?repo=jdk19&pr=97&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8287672 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk19/pull/97.diff Fetch: git fetch https://git.openjdk.org/jdk19 pull/97/head:pull/97 PR: https://git.openjdk.org/jdk19/pull/97
Re: RFR: JDK-8289569: [test] java/lang/ProcessBuilder/Basic.java fails on Alpine/musl
On Mon, 4 Jul 2022 07:05:03 GMT, Matthias Baesken wrote: > Currently the ProcessBuilder/Basic.java test fails on musl. > We run into >>'java.io.IOException: Cannot run program "./prog": error=8, Exec format error > at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1143) > at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1073) > at Basic.run(Basic.java:2771) > at Basic$JavaChild.main(Basic.java:498) > Caused by: java.io.IOException: error=8, Exec format error > at java.base/java.lang.ProcessImpl.forkAndExec(Native Method) > at java.base/java.lang.ProcessImpl.(ProcessImpl.java:319) > at java.base/java.lang.ProcessImpl.start(ProcessImpl.java:249) > at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1110) > ... 3 more > > This seems to be a musl/Alpine specific issue with some process execs. > So adding !vm.musl to the test might make sense. Makes sense to me. Actually, since this is a test fix, I'd prefer if you could do it directly in jdk19. We see the failures in our Alpine testing there, too. - Marked as reviewed by clanger (Reviewer). PR: https://git.openjdk.org/jdk/pull/9361
[jdk19] Integrated: 8287672: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails intermittently in nightly run
On Fri, 1 Jul 2022 09:10:07 GMT, Christoph Langer wrote: > This pull request contains a backport of > [JDK-8287672](https://bugs.openjdk.org/browse/JDK-8287672), commit > [7e211d7d](https://github.com/openjdk/jdk/commit/7e211d7daac32dca8f26f408d1a3b2c7805b5a2e) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Rob McKenna on 21 Jun 2022 and > was reviewed by Daniel Fuchs and Aleksei Efimov. > > It has already been brought to jdk19u but as it is a test fix I think it > should still go into jdk19 as well. This pull request has now been integrated. Changeset: 5b5bc6c2 Author:Christoph Langer URL: https://git.openjdk.org/jdk19/commit/5b5bc6c26e9843e16f241b89853a3a1fa5ae61f0 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod 8287672: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails intermittently in nightly run Reviewed-by: stuefe Backport-of: 7e211d7daac32dca8f26f408d1a3b2c7805b5a2e - PR: https://git.openjdk.org/jdk19/pull/97
Re: [jdk19] RFR: 8289569: [test] java/lang/ProcessBuilder/Basic.java fails on Alpine/musl
On Mon, 4 Jul 2022 10:39:20 GMT, Matthias Baesken wrote: > 8289569: [test] java/lang/ProcessBuilder/Basic.java fails on Alpine/musl Thanks for bringing it to jdk19. - Marked as reviewed by clanger (Reviewer). PR: https://git.openjdk.org/jdk19/pull/106
[jdk19] RFR: 8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows
Hi all, This pull request contains a backport of [JDK-8287902](https://bugs.openjdk.org/browse/JDK-8287902), commit [975316e3](https://github.com/openjdk/jdk/commit/975316e3e5f1208e4e15eadc2493d25c15554647) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Magnus Ihse Bursie on 10 Jun 2022 and was reviewed by Naoto Sato. It fixes an issue that shows up in the new GHA workflow. Thanks! - Commit messages: - Backport 975316e3e5f1208e4e15eadc2493d25c15554647 Changes: https://git.openjdk.org/jdk19/pull/134/files Webrev: https://webrevs.openjdk.org/?repo=jdk19&pr=134&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8287902 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk19/pull/134.diff Fetch: git fetch https://git.openjdk.org/jdk19 pull/134/head:pull/134 PR: https://git.openjdk.org/jdk19/pull/134
[jdk19] Integrated: 8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows
On Mon, 11 Jul 2022 15:07:28 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8287902](https://bugs.openjdk.org/browse/JDK-8287902), commit > [975316e3](https://github.com/openjdk/jdk/commit/975316e3e5f1208e4e15eadc2493d25c15554647) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Magnus Ihse Bursie on 10 Jun 2022 > and was reviewed by Naoto Sato. > > It fixes an issue that shows up in the new GHA workflow. > > Thanks! This pull request has now been integrated. Changeset: 39715f3d Author:Christoph Langer URL: https://git.openjdk.org/jdk19/commit/39715f3da7e8749bf477b818ae06f4dd99c223c4 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod 8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows Backport-of: 975316e3e5f1208e4e15eadc2493d25c15554647 - PR: https://git.openjdk.org/jdk19/pull/134
[jdk19] RFR: 8290460: Alpine: disable some panama tests that rely on std::thread
Hi all, This pull request contains a backport of [JDK-8290460](https://bugs.openjdk.org/browse/JDK-8290460), commit [d7f0de27](https://github.com/openjdk/jdk/commit/d7f0de272c85ee8d0890c9d61e10065b618b69d7) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. It is a testfix, so should be good to submit within RDP2. The issue is observed in SAP's CI. Thanks! - Commit messages: - Backport d7f0de272c85ee8d0890c9d61e10065b618b69d7 Changes: https://git.openjdk.org/jdk19/pull/151/files Webrev: https://webrevs.openjdk.org/?repo=jdk19&pr=151&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8290460 Stats: 3 lines in 2 files changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk19/pull/151.diff Fetch: git fetch https://git.openjdk.org/jdk19 pull/151/head:pull/151 PR: https://git.openjdk.org/jdk19/pull/151
[jdk19] Integrated: 8290460: Alpine: disable some panama tests that rely on std::thread
On Fri, 22 Jul 2022 07:04:29 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8290460](https://bugs.openjdk.org/browse/JDK-8290460), commit > [d7f0de27](https://github.com/openjdk/jdk/commit/d7f0de272c85ee8d0890c9d61e10065b618b69d7) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > It is a testfix, so should be good to submit within RDP2. The issue is > observed in SAP's CI. > > Thanks! This pull request has now been integrated. Changeset: c29242eb Author:Christoph Langer URL: https://git.openjdk.org/jdk19/commit/c29242ebb0cfd3eaa56240664af607c02a11324e Stats: 3 lines in 2 files changed: 3 ins; 0 del; 0 mod 8290460: Alpine: disable some panama tests that rely on std::thread Backport-of: d7f0de272c85ee8d0890c9d61e10065b618b69d7 - PR: https://git.openjdk.org/jdk19/pull/151
Re: RFR: 8290059: Do not use std::thread in panama tests
On Thu, 21 Jul 2022 18:48:14 GMT, Jorn Vernee wrote: > This patch removes the use of std::thread from the `java.lang.foreign` tests, > and switches to the OS specific thread APIs, in order to change things such > as the stack size on some platforms where this is required in the future (see > the JBS issue). > > This is done by adding a small header-only library that exposes a function to > run code in freshly spawned threads (`run_async`). > > Testing: Running the affected tests on platforms that implement the linker. @JornVernee thanks for doing this so quickly. I suggest undoing https://github.com/openjdk/jdk/commit/d7f0de272c85ee8d0890c9d61e10065b618b69d7 in this change, too. If you update this PR I can run it through our Alpine test pipeline. - PR: https://git.openjdk.org/jdk/pull/9599
Re: RFR: 8290059: Do not use std::thread in panama tests
On Fri, 22 Jul 2022 15:09:13 GMT, Christoph Langer wrote: >> This patch removes the use of std::thread from the `java.lang.foreign` >> tests, and switches to the OS specific thread APIs, in order to change >> things such as the stack size on some platforms where this is required in >> the future (see the JBS issue). >> >> This is done by adding a small header-only library that exposes a function >> to run code in freshly spawned threads (`run_async`). >> >> Testing: Running the affected tests on platforms that implement the linker. > > @JornVernee thanks for doing this so quickly. I suggest undoing > https://github.com/openjdk/jdk/commit/d7f0de272c85ee8d0890c9d61e10065b618b69d7 > in this change, too. If you update this PR I can run it through our Alpine > test pipeline. > @RealCLanger Note that I'm not setting the stack size of the thread in this > patch. I'm just using the defaults. From the discussion on the bug, I don't > think this sufficient to make the tests pass on Apline/MuslC. > > I avoided getting into that since I don't have ready access to an > Alpine/MuslC testing environment atm. So, I've left setting the stack size on > MuslC, and re-enabling the tests for someone that does. Hopefully this patch > is enough to get that going easily. OK, thanks for clarifying that. @tstuefe, maybe you want to have a look after this fix is in? - PR: https://git.openjdk.org/jdk/pull/9599
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work
On Tue, 20 Jun 2023 13:23:16 GMT, Matthias Baesken wrote:
> Currently, a number of tests fail on macOS because they miss the core file
> (e.g. serviceability/sa/TestJmapCore.java).
> The reason is that configure detects on some setups that codesign does not
> work ("checking if debug mode codesign is possible... no) .
> So adding the needed entitlement to generate cores is not done in the build.
> This is currently not checked later in the tests.
> But without the entitlement, a core is not generated.
I made a few review suggestions. Does the symptom happen on both, arm64 and x64?
test/lib/jdk/test/lib/Platform.java line 267:
> 265: // Find the path to the java binary.
> 266: String jdkPath = System.getProperty("java.home");
> 267: Path javaPath = Paths.get(jdkPath + "/bin/java");
You could do it more concisely:
Path javaPath = Paths.get(System.getProperty("java.home") + "/bin/java");
if (Files.notExists(javaPath)) {
throw new FileNotFoundException("Could not find file " +
javaPath.toAbsolutePath().toString());
test/lib/jdk/test/lib/Platform.java line 274:
> 272:
> 273: // Run codesign on the java binary.
> 274: ProcessBuilder pb = new ProcessBuilder("codesign", "--display",
> "--verbose", javaFileName);
And then here 'ProcessBuilder pb = new ProcessBuilder("codesign", "--display",
"--verbose", javaPath.toAbsolutePath().toString());`
test/lib/jdk/test/lib/Platform.java line 277:
> 275: pb.redirectErrorStream(true); // redirect stderr to stdout
> 276: Process codesignProcess = pb.start();
> 277: BufferedReader is = new BufferedReader(new
> InputStreamReader(codesignProcess.getInputStream()));
Maybe do the BufferedReader stuff in a try-with-resources and then return false
instead of potentially throwing an IOException?
test/lib/jdk/test/lib/Platform.java line 280:
> 278: String line;
> 279: while ((line = is.readLine()) != null) {
> 280: System.out.println("STDOUT: " + line);
This output is for every line seems too much. Maybe just print the lines where
you find "Info.plist=not bound" or "Info.plist entries="?
test/lib/jdk/test/lib/util/CoreUtils.java line 153:
> 151: throw new SkippedException("Cannot produce core file
> with hardened binary on OSX 10.15 and later");
> 152: }
> 153: } else {
Maybe you could do just one case here:
`else if (!Platform.hasPlistEntriesOSX()) {`...
-
PR Review: https://git.openjdk.org/jdk/pull/14562#pullrequestreview-1491050995
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237190071
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237190832
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237191976
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237194492
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237184922
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work [v2]
On Thu, 22 Jun 2023 09:53:29 GMT, Matthias Baesken wrote:
>> Currently, a number of tests fail on macOS because they miss the core file
>> (e.g. serviceability/sa/TestJmapCore.java).
>> The reason is that configure detects on some setups that codesign does not
>> work ("checking if debug mode codesign is possible... no) .
>> So adding the needed entitlement to generate cores is not done in the build.
>> This is currently not checked later in the tests.
>> But without the entitlement, a core is not generated.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Some adjustments
Looks a bit better. But I think instead of adding a Utils.javaPath() method,
you can do all this path handling in Platform.launchCodesignOnJavaBinary().
Then even more code would be shared.
test/lib/jdk/test/lib/Platform.java line 263:
> 261: }
> 262:
> 263: private static codesignProcess launchCodesignOnJavaBinary(String
> javaFileName) {
That can't work... should be `private static Process` 😉
-
Changes requested by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14562#pullrequestreview-1492927024
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1238354879
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work [v3]
On Thu, 22 Jun 2023 11:51:15 GMT, Matthias Baesken wrote:
>> Currently, a number of tests fail on macOS because they miss the core file
>> (e.g. serviceability/sa/TestJmapCore.java).
>> The reason is that configure detects on some setups that codesign does not
>> work ("checking if debug mode codesign is possible... no) .
>> So adding the needed entitlement to generate cores is not done in the build.
>> This is currently not checked later in the tests.
>> But without the entitlement, a core is not generated.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> move javaPath into caller
Looks good to me now.
-
Marked as reviewed by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14562#pullrequestreview-1493063728
Re: RFR: JDK-8310550: Adjust references to rt.jar [v3]
On Fri, 30 Jun 2023 11:37:10 GMT, Matthias Baesken wrote:
>> There are a few references to rt.jar in comments and in the codebase itself.
>> Some of them might be removed or adjusted.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> remove import
Looks good overall. I made a few suggestions.
test/hotspot/jtreg/vmTestbase/nsk/jvmti/AttachOnDemand/attach024/TestDescription.java
line 40:
> 38: * Agent's JAR file contains modified class
> java.util.TooManyListenersException (it is assumed
> 39: * that this class isn't loaded before agent is loaded), agent
> instantiates TooManyListenersException
> 40: * and checks that non-modified version of this class was loaded from
> jdk image (not from agent's JAR).
"from the jdk image"
test/jdk/com/sun/tools/attach/ProviderTest.java line 110:
> 108: public static void main(String args[]) throws Exception {
> 109: // deal with internal builds where classes are loaded from
> the
> 110: // 'classes' directory rather than the image modules file
"... rather than the runtime image"
test/langtools/tools/javap/4798312/JavapShouldLoadClassesFromRTJarTest.java
line 27:
> 25: * @test
> 26: * @bug 4798312
> 27: * @summary In Windows, javap doesn't load classes from image
"... from the runtime image"
-
Changes requested by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14593#pullrequestreview-1514576016
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253140142
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253141204
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253142105
Re: RFR: JDK-8310550: Adjust references to rt.jar [v3]
On Thu, 22 Jun 2023 09:21:29 GMT, Matthias Baesken wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java >> line 196: >> >>> 194: >>> 195: /** >>> 196: * Set whether or not to use ct.sym as an alternate >> >> As an alternate to what? This needs something else. > > should "to the image modules files" be used instead ? maybe "... to the current runtime."? - PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253139297
Re: RFR: JDK-8310550: Adjust references to rt.jar [v4]
On Wed, 5 Jul 2023 15:01:52 GMT, Matthias Baesken wrote: >> There are a few references to rt.jar in comments and in the codebase itself. >> Some of them might be removed or adjusted. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjust comments Fine from my end now. Just one minor nit left. 😄 src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java line 196: > 194: > 195: /** > 196: * Set whether or not to use ct.sym as an alternate to the current > runtime You should bring back the period at the end of the sentence. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14593#pullrequestreview-1514740197 PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253244166
Re: [jdk21] RFR: 8313260: JDK21: ProblemList java/lang/ScopedValue/StressStackOverflow.java on linux-x86
On Thu, 27 Jul 2023 18:21:45 GMT, Sergey Bylokhov wrote: > This patch adds the java/lang/ScopedValue/StressStackOverflow.java to the > problem list for linux-x86 where it intermittently fails on a GA, ex: > https://github.com/openjdk/jdk21/pull/148 > > This is only for JDK 21, test passes on JDK 22. This is good. Thanks for doing it. I guess we should backport the real fixes to jdk21u. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk21/pull/149#pullrequestreview-1551465386
Re: [jdk21] RFR: 8313260: JDK21: ProblemList java/lang/ScopedValue/StressStackOverflow.java on linux-x86
On Fri, 28 Jul 2023 09:10:20 GMT, Alan Bateman wrote: > JDK-8308609 I added a comment on https://bugs.openjdk.org/browse/JDK-8303498, cc @offamitkumar - PR Comment: https://git.openjdk.org/jdk21/pull/149#issuecomment-1655513822
[jdk21] RFR: 8311822: AIX : test/jdk/java/foreign/TestLayouts.java fails because of different output - expected [[i4](struct)] but found [[I4](struct)]
Hi all, This pull request contains a backport of [JDK-8311822](https://bugs.openjdk.org/browse/JDK-8311822), commit [d1cc2782](https://github.com/openjdk/jdk/commit/d1cc2782606e8a3cfead9055aa845e48e851edd4) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Per Minborg on 24 Jul 2023 and was reviewed by Jorn Vernee. Test fix, so qualifying for jdk21 under RDP2 rules. Thanks! - Commit messages: - Backport d1cc2782606e8a3cfead9055aa845e48e851edd4 Changes: https://git.openjdk.org/jdk21/pull/157/files Webrev: https://webrevs.openjdk.org/?repo=jdk21&pr=157&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8311822 Stats: 7 lines in 1 file changed: 3 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk21/pull/157.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/157/head:pull/157 PR: https://git.openjdk.org/jdk21/pull/157
[jdk21] Integrated: 8311822: AIX : test/jdk/java/foreign/TestLayouts.java fails because of different output - expected [[i4](struct)] but found [[I4](struct)]
On Tue, 1 Aug 2023 21:57:17 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8311822](https://bugs.openjdk.org/browse/JDK-8311822), commit > [d1cc2782](https://github.com/openjdk/jdk/commit/d1cc2782606e8a3cfead9055aa845e48e851edd4) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Per Minborg on 24 Jul 2023 and > was reviewed by Jorn Vernee. > > > Test fix, so qualifying for jdk21 under RDP2 rules. > > Thanks! This pull request has now been integrated. Changeset: 610812d4 Author:Christoph Langer URL: https://git.openjdk.org/jdk21/commit/610812d4743d9ef9f6e92f7f2eea179fbdf81387 Stats: 7 lines in 1 file changed: 3 ins; 0 del; 4 mod 8311822: AIX : test/jdk/java/foreign/TestLayouts.java fails because of different output - expected [[i4](struct)] but found [[I4](struct)] Reviewed-by: mbaesken Backport-of: d1cc2782606e8a3cfead9055aa845e48e851edd4 - PR: https://git.openjdk.org/jdk21/pull/157
RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges
On Windows, the test java/lang/ProcessHandle/InfoTest.java can fail when run as user that is member of the Administrators group. In that case new files are not owned by the user but instead by BUILTIN\ADMINISTRATORS. This breaks the assumptions of the test's whoami check. My suggestion is to cater for this case and don't fail the test but write a warning message to stdout that a whoami check is not correctly possible. - Commit messages: - JDK-8314094 Changes: https://git.openjdk.org/jdk/pull/15222/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15222&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8314094 Stats: 21 lines in 1 file changed: 14 ins; 3 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/15222.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15222/head:pull/15222 PR: https://git.openjdk.org/jdk/pull/15222
Re: RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges
On Mon, 28 Aug 2023 05:24:09 GMT, Arno Zeller wrote:
>> I think you might use System.getProperty("user.name"). But I am not sure
>> about domain names of users on Windows.
>> I am also not sure why the user name is currently determined by creating a
>> file - there might be a reason for this that is not obvious to me.
>
> It seems that ProcessHandle.info() returns **DOMAIN/USERNAME** on Windows
> but System.getProperty("user.name") only the **USERNAME**.
> You can get **DOMAIN** and **USERNAME** on **Windows** by calling:
> com.sun.security.auth.module.NTSystem NTSystem = new
> com.sun.security.auth.module.NTSystem();
> String user = NTSystem.getName();
> String domain = NTSystem.getDomain();
Yes, I think using System.getProperty("user.name") is brittle as well. If we'd
use `com.sun.security.auth.module.NTSystem`, we would introduce the dependency
to another module - `jdk.security.auth`. Not sure, whether this is a good
option.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15222#discussion_r1308412488
Re: RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges [v2]
On Thu, 31 Aug 2023 15:08:34 GMT, Roger Riggs wrote:
>> The problem with the environment variables is, that jtreg only passes very
>> few of them down to the testee process - USERDOMAIN and USERNAME are not
>> part of these as far as I know.
>
> ok, more overhead than value in the suggestion.
> If its windows, strip off the domain (before "/") and compare.
> If that doesn't work then just drop the username compare on windows.
After verifying that System.getenv yields empty results for USERDOMAIN and
USERNAME, I updated the change to use System.getProperty("user.name") in the
Windows Administrators case. Let's see how testing goes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15222#discussion_r1312738642
Re: RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges [v2]
> On Windows, the test java/lang/ProcessHandle/InfoTest.java can fail when run > as user that is member of the Administrators group. In that case new files > are not owned by the user but instead by BUILTIN\ADMINISTRATORS. This breaks > the assumptions of the test's whoami check. My suggestion is to cater for > this case and don't fail the test but write a warning message to stdout that > a whoami check is not correctly possible. Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Use System.getProperty(user.name) for the Windows Adminstrators case - Merge branch 'master' into JDK-8314094 - JDK-8314094 - Changes: - all: https://git.openjdk.org/jdk/pull/15222/files - new: https://git.openjdk.org/jdk/pull/15222/files/4e5cbf82..db6d3d14 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15222&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15222&range=00-01 Stats: 27267 lines in 894 files changed: 17633 ins; 3604 del; 6030 mod Patch: https://git.openjdk.org/jdk/pull/15222.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15222/head:pull/15222 PR: https://git.openjdk.org/jdk/pull/15222
Re: RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges [v2]
On Fri, 1 Sep 2023 08:32:20 GMT, Christoph Langer wrote: >> On Windows, the test java/lang/ProcessHandle/InfoTest.java can fail when run >> as user that is member of the Administrators group. In that case new files >> are not owned by the user but instead by BUILTIN\ADMINISTRATORS. This breaks >> the assumptions of the test's whoami check. My suggestion is to cater for >> this case and don't fail the test but write a warning message to stdout that >> a whoami check is not correctly possible. > > Christoph Langer has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Use System.getProperty(user.name) for the Windows Adminstrators case > - Merge branch 'master' into JDK-8314094 > - JDK-8314094 OK, the latest update seems to work. I'll integrate tomorrow, unless I hear concerns. - PR Comment: https://git.openjdk.org/jdk/pull/15222#issuecomment-1704820273
Integrated: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges
On Thu, 10 Aug 2023 09:54:43 GMT, Christoph Langer wrote: > On Windows, the test java/lang/ProcessHandle/InfoTest.java can fail when run > as user that is member of the Administrators group. In that case new files > are not owned by the user but instead by BUILTIN\ADMINISTRATORS. This breaks > the assumptions of the test's whoami check. My suggestion is to cater for > this case and don't fail the test but write a warning message to stdout that > a whoami check is not correctly possible. This pull request has now been integrated. Changeset: 69c9ec92 Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/69c9ec92d04a399946b2157690a1dc3fec517329 Stats: 46 lines in 1 file changed: 31 ins; 10 del; 5 mod 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges Reviewed-by: mbaesken, azeller - PR: https://git.openjdk.org/jdk/pull/15222
Re: RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges [v2]
On Tue, 5 Sep 2023 13:52:09 GMT, Roger Riggs wrote: > A bit late due to a US holiday. Looks good. Thanks 🙇 - PR Comment: https://git.openjdk.org/jdk/pull/15222#issuecomment-1706695064
RFR: 8320601: ProblemList java/lang/invoke/lambda/LambdaFileEncodingSerialization.java on linux-all
java/lang/invoke/lambda/LambdaFileEncodingSerialization.java is already problem listed on linux-x64. However, the issue is not processor specific. We see the failure on linux on other architectures as well. - Commit messages: - Update ProblemList.txt Changes: https://git.openjdk.org/jdk/pull/16784/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16784&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8320601 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16784.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16784/head:pull/16784 PR: https://git.openjdk.org/jdk/pull/16784
Integrated: 8320601: ProblemList java/lang/invoke/lambda/LambdaFileEncodingSerialization.java on linux-all
On Wed, 22 Nov 2023 15:59:38 GMT, Christoph Langer wrote: > java/lang/invoke/lambda/LambdaFileEncodingSerialization.java is already > problem listed on linux-x64. However, the issue is not processor specific. We > see the failure on linux on other architectures as well. This pull request has now been integrated. Changeset: f6e5559a Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/f6e5559ae9d1c8b84b31af5d36e93b43e7731ba5 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8320601: ProblemList java/lang/invoke/lambda/LambdaFileEncodingSerialization.java on linux-all Reviewed-by: mbaesken - PR: https://git.openjdk.org/jdk/pull/16784
Re: RFR: JDK-8317307: test/jdk/com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails with ConnectException: Connection timed out: no further information
On Fri, 1 Dec 2023 13:46:14 GMT, Matthias Baesken wrote:
> On Windows we recently run into this error rather often in the test
> LdapPoolTimeoutTest.java :
>
> MSG RTE: javax.naming.CommunicationException: example.com:1234 [Root
> exception is java.net.ConnectException: Connection timed out: no further
> information]
> MSG: Connect timed out
> MSG: Timeout exceeded while waiting for a connection: 26984ms
> MSG: Timed out waiting for lock
> MSG: Timed out waiting for lock
> MSG: Timed out waiting for lock
> MSG: Timeout exceeded while waiting for a connection: 26984ms
> MSG: Timeout exceeded while waiting for a connection: 26984ms
> java.lang.Exception: failures: 1
> at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:104)
> at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:58)
> at
> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
> at
> com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
> at java.base/java.lang.Thread.run(Thread.java:1570)
>
> We should extend the accepted exception message strings and also also
> 'Connection timed out' .
Looks good and makes sense to accept this case as well since we see it occuring
in real life.
Maybe move the `|| msg.contains("Connection timed out")` into the next line.
-
Marked as reviewed by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16922#pullrequestreview-1759855858
RFR: 8322772: Clean up code after JDK-8322417
In the review of the PR for JDK-8322417 it was noted that a fully qualified class name "java.util.Arrays" is unnecessary but it was forgotten to clean it up prior to integration. - Commit messages: - JDK-8322417 Changes: https://git.openjdk.org/jdk/pull/17203/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17203&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322772 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17203.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17203/head:pull/17203 PR: https://git.openjdk.org/jdk/pull/17203
Re: RFR: 8322772: Clean up code after JDK-8322417
On Fri, 29 Dec 2023 13:44:27 GMT, Christoph Langer wrote: > In the review of the PR for JDK-8322417 it was noted that a fully qualified > class name "java.util.Arrays" is unnecessary but it was forgotten to clean it > up prior to integration. Integrating under trivial rule. - PR Comment: https://git.openjdk.org/jdk/pull/17203#issuecomment-1872353704
Integrated: 8322772: Clean up code after JDK-8322417
On Fri, 29 Dec 2023 13:44:27 GMT, Christoph Langer wrote: > In the review of the PR for JDK-8322417 it was noted that a fully qualified > class name "java.util.Arrays" is unnecessary but it was forgotten to clean it > up prior to integration. This pull request has now been integrated. Changeset: 32d80e2c Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/32d80e2caf6063b58128bd5f3dc87b276f3bd0cb Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8322772: Clean up code after JDK-8322417 Reviewed-by: mdoerr, goetz, mbaesken, vtewari - PR: https://git.openjdk.org/jdk/pull/17203
Re: RFR: 8322565 (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits [v2]
On Tue, 9 Jan 2024 10:22:40 GMT, Eirik Bjørsnøs wrote: >> This PR suggests that `Files.setPosixPermissions`as implemented by >> `ZipFileSystem` should preserve the leading seven bits of the 'external file >> attributes' field. These bits contain the 'file type', 'setuid', 'setgid', >> and 'sticky' bits. These are unrelated to permissions and should not be >> modified by this operation. >> >> The fix is to update `Entry.readCEN` to read all 16 bits instead of just the >> trailing 12 and to update `ZipFileSystem.setPermissions` to preserve the >> leading 7 bits when updating the trailing 9 permission-related bits of the >> `Entry.posixPerms` field. >> >> The PR adds a new test `TestPosix.preserveRemainingBits()` which verifies >> that the leading 7 bits are not affected by `Files.setPosixPermissions`. >> This test also verifies that operations not related to POSIX, such as >> Files.setLastModifiedTime does not affect the 'external file attributes' >> value. >> >> Note that this PR does not aim to preserve the leading seven bits for the >> case when `Files.setPosixPermissions` is called with a `null` permission >> set. (The implementation currently interprets this as a signal that the >> 'external file attributes' should not be populated and the 'version made >> by' OS will be MSDOS instead of Unix) > > Eirik Bjørsnøs has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Verify that ZipFileSystem preserves 'external file attribute' bits when > performing operations unrelated to POSIX, such as Files.setLastModifiedTime. > - Merge branch 'master' into zipfs-preserve-external-file-attrs > - Preserve non-permission 'external file attributes' bits when setting posix > permissions Looks correct to me, too. I also went over the changes to the test and it makes sense. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17170#pullrequestreview-1812103908
Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]
On Wed, 17 Jan 2024 15:38:22 GMT, Richard Reingruber wrote: >> Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for >> correct (Dekker scheme) synchronization with concurrent execution of >> [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176). >> >> The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot >> and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance >> Suite, and SAP specific tests. >> Testing was done with fastdebug and release builds on the main platforms and >> also on Linux/PPC64le and AIX. > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Review Alan test/jdk/java/nio/channels/Selector/LotsOfInterrupts.java line 2: > 1: /* > 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. I guess you should credit SAP here. - PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1456496044
RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket
During analysing a customer case I figured out that we have an inconsistency between documentation and actual behavior in class com.sun.jndi.ldap.Connection. The [method documentation of com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) states: "If a timeout is supplied but unconnected sockets are not supported then the timeout is ignored and a connected socket is created." This, however does not happen. If a SocketFactory would not support unconnected sockets, it would likely throw a SocketException in [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). And since [the code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) does not check for this behavior, a connection with timeout value through a SocketFactory that does not support unconnected sockets would simply fail with an IOException. So we should either make the code adhere to what is documented or adapt the documentation to the actual behavior. I hereby try to fix the connect coding. Alternatively, we could also adapt the description - I have no strong opinion. What do the experts suggest? - Commit messages: - JDK-8325579 Changes: https://git.openjdk.org/jdk/pull/17797/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325579 Stats: 38 lines in 1 file changed: 17 ins; 12 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket
On Fri, 9 Feb 2024 21:29:28 GMT, Christoph Langer wrote: > During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Hi Aleksei, thanks for taking a look. Yes, I'm working on extending the test. Stay tuned. 😄 Cheers Christoph - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1944018942
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v2]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: JDK-8325579 - Changes: - all: https://git.openjdk.org/jdk/pull/17797/files - new: https://git.openjdk.org/jdk/pull/17797/files/419f96f5..48318eb2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v3]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Enhance test - JDK-8325579 - Changes: https://git.openjdk.org/jdk/pull/17797/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=02 Stats: 235 lines in 3 files changed: 128 ins; 29 del; 78 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v3]
On Thu, 15 Feb 2024 15:11:15 GMT, Christoph Langer wrote: >> During analysing a customer case I figured out that we have an inconsistency >> between documentation and actual behavior in class >> com.sun.jndi.ldap.Connection. The [method documentation of >> com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) >> states: "If a timeout is supplied but unconnected sockets are not supported >> then the timeout is ignored and a connected socket is created." >> >> This, however does not happen. If a SocketFactory would not support >> unconnected sockets, it would likely throw a SocketException in >> [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). >> And since [the >> code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) >> does not check for this behavior, a connection with timeout value through a >> SocketFactory that does not support unconnected sockets would simply fail >> with an IOException. >> >> So we should either make the code adhere to what is documented or adapt the >> documentation to the actual behavior. >> >> I hereby try to fix the connect coding. Alternatively, we could also adapt >> the description - I have no strong opinion. What do the experts suggest? > > Christoph Langer has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Enhance test > - JDK-8325579 So, I've updated the PR. Sorry for force-pushing, hope you didn't review the code in detail yet. The new version updates module-info as requested. I also enhanced the test `LdapSSLHandshakeFailureTest.java` and added variants that exercise a Socket Factory which does not support unconnected sockets. Two of the test variants would fail with the current JDK. - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1946315957
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v4]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has updated the pull request incrementally with one additional commit since the last revision: Rename test and refine comment - Changes: - all: https://git.openjdk.org/jdk/pull/17797/files - new: https://git.openjdk.org/jdk/pull/17797/files/c0dee34c..88ae0b27 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=02-03 Stats: 683 lines in 2 files changed: 344 ins; 339 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Typo - Merge branch 'master' into JDK-8325579 - Rename test and refine comment - Enhance test - JDK-8325579 - Changes: - all: https://git.openjdk.org/jdk/pull/17797/files - new: https://git.openjdk.org/jdk/pull/17797/files/88ae0b27..d8d1d6db Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=03-04 Stats: 1085 lines in 40 files changed: 842 ins; 125 del; 118 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v6]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Review feedback - Rename back to LdapSSLHandshakeFailureTest to ease reviewing - Merge branch 'master' into JDK-8325579 - Typo - Merge branch 'master' into JDK-8325579 - Rename test and refine comment - Enhance test - JDK-8325579 - Changes: - all: https://git.openjdk.org/jdk/pull/17797/files - new: https://git.openjdk.org/jdk/pull/17797/files/d8d1d6db..ec6579d2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=04-05 Stats: 5175 lines in 157 files changed: 1842 ins; 1842 del; 1491 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v6]
On Tue, 20 Feb 2024 09:45:22 GMT, Christoph Langer wrote: >> During analysing a customer case I figured out that we have an inconsistency >> between documentation and actual behavior in class >> com.sun.jndi.ldap.Connection. The [method documentation of >> com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) >> states: "If a timeout is supplied but unconnected sockets are not supported >> then the timeout is ignored and a connected socket is created." >> >> This, however does not happen. If a SocketFactory would not support >> unconnected sockets, it would likely throw a SocketException in >> [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). >> And since [the >> code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) >> does not check for this behavior, a connection with timeout value through a >> SocketFactory that does not support unconnected sockets would simply fail >> with an IOException. >> >> So we should either make the code adhere to what is documented or adapt the >> documentation to the actual behavior. >> >> I hereby try to fix the connect coding. Alternatively, we could also adapt >> the description - I have no strong opinion. What do the experts suggest? > > Christoph Langer has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Review feedback > - Rename back to LdapSSLHandshakeFailureTest to ease reviewing > - Merge branch 'master' into JDK-8325579 > - Typo > - Merge branch 'master' into JDK-8325579 > - Rename test and refine comment > - Enhance test > - JDK-8325579 I made some updates and addressed the review comments. I reverted the renaming of the test for the sake of reviewing. While I still think LdapSSLHandshakeTest is a better, more generic name for the test's purpose, we could change its name in a separate change. - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1953904252
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Mon, 19 Feb 2024 13:17:48 GMT, Goetz Lindenmaier wrote:
>> Christoph Langer has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Typo
>> - Merge branch 'master' into JDK-8325579
>> - Rename test and refine comment
>> - Enhance test
>> - JDK-8325579
>
> test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeTest.java line 224:
>
>> 222:
>> 223: public CustomSocket(String s, int timeout) throws IOException {
>> 224: super(s, timeout);
>
> The argument should be called "port" not timeout.
Correct.
> test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeTest.java line 246:
>
>> 244:
>> 245: @Override
>> 246: public Socket createSocket(String s, int timeout) throws
>> IOException {
>
> The argument should be "int port" instead of timeout.
Right.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1495579995
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1495579746
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Fri, 16 Feb 2024 16:39:33 GMT, Daniel Fuchs wrote: >> Christoph Langer has updated the pull request with a new target base due to >> a merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - Typo >> - Merge branch 'master' into JDK-8325579 >> - Rename test and refine comment >> - Enhance test >> - JDK-8325579 > > test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeTest.java line 62: > >> 60: * whether the socket is closed after closing the Context. >> 61: * >> 62: * @run main/othervm --add-opens java.naming/javax.naming=ALL-UNNAMED >> --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED > > sigh... git handles the renaming of this test file as a deleted file + a new > file which makes it hard to review the changes. > > The --add-opens directive are very strange here. I suggest removing them and > replacing them with a single `@modules` directive: > > > @modules java.naming/javax.naming:+open > java.naming/com.sun.jndi.ldap:+open Good suggestion. Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1495579467
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Mon, 19 Feb 2024 16:57:23 GMT, Aleksei Efimov wrote:
>> Christoph Langer has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Typo
>> - Merge branch 'master' into JDK-8325579
>> - Rename test and refine comment
>> - Enhance test
>> - JDK-8325579
>
> src/java.naming/share/classes/module-info.java line 51:
>
>> 49: * network times out.
>> 50: * If a custom socket factory is provided via property
>> 51: * {@code java.naming.ldap.factory.socket} and unconnected
>> sockets
>
> Suggestion:
>
> * If a custom socket factory is provided via {@code
> java.naming.ldap.factory.socket} environment
> *property and unconnected sockets
Addressed.
> src/java.naming/share/classes/module-info.java line 51:
>
>> 49: * network times out.
>> 50: * If a custom socket factory is provided via property
>> 51: * {@code java.naming.ldap.factory.socket} and unconnected
>> sockets
>
> Since we're referencing socket factory env property, maybe we can also
> document it here. Something like:
>
> {@code java.naming.ldap.factory.socket}:
> The value of this environment property specifies the fully
> qualified class name of the socket factory used by the LDAP provider.
> This class must implement the javax.net.SocketFactory abstract class.
> By default the environment property is not set.
>
Added.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1495580384
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1495580620
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Mon, 19 Feb 2024 16:46:18 GMT, Aleksei Efimov wrote: > Currently, it is hard to distinguish what part of the test responsible for > [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) testing, and which > part is for [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579). I > would prefer to add a new test for the current fix instead: that could be > done as additional test mode, OR even better to add a completely new test. Hm, I think the test was overpurposed already. Creating another test file with duplicating code does not sound too good IMHO. Maybe it is acceptable without the renaming? Another question: Do we need a CSR/Release note as there is some behavioral change involved (although it should always have been like with this change)? - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1953911292
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Wed, 21 Feb 2024 18:26:18 GMT, Aleksei Efimov wrote: >>> Currently, it is hard to distinguish what part of the test responsible for >>> [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) testing, and >>> which part is for >>> [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579). I would prefer >>> to add a new test for the current fix instead: that could be done as >>> additional test mode, OR even better to add a completely new test. >> >> Hm, I think the test was overpurposed already. Creating another test file >> with duplicating code does not sound too good IMHO. Maybe it is acceptable >> without the renaming? >> >> Another question: Do we need a CSR/Release note as there is some behavioral >> change involved (although it should always have been like with this change)? > >> Hm, I think the test was overpurposed already. > > This test was added by JDK-8314063 fix, and I do not think it was change > after that. > >> Creating another test file with duplicating code does not sound too good >> IMHO. Maybe it is acceptable without the renaming? > > I think it is acceptable. Currently, it is hard to separate the test cases > for `a)` the original > [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) fix (the opened > socket is not closed properly when connection timeout occurs) and `b)` the > current fix [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579) > (unconnected sockets are not supported by SocketFactory). It would be great > to have this distinction in the modified test. I drafted a CSR. @AlekseiEfimov, would be nice if you could review it. As for the test, I had a closer look now and I find it hard to separate testing of [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) from [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579). Furthermore, most of the entries test things that hadn't been addressed by any of these two bugs at all. So, [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) is only tested in lines 72, 73, 76 and 77 The original problem of this issue [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579) is touched in line 71 and 73. That means, most of the other test invocations test some generic behavior which was never erroneous so far. I could, however, give each line its own test id and annotate the bugs accordingly. Do you think that makes sense? - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1959271452
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v2]
On Thu, 22 Feb 2024 14:57:05 GMT, Matthias Baesken wrote: >> Currently assertEquals has in the failure case sometimes confusing output >> like : >> >> java.lang.RuntimeException: VM output should contain exactly one RTM locking >> statistics entry for method >> compiler.rtm.locking.TestRTMTotalCountIncrRate$Test:🔒 expected 0 to equal 1 >>at jdk.test.lib.Asserts.fail(Asserts.java:634) >>at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) >> >> (I don't think we really expected that for some reason 0 equals 1) >> This should be improved. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjust COPYRIGHT year info I think it is a good idea to improve this. I was irritated by that output more than once. Maybe a better message would be ... _"..." is not equal to "..."_ ? - PR Comment: https://git.openjdk.org/jdk/pull/17952#issuecomment-1959680638
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v3]
On Thu, 22 Feb 2024 16:01:19 GMT, Matthias Baesken wrote: >> Currently assertEquals has in the failure case sometimes confusing output >> like : >> >> java.lang.RuntimeException: VM output should contain exactly one RTM locking >> statistics entry for method >> compiler.rtm.locking.TestRTMTotalCountIncrRate$Test:🔒 expected 0 to equal 1 >>at jdk.test.lib.Asserts.fail(Asserts.java:634) >>at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) >> >> (I don't think we really expected that for some reason 0 equals 1) >> This should be improved. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > assertEquals adjust output Looks good from my end. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17952#pullrequestreview-1897634635
RFR: 8326591: New test JmodExcludedFiles.java fails on Windows when --with-external-symbols-in-bundles=public is used
The new test JmodExcludedFiles.java checks that no debug symbol files are contained in the jmod files. It does not take into account that with configure option --with-external-symbols-in-bundles=public we want to have stripped pdb files, also in jmods, to get native callstacks with line number. This modifies the test and checks if equivalent *stripped.pdb files exist when .pdb files are encountered inside jmods. In that case one can assume that --with-external-symbols-in-bundles=public was chosen as configure option. - Commit messages: - JDK-8326591 Changes: https://git.openjdk.org/jdk/pull/17990/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17990&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326591 Stats: 48 lines in 1 file changed: 30 ins; 5 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/17990.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17990/head:pull/17990 PR: https://git.openjdk.org/jdk/pull/17990
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v3]
On Sat, 24 Feb 2024 11:45:46 GMT, Jaikiran Pai wrote: > This is similar to what other test libraries usually report for such failures. But in the case of a non-empty `msg` you would not see the actual values any more which I think could be helpful in a lot of cases... - PR Comment: https://git.openjdk.org/jdk/pull/17952#issuecomment-1962412001
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v3]
On Thu, 22 Feb 2024 16:01:19 GMT, Matthias Baesken wrote: >> Currently assertEquals has in the failure case sometimes confusing output >> like : >> >> java.lang.RuntimeException: VM output should contain exactly one RTM locking >> statistics entry for method >> compiler.rtm.locking.TestRTMTotalCountIncrRate$Test:🔒 expected 0 to equal 1 >>at jdk.test.lib.Asserts.fail(Asserts.java:634) >>at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) >> >> (I don't think we really expected that for some reason 0 equals 1) >> This should be improved. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > assertEquals adjust output Then maybe it should be > ```diff > +fail((msg == null ? "assertEquals" : msg) + " expected: " + lhs > + " but was: " + rhs); > ``` ? - PR Comment: https://git.openjdk.org/jdk/pull/17952#issuecomment-1963086026
