RFR: 8349953: Avoid editing AOTConfiguration file in "make test JTREG_AOT_JDK=true" [v5]

[Erik Joelsson]

On Tue, 18 Feb 2025 19:27:58 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> When running HotSpot jtreg tests in the "AOT mode", for example:
>> 
>> 
>> make test JTREG_AOT_JDK=true open/test/hotspot/jtreg/runtime/stringtable
>> 
>> 
>> Before this PR, in the test set up phase, we record several AOT configuration files by running a few separate Java tools (javac, javap, jlink, and jar), and then combine them together with sed, grep, sort and uniq:
>> 
>> https://github.com/openjdk/jdk/blob/adc3f53d2403cd414a91e71c079b4108b2346da0/make/RunTests.gmk#L723-L744
>> 
>> After [JDK-8348426](https://bugs.openjdk.org/browse/JDK-8348426), the AOT configuration file will change to a binary format and can no longer be edited this way. In preparation for [JDK-8348426](https://bugs.openjdk.org/browse/JDK-8348426), we should change the "JTREG_AOT_JDK=true" set up to run a single Java program that accomplishes the same effect as the current implementation.
>> 
>> ** Changes in this PR **
>> 
>> This PR combines the invocation of these Java tools into a single Java program, so we just have a single AOT configuration file. It also uses the `-XX:ExtraSharedClassListFile` option to include the default classlist from the JDK home directory,
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Move the contents of SetupAOT.gmk back to RunTests.gmk, as it is not necessary to have the definitions in a stand-alone file

Thank you for helping cleaning this up and moving the compilation of the tool to build time and the test image. Added some more comments.

make/Main.gmk line 757:

> 755: 
> 756: $(eval $(call SetupTarget, build-test-setup-aot, \
> 757:     MAKEFILE := test/BuildJtregSetupAOT, \

Maybe name the file to match the target (i.e. BuildTestSetupAot)? I don't think there is anything actually jtreg specific about this AOT tool we are building, is there?

make/Main.gmk line 758:

> 756: $(eval $(call SetupTarget, build-test-setup-aot, \
> 757:     MAKEFILE := test/BuildJtregSetupAOT, \
> 758:     DEPS := interim-langtools exploded-image build-test-lib, \

I don't think `build-test-lib` is needed.

make/PreInitSupport.gmk line 46:

> 44: # All known make control variables; these are handled in other makefiles
> 45: MAKE_CONTROL_VARIABLES += JDK_FILTER SPEC_FILTER \
> 46:     TEST TEST_JOBS JTREG JTREG_AOT_JDK GTEST MICRO TEST_OPTS TEST_VM_OPTS TEST_DEPS

This isn't how this filter is meant to be used. `JTREG_AOT_JDK` is the internal representation of setting `JTREG=AOT_JDK=...` on the make command line. While it is technically possible to directly set the internal variable, the intended way of setting this is through the `JTREG` control variable. This gives us the ability to validate the input to reduce the chance of typos messing up a test run. So `JTREG_AOT_JDK` should not be added here as we want to be warned when someone uses the internal form.

make/test/BuildJtregSetupAOT.gmk line 29:

> 27: 
> 28: include $(SPEC)
> 29: include MakeBase.gmk

This should be replaced with just `include MakeFileStart.gmk` since [JDK-8349515](https://bugs.openjdk.org/browse/JDK-8349515), and the file should end with `include MakeFileEnd.gmk`. This will declare `all` as the default target and declare `all: $(TARGETS)` at the bottom. You will need to retain the `images` target and the `.PHONY` declaration for `images`.

make/test/BuildJtregSetupAOT.gmk line 41:

> 39: SETUP_AOT_SUPPORT := $(SUPPORT_OUTPUTDIR)/test/jtreg_setup_aot
> 40: SETUP_AOT_JAR     := $(SETUP_AOT_SUPPORT)/jtregSetupAOT.jar
> 41: SETUP_AOT_CLASS   := $(SETUP_AOT_SUPPORT)/classes/ExerciseJDKClasses.class

We do not encourage aligning assignments like this in the makefiles.
https://openjdk.org/groups/build/doc/code-conventions.html

make/test/BuildJtregSetupAOT.gmk line 47:

> 45:     SRC := $(SETUP_AOT_BASEDIR), \
> 46:     BIN := $(SETUP_AOT_SUPPORT)/classes, \
> 47:     JAR := $(SETUP_AOT_JAR), \

If we aren't using the jar, no need to build it.

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

PR Review: https://git.openjdk.org/jdk/pull/23620#pullrequestreview-2624978833
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1960561460
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1960572020
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1960558691
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1960566324
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1960567801
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1960569143