Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v3]
On Tue, 4 Jun 2024 01:30:55 GMT, Jaikiran Pai wrote: >> test/jdk/java/lang/instrument/RetransformApp.java line 81: >> >>> 79: final OutputAnalyzer oa = ProcessTools.executeTestJava( >>> 80: "--enable-preview", // due to usage of ClassFile API >>> PreviewFeature in the agent >>> 81: "-XX:+UnlockDiagnosticVMOptions", >>> "-XX:-CheckIntrinsics", >> >> This `-XX` flags were not present for this test (and I don't think they are >> needed for NativeMethodPrefix test too) > > You are right - it was a copy/paste error on my part to have include these > flags for the `RetransformApp`. Based on your suggestion, I have removed > them from the other test too. The tests continue to pass. It turns out we do need the `-XX:-CheckIntrinsics` for the `NativeMethodPrefix` test. Without that we have a test failure in a higher tier https://bugs.openjdk.org/browse/JDK-8333756. - PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1630482492
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v3]
On Mon, 3 Jun 2024 20:24:49 GMT, Alex Menkov wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Alex's input - simplify the test by using ClassFileInstaller
>
> test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 86:
>
>> 84: Can-Set-Native-Method-Prefix: true
>> 85: """
>> 86: + "Boot-Class-Path: " +
>> bootClassesDir.toString().replace(File.separatorChar, '/') + "/"
>
> AFAIU Boot-Class-Path is needed only because the agent needs
> bootreporter.StringIdCallbackReporter
> I don't see much point in separate bootreporter dir.
> So we can set Boot-Class-Path to System.getProperty("test.classes") and drop
> createBootClassesDir() logic
Hello Alex, I have updated the PR to add "test.classes" to Boot-Class-Path
instead of just those 2 classes. Tests in the updated PR continue to pass.
> test/jdk/java/lang/instrument/RetransformApp.java line 81:
>
>> 79: final OutputAnalyzer oa = ProcessTools.executeTestJava(
>> 80: "--enable-preview", // due to usage of ClassFile API
>> PreviewFeature in the agent
>> 81: "-XX:+UnlockDiagnosticVMOptions", "-XX:-CheckIntrinsics",
>
> This `-XX` flags were not present for this test (and I don't think they are
> needed for NativeMethodPrefix test too)
You are right - it was a copy/paste error on my part to have include these
flags for the `RetransformApp`. Based on your suggestion, I have removed them
from the other test too. The tests continue to pass.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625219101
PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625219542
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v3]
On Sat, 1 Jun 2024 07:41:29 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this test-only change which addresses
>> https://bugs.openjdk.org/browse/JDK-8333130?
>>
>> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp`
>> under `test/jdk/java/lang/instrument/` which launch the app/test with a
>> `-javaagent:` pointing to a test specific jar. The test, launched with
>> that `javaagent`, then verifies the test specific details.
>>
>> In their current form, in order to construct the agent `.jar` and make it
>> available to the jtreg test that's launched, they `@run` a jtreg `shell`
>> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar
>> file. The contents of the script is relatively straightforward - it compiles
>> (using `javac`) some boot classpath classes, some agent specific classes and
>> then generates a jar file with the agent classes and a `MANIFEST.MF` which
>> points to the boot classpath classes along with test specific manifest
>> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass
>> `--enable-preview --release 23` since one of the existing agent class was
>> updated to use the ClassFile API PreviewFeature
>> (https://github.com/openjdk/jdk/pull/13009).
>>
>> This generated agent jar then is passed as `-javaagent:` to the subsequent
>> `@run main` test which does the actual testing.
>>
>> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there
>> to create a jar file that's needed by the actual test. Within the JDK we
>> have several tests which compile other classes and generate jar files using
>> in-process test libraries and the `java.util.spi.ToolProvider`
>> implementations. These 2 tests too can be updated to use a similar
>> technique, to completely get rid of the `MakeJAR2.sh`. Removing the
>> `MakeJAR2.sh` will simplify the ability to pass around value for `--release`
>> when using `--enable-preview`.
>>
>> The commit in this PR updates these 2 tests to use `@run driver` which then
>> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and
>> generates the agent jar file and then finally launching the test process. As
>> part of the update, I did not retain the `@author` tag from the 2 test
>> definitions, since it's my understanding that we no longer use those. I will
>> add them back if we should retain them.
>>
>> The tests continue to pass locally with this change. I've also triggered
>> tier testing in our CI for this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Alex's input - simplify the test by using ClassFileInstaller
test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 86:
> 84: Can-Set-Native-Method-Prefix: true
> 85: """
> 86: + "Boot-Class-Path: " +
> bootClassesDir.toString().replace(File.separatorChar, '/') + "/"
AFAIU Boot-Class-Path is needed only because the agent needs
bootreporter.StringIdCallbackReporter
I don't see much point in separate bootreporter dir.
So we can set Boot-Class-Path to System.getProperty("test.classes") and drop
createBootClassesDir() logic
test/jdk/java/lang/instrument/RetransformApp.java line 81:
> 79: final OutputAnalyzer oa = ProcessTools.executeTestJava(
> 80: "--enable-preview", // due to usage of ClassFile API
> PreviewFeature in the agent
> 81: "-XX:+UnlockDiagnosticVMOptions", "-XX:-CheckIntrinsics",
This `-XX` flags were not present for this test (and I don't think they are
needed for NativeMethodPrefix test too)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625009748
PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1624969611
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v3]
> Can I please get a review of this test-only change which addresses > https://bugs.openjdk.org/browse/JDK-8333130? > > There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` > under `test/jdk/java/lang/instrument/` which launch the app/test with a > `-javaagent:` pointing to a test specific jar. The test, launched with > that `javaagent`, then verifies the test specific details. > > In their current form, in order to construct the agent `.jar` and make it > available to the jtreg test that's launched, they `@run` a jtreg `shell` > test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar > file. The contents of the script is relatively straightforward - it compiles > (using `javac`) some boot classpath classes, some agent specific classes and > then generates a jar file with the agent classes and a `MANIFEST.MF` which > points to the boot classpath classes along with test specific manifest > attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass > `--enable-preview --release 23` since one of the existing agent class was > updated to use the ClassFile API PreviewFeature > (https://github.com/openjdk/jdk/pull/13009). > > This generated agent jar then is passed as `-javaagent:` to the subsequent > `@run main` test which does the actual testing. > > So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there > to create a jar file that's needed by the actual test. Within the JDK we have > several tests which compile other classes and generate jar files using > in-process test libraries and the `java.util.spi.ToolProvider` > implementations. These 2 tests too can be updated to use a similar technique, > to completely get rid of the `MakeJAR2.sh`. Removing the `MakeJAR2.sh` will > simplify the ability to pass around value for `--release` when using > `--enable-preview`. > > The commit in this PR updates these 2 tests to use `@run driver` which then > compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and > generates the agent jar file and then finally launching the test process. As > part of the update, I did not retain the `@author` tag from the 2 test > definitions, since it's my understanding that we no longer use those. I will > add them back if we should retain them. > > The tests continue to pass locally with this change. I've also triggered tier > testing in our CI for this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Alex's input - simplify the test by using ClassFileInstaller - Changes: - all: https://git.openjdk.org/jdk/pull/19495/files - new: https://git.openjdk.org/jdk/pull/19495/files/88997f4f..3c7512b2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=01-02 Stats: 187 lines in 2 files changed: 13 ins; 144 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/19495.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19495/head:pull/19495 PR: https://git.openjdk.org/jdk/pull/19495
