On Fri, 21 Jun 2024 18:31:00 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add extra test for missing root modules
>
> src/jdk.jdeps/share/classes/com/sun/tools/jdeprscan/Main.java line 417:
>
>> 415: return false;
>> 416: }
>> 417: JavaFileManager fm =
>> pp.getPlatformTrusted(release).getFileManager();
>
> Not sure if this change is necessary. I believe `release` is verified to be a
> valid platform name at this point, so even with the new check, it should
> still work. (And `getPlatformTrusted` could possibly be eliminated.) But
> maybe I am missing something?
I wanted to avoid adding exception handling code here, since
`PlatformNotSupported` is a checked exception.
> test/langtools/tools/jnativescan/TestJNativeScan.java line 174:
>
>> 172: "-add-modules",
>> "org.singlejar,org.myapp",
>> 173: "--print-native-access"))
>> 174: .stdoutShouldContain("org.singlejar")
>
> It is a small thing, bu was there a consideration for a stronger assert on
> the output, checking that the output is precisely something like
> `--enable-native-access org.lib,org.service,org.singlejar`? Would require
> that the output is stable, which may be tricky, but also not a bad property.
> Just an idea for consideration.
Good point. The result should be 'clean' and directly forward-able to
`--enable-native-access`. (So in this case it should be exactly `org.singlejar`)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649312391
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649313272
