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. Changes to UPGRADEABLE_PLATFORM_MODULES require discussion as adding or removing a java.* modules impacts the Java SE specification. 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. ------------- Changes requested by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15102#pullrequestreview-1556678796
