8313274: Failure building java.base-jmod target

[jaikiran]

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 #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.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8313274: Failure building java.base-jmod target (Bug - P1)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15102/head:pull/15102
$ git checkout pull/15102

Update a local copy of the PR:
$ git checkout pull/15102
$ git pull https://git.openjdk.org/jdk.git pull/15102/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15102

View PR using the GUI difftool:
$ git pr show -t 15102

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15102.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@jaikiran The following label will be automatically applied to this pull request:

• build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[jaikiran]

/label add core-libs

[openjdk]

@jaikiran
The core-libs label was successfully added.

[jaikiran]

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.charsets, 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.

[mlbridge]

Webrevs

• 00: Full (467aea91)

[AlanBateman]

Changes to UPGRADEABLE_PLATFORM_MODULES require discussion as adding or removing a java.* module impacts the Java SE specification. It also opens the door to accidental mix 'n match of modules from different JDK builds. So I don't think this is the right change for this issue.

[jaikiran]

It also opens the door to accidental mix 'n match of modules from different JDK modules. So I don't think this the right change for this issue.

Thank you Alan for that input. I'll close this PR then. I think in the PR that introduced the original change, there was a mention that the change isn't too helpful for the build but is mostly a good to have:

Fixing this won't impact the build much, but certainly won't hurt either.

So for now I suspect we could revert that change since it currently does adversely impact the build.

[dholmes-ora]

@jaikiran I would concur - back out the change that caused the problem.

[jaikiran]

@jaikiran I would concur - back out the change that caused the problem.

Hello David, I have now opened a PR to revert that change #15118