On Tue, 1 Aug 2023 09:01:49 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review of this change which proposes to address the build > failure noted in https://bugs.openjdk.org/browse/JDK-8313274? > > The build failure is consistently reproducible with `--with-jobs=1`. Martin, > in that JBS issue, has narrowed down the commit to the change in > https://github.com/openjdk/jdk/pull/14561, starting which this failure is > reproducible. The change in that PR, from what I understand, was meant to not > require upgradable modules be a prerequisite for the `java.base-jmod` make > target: > >> ... upgradeable modules, those shouldn't be on the prerequisites list for >> java.base-jmod. > > The implementation of that change uses the `FindAllUpgradeableModules` > function which as commented in the make files does: > >> #Upgradeable modules are those that are either defined as upgradeable or that >> #require an upradeable module. > > The implementation of `FindAllUpgradeableModules` uses the > `UPGRADEABLE_PLATFORM_MODULES` make variable which similarly comments: > >> #Modules that directly or indirectly requiring upgradeable modules >> #should carefully be considered if it should be upgradeable or not. > > However, that set currently doesn't include the "indirectly requiring > upgradable modules" and thus appears to be missing some of the modules that > are considered upgradable. > > As a result, what seems to be happening is that the `java.base-jmod` make > target now can (and does) end up requiring a particular target as a > prerequisite, say `jdk.jdeps` (which uses `java.compiler` upgradable module), > but doesn't add the `java.compiler-jmod` target as a prerequisite and thus > ends up with that build failure: > > > Creating java.base.jmod > Error: Resolution failed: Module java.compiler not found, required by > jdk.jdeps > > > The commit in this PR proposes to fix this by updating the static set of > `UPGRADEABLE_PLATFORM_MODULES` to include the indirect modules that require > the upgradable modules. How these additional modules were derived is > explained as a separate comment in this PR. > > The build succeeds with this change, both with `--with-jobs=1` and without > the `--with-jobs` option (in which case on my setup it uses 8 jobs). > > I have triggered tier testing in the CI to make sure this doesn't cause any > unexpected regressions. > > This change will require reviews from both the build team as well as the Java > modules team - my knowledge of these areas is limited and I'm unsure if there > are any additional considerations to take into account. The additional set of modules included in the `UPGRADEABLE_PLATFORM_MODULES` was determined programatically using the following code, which starts with the "well known" upgradable modules (defined in JEP 261 https://openjdk.org/jeps/261) and then finds the dependents of these upgradable modules: import java.lang.module.ModuleDescriptor; import java.util.HashSet; import java.util.Set; import java.lang.module.ModuleFinder; import java.lang.module.ModuleReference; import java.util.Collections; import java.util.TreeSet; import java.util.stream.Collectors; public class UpgradableModules { public static void main(final String[] args) throws Exception { // upgradable as specified by the JEP https://openjdk.org/jeps/261 final Set<String> specedUpgradable = new TreeSet<>(Set.of( // "java.activation", // no longer present in mainline "java.compiler", // "java.corba", // no longer present in mainline // "java.transaction", // no longer present in mainline // "java.xml.bind", // no longer present in mainline // "java.xml.ws" // no longer present in mainline //"java.xml.ws.annotation", // no longer present in mainline "jdk.internal.vm.compiler" // "jdk.xml.bind" // no longer present in mainline // "jdk.xml.ws" // no longer present in mainline )); // all other modules that "require" these JEP specified upgradable modules final Set<String> dependentUpgradable = new TreeSet<>(); final ModuleFinder sysModuleFinder = ModuleFinder.ofSystem(); final Set<ModuleReference> allModules = sysModuleFinder.findAll(); System.out.println("Using following modules as universe for finding upgradable modules: " + allModules.stream() .map((mr) -> mr.descriptor().name()) .collect(Collectors.toList())); for (final String upgradableModName : specedUpgradable) { final ModuleReference dependency = sysModuleFinder.find(upgradableModName) .orElseThrow(() -> new AssertionError("Missing module " + upgradableModName)); dependentUpgradable.addAll(findDependents(upgradableModName, allModules)); } System.out.println("JEP upgradable modules: " + specedUpgradable); System.out.println("Dependent upgradable modules: " + dependentUpgradable); } private static Set<String> findDependents(final String dependencyModName, final Set<ModuleReference> allModules) { System.out.println("finding dependents of " + dependencyModName); final Set<String> dependents = new TreeSet<>(); for (final ModuleReference sysModRef : allModules) { for (final ModuleDescriptor.Requires req : sysModRef.descriptor().requires()) { final String reqModName = req.name(); if (dependencyModName.equals(reqModName)) { // found a dependent final String dependent = sysModRef.descriptor().name(); System.out.println(dependencyModName + " is required by " + dependent); dependents.add(dependent); // if requirement is transitive find dependencies of this dependent if (req.modifiers().contains(ModuleDescriptor.Requires.Modifier.TRANSITIVE)) { final Set<String> transitive = findDependents(dependent, allModules); System.out.println(dependent + " is required by " + transitive); dependents.addAll(transitive); } } } } return dependents; } } When this code is run against the current mainline, it generates: Using following modules as universe for finding upgradable modules: [java.sql, java.sql.rowset, jdk.internal.le, jdk.jsobject, jdk.sctp, jdk.incubator.vector, jdk.compiler, java.xml, jdk.random, jdk.internal.vm.ci, jdk.crypto.cryptoki, java.scripting, jdk.security.auth, jdk.jdi, jdk.jpackage, jdk.httpserver, jdk.nio.mapmode, jdk.security.jgss, jdk.jshell, java.security.sasl, jdk.internal.opt, jdk.jfr, jdk.naming.dns, java.naming, jdk.javadoc, jdk.net, java.rmi, java.compiler, jdk.accessibility, java.security.jgss, java.prefs, jdk.editpad, java.smartcardio, jdk.internal.jvmstat, jdk.jconsole, jdk.jdeps, jdk.dynalink, jdk.crypto.ec, jdk.jlink, jdk.management.jfr, jdk.jcmd, jdk.internal.vm.compiler, jdk.jdwp.agent, jdk.unsupported.desktop, jdk.zipfs, jdk.management, java.base, jdk.hotspot.agent, java.desktop, java.management.rmi, jdk.attach, jdk.localedata, jdk.naming.rmi, java.datatransfer, jdk.jstatd, java.management, jdk.jartool, java.xml.crypto, jdk.internal.ed, java.logging, jdk.ch arsets, java.instrument, java.net.http, java.se, jdk.unsupported, jdk.internal.vm.compiler.management, java.transaction.xa, jdk.xml.dom, jdk.management.agent] finding dependents of java.compiler java.compiler is required by jdk.compiler finding dependents of jdk.compiler jdk.compiler is required by jdk.jshell jdk.compiler is required by jdk.javadoc finding dependents of jdk.javadoc jdk.javadoc is required by [] jdk.compiler is required by jdk.jdeps jdk.compiler is required by [jdk.javadoc, jdk.jdeps, jdk.jshell] java.compiler is required by jdk.jshell finding dependents of jdk.jshell jdk.jshell is required by [] java.compiler is required by jdk.javadoc finding dependents of jdk.javadoc jdk.javadoc is required by [] java.compiler is required by jdk.jdeps java.compiler is required by java.se finding dependents of java.se java.se is required by [] finding dependents of jdk.internal.vm.compiler JEP upgradable modules: [java.compiler, jdk.internal.vm.compiler] Dependent upgradable modules: [java.se, jdk.compiler, jdk.javadoc, jdk.jdeps, jdk.jshell] So `java.se, jdk.compiler, jdk.javadoc, jdk.jdeps, jdk.jshell` are the additional modules that this commit adds to the `UPGRADEABLE_PLATFORM_MODULES` set. ------------- PR Comment: https://git.openjdk.org/jdk/pull/15102#issuecomment-1659898091
