On Fri, 6 Jan 2023 13:47:22 GMT, Erik Joelsson <er...@openjdk.org> wrote:
>> Matias Saavedra Silva has updated the pull request with a new target base
>> due to a merge or a rebase. The incremental webrev excludes the unrelated
>> changes brought in by the merge/rebase. The pull request contains eight
>> additional commits since the last revision:
>>
>> - Fixed jib.sh
>> - Removed prints
>> - Ioi comments and jib.sh restored
>> - Merge branch 'master' into sharedArchiveTest_8287873
>> - Removing unused code
>> - Removed file added by mistake
>> - Defaults to old functionality on failure
>> - 8287873: Add test for using -XX:+AutoCreateSharedArchive with different
>> JDK versions
>
> test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java
> line 62:
>
>> 60: // "make test
>> TEST=test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java",
>> 61: // the test.boot.jdk property is normally passed by make/RunTests.gmk
>> 62: private static String BOOT_JDK;
>
> Could we please call this something else? The concept of the BOOT_JDK is a
> rather specific thing in the JDK build process. This test is using the
> build's BOOT_JDK as a default for "a JDK of some older version than the
> current". Calling that "BOOT_JDK" in this test is confusing at least to me.
>
> Perhaps something like `OLD_JDK` would be more suitable?
Actually OLD_JDK would be confusing because we also have PREV_JDK. This is the
original code before this PR:
// If you're running this test manually, specify the location of a previous
version of
// the JDK using "jtreg -vmoption:-Dtest.previous.jdk=${JDK19_HOME} ..."
private static final String PREV_JDK = System.getProperty("test.previous.jdk",
null);
// If you're unning this test using something like
// "make test
TEST=test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestAutoCreateSharedArchiveUpgrade.java",
// the test.boot.jdk property is passed by make/RunTests.gmk
private static final String BOOT_JDK = System.getProperty("test.boot.jdk",
null);
So the problem with this PR is it overloads the meaning of BOOT_JDK. I think
it's better to do something like this:
static void setupJVMs(int fetchVersion) throws Throwable {
....
if (fetchVersion> 0) {
oldJVM = fetchJDK(fetchVersion) + FS + "bin" + FS + "java";
} else if (PREV_JDK != null) {
oldJVM = PREV_JDK + FS + "bin" + FS + "java";
} else if (BOOT_JDK != null) {
oldJVM = BOOT_JDK + FS + "bin" + FS + "java";
} else {
-------------
PR: https://git.openjdk.org/jdk/pull/11852
