8258411: Move module set configuration from Modules.gmk to conf dir #1781

magicus · 2020-12-15T16:11:45Z

The hard-coded list of modules in make/common/Modules.gmk has always been a wart in the build system. We pride ourself on using discovery instead of hard-coded list. In this case, it is not possible to do do auto-discovery, since the different module sets are configured, not determined.

Thus the actual lists of module sets should move to the make/conf directory.

This is the first patch in a series where I will move configuration values spread over the build system into the designated make/conf directory.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8258411: Move module set configuration from Modules.gmk to conf dir

Reviewers

Alan Bateman (@AlanBateman - Reviewer)
Mandy Chung (@mlchung - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1781/head:pull/1781
$ git checkout pull/1781

bridgekeeper · 2020-12-15T16:12:44Z

👋 Welcome back ihse! 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 · 2020-12-15T16:13:31Z

@magicus 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.

mlbridge · 2020-12-15T16:16:05Z

Webrevs

AlanBateman

I really dislike patch as it mixes up several things in module-sets.conf. If you really need to move configuration out of Modules.gmk (and I see no reason to do this) then please look at separating out the static mapping of modules to class loaders, the modules used for the interim builds, and the modules used to create API docs.

magicus · 2020-12-15T17:53:34Z

@AlanBateman It's not really more mixed-up than it was previously in Modules.gmk, since I lifted the code mostly unchanged from there. But sure, I can split it up further; I agree that it might make sense to do so.

AlanBateman

This looks better but I think we need to find better names for the conf files. Prefixing them with "module-sets" looks really strange.
JRE_TOOL_MODULES in module-sets-classloaders.conf might also need to be re-examined because it is not used to generate ModuleLoaderMap. Instead it was defined in Modules.gmk for the legacy-jre-image build target.

magicus · 2020-12-15T20:27:36Z

I thought it was a consistent and clear naming scheme. :-) But I guess to each their own...

Would classloader-modules.conf, docs-modules.conf and build-modules.con be better? Otherwise you'll need to come up with any better solutions yourself, since I'm starting to run out of ideas.

magicus · 2020-12-15T20:32:05Z

As for JRE_TOOL_MODULES, I understand what you mean but it is at least kind of a "sibling" to the others. After all, we use these sets of modules together to form the set of modules for the JRE:

JRE_MODULES += $(filter $(ALL_MODULES), $(BOOT_MODULES) \
    $(PLATFORM_MODULES) $(JRE_TOOL_MODULES))

So given that BOOT_MODULES and PLATFORM_MODULE has a role to play here as well, I think it would be odd not to have JRE_TOOL_MODULES defined at the same place.

mlchung · 2020-12-15T22:47:16Z

JRE_TOOL_MODULES started with more than one modules in JDK 9:

JRE_TOOL_MODULES += \
    jdk.jdwp.agent \
    jdk.pack \
    jdk.scripting.nashorn.shell \
    #

Since only jdk.jdwp.agent one module is left for JRE_TOOL_MODULES, as you are refactoring this file, I suggest to get rid of JRE_TOOL_MODULES and explicitly name jdk.jdwp.agent in JRE_MODULES.

mlchung · 2020-12-15T22:58:47Z

Do you see a way to get rid of DOCS_MODULES but determine this set at build time? IIRC it was added for expediency for JDK-8172312. This is the set of Java SE + JDK modules that excludes jdk.internal.* modules and jdk.unsupported and also platform-specific modules. (History: the docs bundle generated prior to JDK 9 only included platform-neutral APIs.)

As for the conf file for module to class loader mapping, I actually like one single file jdk-modules.conf to enumerate all JDK modules. Currently it only defines the list of modules defined to boot and platform class loader but excludes any modules defined to application class loaders. I consider to enumerate all modules in this file and the build can verify if any module is missing.

module-sets-build.conf is a bit awkward and I will give more thought on naming ideas.

mlchung · 2020-12-15T23:50:54Z

Can any of INTERIM_IMAGE_MODULES , HOTSPOT_MODULES and LANGTOOLS_MODULES be inlined in the appropriate .gmk file?

INTERIM_IMAGE_MODULES is for building interim image. If it has to be defined in a conf file, I like its name be explicit and match the target or makefile, for example, interim-images.conf or InterimImages.conf. This way I can tell what this conf file intends for. What do you think?

