On Thu, 13 Feb 2025 19:20:05 GMT, Erik Joelsson <er...@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
