On Tue, 18 Feb 2025 19:27:58 GMT, Ioi Lam <ik...@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