magicus · 2020-12-16T00:14:02Z

@mlchung The entire point of this exercise is to not have hard-coded lists of modules in make files...

Having hard-coded lists have come back to bite us, time after time again. We try to auto-discover everything that is possible. For these sets of modules, however, auto-discovery is not possible since these lists define what we mean by e.g. platform modules or an interim image.

AlanBateman · 2020-12-16T10:21:08Z

The update to JRE_MODULES in Images.gmk resolves my comment above. However, the naming for the configuration is still a bit odd, e.g. module-sets-classloaders.conf should be something like module-loader-map.conf as used to generate ModuleLoaderMap.java in the build.

magicus · 2020-12-16T13:51:50Z

@AlanBateman I don't have a problem with renaming the conf files, I just did not know what you wanted them to be called. :-) I renamed module-sets-classloaders.conf to module-loader-map.conf. Based on this, I rename the other two files javadoc-modules.conf and build-module-sets.conf, respectively. I hope this is okay. Otherwise, please just let me know what you think they should be called.

AlanBateman · 2020-12-16T14:40:14Z

Thanks for the update.

javadoc-modules.conf is probably okay although someone finding this in the repo might initially think it's the configuration for the javadoc modules. That plus it sets DOCS_MODULES, so maybe it should be apidocs-modules.conf.

module-loader-map.conf works as the configuration file that defines BOOT_MODULES and PLATFORM_MODULES. I think AGGREGATOR_MODULES should be dropped and "java.se" added to PLATFORM_MODULES. If I remember correctly, this was separated out in JDK 9 and 10 because of the java.se.ee aggregator module (that one was removed in Java 11 by JEP 320).

We should probably look at UPGRADEABLE_MODULES while we are here. This is the modules that are overriddable by way of excluding from the hashes stored in java.base (CreateJmods.gmk). I think it's okay to leave it in module-loader-map.conf because these modules are mapped to the platform class loader. Could we just rename it to UPGRADEABLE_PLATFORM_MODULES so that its a bit clearer (in Modules.gmk) as to why they are append to PLATFORM_MODULES?

mlchung · 2020-12-16T17:06:41Z

javadoc-modules.conf is probably okay although someone finding this in the repo might initially think it's the configuration for the javadoc modules. That plus it sets DOCS_MODULES, so maybe it should be apidocs-modules.conf.

I'm okay with apidocs-modules.conf or docs-modules.conf. It's good to match the make target name which makes it easier to understand what this configuration is for.

module-loader-map.conf works as the configuration file that defines BOOT_MODULES and PLATFORM_MODULES. I think AGGREGATOR_MODULES should be dropped and "java.se" added to PLATFORM_MODULES. If I remember correctly, this was separated out in JDK 9 and 10 because of the java.se.ee aggregator module (that one was removed in Java 11 by JEP 320).

module-loader-map.conf works for me too. We can drop AGGREGATOR_MODULES since java.se is the sole aggregator module in JDK now.

openjdk · 2020-12-16T18:57:23Z

@magicus This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8258411: Move module set configuration from Modules.gmk to conf dir

Reviewed-by: alanb, mchung

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 18 new commits pushed to the master branch:

ab5d581: 8258447: Move make/hotspot/hotspot.script to make/scripts
70183f4: 8251438: Issues with our POSIX set_signal_handler()
6eca296: 8258420: Move URL configuration from Docs.gmk to conf dir
3c66485: 8257906: JFR: RecordingStream leaks memory
0c8cc2c: 8258058: improve description of OutOfMemoryError relevant flags
cdb5342: 8258252: Move PtrQueue enqueue to PtrQueueSet subclasses
17ace83: 8258074: Move some flags related to compiler to compiler_globals.hpp
47ba652: 8258455: problem list tools/jdeprscan/tests/jdk/jdeprscan/TestRelease.java
1e77896: 8236225: Remove expired flags in JDK 17
4d6f318: 8257726: Make -XX:+StressLdcRewrite option a diagnostic option
... and 8 more: https://git.openjdk.java.net/jdk/compare/a372be4ba2216adb325f7616e54c1d58ac8fdf2a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

AlanBateman

Thanks for the updates.

magicus · 2020-12-16T21:57:12Z

/integrate

openjdk · 2020-12-16T21:57:54Z

@magicus Since your change was applied there have been 23 commits pushed to the master branch:

