RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v4]

[Serguei Spitsyn]

On Tue, 4 Jun 2024 01:34:39 GMT, Jaikiran Pai <jpai at openjdk.org> 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:<jar>` 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 three additional commits since the last revision:
> 
>  - Alex suggestion - remove -XX:-CheckIntrinsics from NativeMethodPrefixApp test too
>  - Alex's suggestion - no need to include only the bootreporter classes in boot classpath
>  - RetransformApp test did not previously use -XX:-CheckIntrinsics

I've posted just a question and a nit. Otherwise, the fix looks good.
Thank you for taking care about this refactoring!

test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java line 34:

> 32:  * @modules java.management
> 33:  *          java.instrument
> 34:  * @run shell/timeout=240 MakeJAR2.sh NativeMethodPrefixAgent NativeMethodPrefixApp 'Can-Retransform-Classes: true' 'Can-Set-Native-Method-Prefix: true'

Just a question. The original test had a timeout:`timeout=240`. My guess is that the default timeout was not enough. Do you think, it is not needed anymore or you just missed to add it?
The same question applies to the other test.

test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 26:

> 24: 
> 25: import java.io.File;
> 26: import java.nio.file.Files;

Nit: It seems the import at line 26 is not needed.

-------------

Marked as reviewed by sspitsyn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19495#pullrequestreview-2095508509
PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625470816
PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625472194