On Tue, 1 Aug 2023 10:31:13 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... > >> 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. @jaikiran I would concur - back out the change that caused the problem. ------------- PR Comment: https://git.openjdk.org/jdk/pull/15102#issuecomment-1661514395
