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

[Ioi Lam]

On Thu, 13 Feb 2025 19:20:05 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Comments from @erikj79
>
> make/RunTests.gmk line 734:
> 
>> 732: 	$$(call MakeDir, $$($1_TEST_SUPPORT_DIR)/aot)
>> 733: 
>> 734: 	$(info AOT: Create cache configuration) \
> 
> Please use logging framework for any messages. To get something to print by default `$(call LogWarn, ...). While here, please also clean up the left over debug $$(info ) left over further down in this file that we weren't able to point out in the original review.

Fixed. I also fixed the other `$(info)` used by AOT testing.

> make/RunTests.gmk line 737:
> 
>> 735: 	$$(call ExecuteWithLog, $$($1_TEST_SUPPORT_DIR)/aot, ( \
>> 736: 	    $(CD) $$($1_AOT_JDK_DIR); \
>> 737: 	    $(JDK_UNDER_TEST)/bin/javac -d . $(TOPDIR)/make/test/SetupAot.java; \
> 
> This is putting the compilation output in the current directory, which is the make directory in the workspace. That's not acceptable. Since this is a single class java program, would it be possible to just run it without compilation or would that interfere with the intended behavior? Otherwise it needs to be moved to somewhere else, under `$$($1_TEST_SUPPORT_DIR)`.
> 
> I don't like that the compilation is run in the same recipe as the java command running the tool. At the very least those should be split into separate recipes, with separate ExecuteWithLog calls. A cleaner and preferred solution would be to build this tool as part of the test image so we avoid having to compile code during test setup.

I added the class file to the test image.

> make/RunTests.gmk line 741:
> 
>> 739: 	                -Xlog:cds,cds+class=debug:file=$$($1_AOT_JDK_CONF).log \
>> 740: 	                -XX:AOTMode=record -XX:AOTConfiguration=$$($1_AOT_JDK_CONF) \
>> 741: 	                SetupAot \
> 
> Please use 4 spaces indent to follow the [Code Conventions for the Build System](https://openjdk.org/groups/build/doc/code-conventions.html).
> 
> Suggestion:
> 
> 	    $$(FIXPATH) $(JDK_UNDER_TEST)/bin/java $$($1_VM_OPTIONS) \
> 	        -Xlog:cds,cds+class=debug:file=$$($1_AOT_JDK_CONF).log \
> 	        -XX:AOTMode=record -XX:AOTConfiguration=$$($1_AOT_JDK_CONF) \
> 	        SetupAot \

Fixed.

> make/RunTests.gmk line 744:
> 
>> 742: 	))
>> 743: 
>> 744: 	$$(info AOT: Generate AOT cache $$($1_AOT_JDK_CACHE) with flags: $$($1_VM_OPTIONS))
> 
> Again, please use the logging framework. In this case I would recommend `$(call LogInfo, ...)` to reduce spam on the default level. I think one line is enough on warn level for indicating that AOT preparation is taking place.

I changed the `$(info)` to `$(call LogWarn)`. Since AOT testing is still a new feature and all the test tasks we use "make test JTREG_AOT_JDK=true" for are for the purpose of testing AOT, it's better to print out the messages related to the AOT cache by default to help with diagnosing test failures.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1955574061
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1955573695
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1955573715
PR Review Comment: https://git.openjdk.org/jdk/pull/23620#discussion_r1955573910