RFR: 8319343: Improve CDS module graph support for --add-modules option [v2]

[Alan Bateman]

On Mon, 21 Oct 2024 05:15:50 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

>> src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 467:
>> 
>>> 465:         if (CDS.isDumpingStaticArchive()
>>> 466:                 && !haveUpgradeModulePath
>>> 467:                 && (addModules.isEmpty() || addModulesFromRuntimeImage(addModules))
>> 
>> I think it would be better to check the modules in the Configuration as that is what will be archived. Can you try this, calling addJrt(cf, addModules) where this method is below (it's similar to allJrtOrModularJar that we added recently, not tested btw).
>> 
>> 
>>     /**
>>      * Returns true if all modules named in the given set are in the Configuration and
>>      * the run-time image.
>>      */
>>     private static boolean allJrt(Configuration cf, Set<String> moduleNames) {
>>         return !moduleNames.stream()
>>                 .map(mn -> cf.findModule(mn).orElseThrow())
>>                 .map(m -> m.reference().location().orElseThrow())
>>                 .anyMatch(uri -> !uri.getScheme().equalsIgnoreCase("jrt"));
>>     }
>
> Hi Alan,
> I tried your suggestion but it can't handle the `ALL-SYSTEM` case.
> I made some slight adjustments to your patch as follows:
> 
> 
>     /**
>      * Returns true if all modules named in the given set are in the Configuration and
>      * the run-time image.
>      */
>     private static boolean allJrt(Configuration cf, Set<String> moduleNames) {
>         if (moduleNames.size() == 1 && moduleNames.contains(ALL_SYSTEM)) {
>             return true;
>         }
>         return !moduleNames.stream()
>                 .filter(mn -> !mn.equals(ALL_SYSTEM))
>                 .map(mn -> cf.findModule(mn).orElseThrow())
>                 .map(m -> m.reference().location().orElseThrow())
>                 .anyMatch(uri -> !uri.getScheme().equalsIgnoreCase("jrt"));
>     }
> 
> What do you think?

Good point as ALL-SYSTEM and ALL-MODULE-PATH are allowed in the value. I assume you don't need to special case the single value case as this is dump time so not performance critical.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21553#discussion_r1808807468