aa9c136: 8251989: Hex formatting and parsing utility
efd61c6: 8258514: Replace Collections.unmodifiableList with List.of
59f4c4a: 8258075: Use auto variable declarations for enum iteration
330ce86: 8257700: Add logging for sealed classes in JVM_GetPermittedSubclasses
c463264: 8258048: Placeholder hash code is the same as Dictionary hash code
ab5d581: 8258447: Move make/hotspot/hotspot.script to make/scripts
70183f4: 8251438: Issues with our POSIX set_signal_handler()
6eca296: 8258420: Move URL configuration from Docs.gmk to conf dir
3c66485: 8257906: JFR: RecordingStream leaks memory
0c8cc2c: 8258058: improve description of OutOfMemoryError relevant flags
... and 13 more: https://git.openjdk.java.net/jdk/compare/a372be4ba2216adb325f7616e54c1d58ac8fdf2a...master

Your commit was automatically rebased without conflicts.

Pushed as commit a244b82.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8258411: Move module set configuration from Modules.gmk to conf dir

9d90330

openjdk bot added the rfr Pull request is ready for review label Dec 15, 2020

openjdk bot added the build build-dev@openjdk.org label Dec 15, 2020

AlanBateman suggested changes Dec 15, 2020

View reviewed changes

Split up module-sets.conf

df445d6

AlanBateman reviewed Dec 15, 2020

View reviewed changes

Remove JRE_TOOL_MODULES and inline jdk.jdwp.agent in JRE definition

ef9d53c

Rename configuration files

199832a

Changes as requested by reviewers

b7d393f

mlchung approved these changes Dec 16, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 16, 2020

AlanBateman approved these changes Dec 16, 2020

View reviewed changes

openjdk bot closed this Dec 16, 2020

openjdk bot added the integrated Pull request has been integrated label Dec 16, 2020

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 16, 2020

magicus deleted the module-sets-to-conf branch January 11, 2021 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8258411: Move module set configuration from Modules.gmk to conf dir #1781

8258411: Move module set configuration from Modules.gmk to conf dir #1781

magicus commented Dec 15, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 15, 2020

openjdk bot commented Dec 15, 2020

mlbridge bot commented Dec 15, 2020 •

edited

Loading

AlanBateman left a comment

magicus commented Dec 15, 2020

AlanBateman left a comment

magicus commented Dec 15, 2020

magicus commented Dec 15, 2020

mlchung commented Dec 15, 2020

mlchung commented Dec 15, 2020 •

edited

Loading

mlchung commented Dec 15, 2020 •

edited

Loading

magicus commented Dec 16, 2020

AlanBateman commented Dec 16, 2020

magicus commented Dec 16, 2020

AlanBateman commented Dec 16, 2020

mlchung commented Dec 16, 2020

openjdk bot commented Dec 16, 2020 •

edited

Loading

AlanBateman left a comment

magicus commented Dec 16, 2020

openjdk bot commented Dec 16, 2020

8258411: Move module set configuration from Modules.gmk to conf dir #1781

8258411: Move module set configuration from Modules.gmk to conf dir #1781

Conversation

magicus commented Dec 15, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 15, 2020

openjdk bot commented Dec 15, 2020

mlbridge bot commented Dec 15, 2020 • edited Loading

Webrevs

AlanBateman left a comment

Choose a reason for hiding this comment

magicus commented Dec 15, 2020

AlanBateman left a comment

Choose a reason for hiding this comment

magicus commented Dec 15, 2020

magicus commented Dec 15, 2020

mlchung commented Dec 15, 2020

mlchung commented Dec 15, 2020 • edited Loading

mlchung commented Dec 15, 2020 • edited Loading

magicus commented Dec 16, 2020

AlanBateman commented Dec 16, 2020

magicus commented Dec 16, 2020

AlanBateman commented Dec 16, 2020

mlchung commented Dec 16, 2020

openjdk bot commented Dec 16, 2020 • edited Loading

AlanBateman left a comment

Choose a reason for hiding this comment

magicus commented Dec 16, 2020

openjdk bot commented Dec 16, 2020

magicus commented Dec 15, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 15, 2020 •

edited

Loading

mlchung commented Dec 15, 2020 •

edited

Loading

mlchung commented Dec 15, 2020 •

edited

Loading

openjdk bot commented Dec 16, 2020 •

edited

Loading