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

[Ioi Lam]

On Tue, 18 Feb 2025 20:40:12 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> 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
>
> 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?

I renamed the file to `BuildTestSetupAOT.gmk`. I also changed the `SetupAot` function to `SetupAOT` (the convention we have been following for the rest of the code has been to use either `AOT` or `aot`, but not `Aot`.).

> 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.

Removed.

> 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.

Fixed.

> 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`.

I updated the file using the same format as the existing BuildJtregTestThreadFactory.gmk file. Could you check if I missed anything?

> 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

Fixed. I also fixed other places where we had such alignments.

> 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.

Removed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1960776675
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1960776539
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1960776731
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1960776643
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1960776608
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1960776564