On Tue, 18 Feb 2025 20:40:12 GMT, Erik Joelsson <er...@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
