RFR  Modular Source Code
erik.joelsson at oracle.com
Fri Aug 15 10:13:45 UTC 2014
Thanks for the thorough review. I tend to agree with Alan, that we
should rather push this in unless we find critical issues but make sure
to continue cleaning this up in jdk9/dev. I have created bugs for
(almost) everything you have listed below. See inline for links and
On 2014-08-15 10:52, Magnus Ihse Bursie wrote:
> Here follows a partial review of the changes in the jdk repo. Even
> though it's long, I'm not done. ;-)
> *** General issues ***
> * The new directory jdk/make/bundle should instead reside in
> * In GensrcSwing.gmk is a "$(if $(SHUFFLED)..." part that seems to be
> remnants of older, temporary work.
Created " General cleanup of minor issues from source restructure"
> * The new file GensrcProviders.gmk are included by the Gensrc files
> for jdk.attach and jdk.jdi, which only uses half of it each. Each half
> is just a few lines long. I believe a better solution is to remove the
> GensrcProviders.gmk file, move the process-provider macro to
> GensrcCommon.gmk and move the two actual rules to the respective
> module gensrc file.
> * In the gensrc directory, there are now almost twice as many files.
> For many of them, the following pattern holds:
> GensrcOldStyle.gmk -- defines the actual logic for some gensrc target
> Gensrc-<module>.gmk -- does nothing but includes GensrcOldStyle.gmk
> In these cases, I think the contents of GensrcOldStyle should be
> inlined directly in the Gensrc-<module>.gmk instead. This holds for
> all modules except the two mammuth modules java.base and java.desktop.
> (Depending on the treatment of GensrcProviders as described above.)
Created "Cleanup gensrc after source code restructure"
> * In CreateJars.gmk, the following new comment is found embedded:
> # This currently won't work with modular build layout, but
> there currently are no
> # types needing to be re added.
> In my opinion, leaving code which looks like it's working but with a
> note saying "out of order", is bad practice. I'd recommend that the
> code is removed or commented out, if it is not needed.
While I agree on this in principle, I would rather not spend time on the
profiles generation code since it's planned to go away soon anyway.
> * In CreateJars.gmk, in BUILD_TOOLS_JAR: The following looks like a
> duplication; I believe the "classes" version should be removed.
> * In CreateSecurityJars.gmk, there is a variable
> SECURITY_CLASSES_SUBDIR that is always set to 'modules'. I believe
> this is remains of an older temporary design, and that "modules"
> should be hard-coded into the paths.
> * The old Setup.gmk had a very generic-sounding name, even though it
> only did setup java compilers. So, the rename to SetupJava.gmk was
> good; however, I'd suggest we follow it through all the way and rename
> it further to SetupJavaCompilers.gmk, since that is an even more
> accurate description of it's job.
> * The file CopyIntoClasses.gmk is not used anymore and should be removed.
> * In CoreLibraries.gmk, there used to be a line "BUILD_LIBRARIES +=
> $(BUILD_LIBFDLIBM)" which is now removed. After discussion with Erik,
> I learned that this was since the libfdlibm was not delivered in
> itself, but was used solely as a helper lib for libjava, which has
> BUILD_LIBFDLIBM as a requirement. While this change is thus correct, I
> believe a comment describing this fact would be in place on libfdlibm,
> since it makes it behave differently than all other libraries.
Added to general cleanup
> *** Issues with source files moving and its repercussions ***
> * When the source code has moved, especially the native libraries,
> most of the specific INCLUDE and EXCLUDE statements are, or should be,
> unnecessary. Nevertheless, there are still several occasions of such
> statements. In some cases, it seems like dead code that can (and
> should) be removed. But in some cases, I believe it is an indication
> that the source code has still not yet been moved to a suitable
> location. I believe the end goal with this shuffling regarding native
> library source code is that there should be a one-to-one
> correspondance between compiled native library and source code
> directory. This is now indeed the case for 99% of the libraries, but
> there are still some exceptions.
> This is a slightly vague point at the moment. I indent to check the
> INCLUDE and EXCLUDE statements more fully and will post a second
> review with results of what I find. Nevertheless, I think it is
> important to make sure we do get things correct this time.
Created "Cleanup include and exclude of native libraries after source
code restructure" https://bugs.openjdk.java.net/browse/JDK-8055190
> *** Issues regarding modules.xml ***
> The new modules.xml and associated Java tools and make files seems
> rather confusing to me. I understand some of the mysteries here are
> due the the fact that the file has moved around during development.
> Nevertheless, such historical relics should be removed when the code
> is ready to be pushed to mainline. More concretely:
> * ModulesXml.gmk referes to make/data/checkdeps/modules.xml. This file
> does not exist. I believe this should be the $(TOPDIR)/modules.xml.
> * GenererateModules.Xml says: "This tool is used to generate
> com/sun/tools/jdeps/resources/modules.xml". This is an incorrect
> claim. In fact, the output file of the tool is specified by the user.
> The way it is used by the build tool currently, the output file is
> placed in
> but that is decided by the make file and not the Java utility.
> * In fact, what happens is this:
> ** $(TOPDIR)/modules.xml is copied to
> ** Then the GenerateModulesXml tool is executed.
> ** The tool will open and read this file using
> ** The tool will generate output to a new file, specified to be
> ** Afterwards, the separate tool $(JDK_OUTPUTDIR)/bin/jdeps is
> executed, which will pick up the
> presumably using getResourceAsStream. (I have not verified this.)
> I have several objections to this.
> * First, we are actually dealing with two different files, but both
> are named modules.xml. I believe one of them is an annotated version
> of the other, but I have not chec,ed. Nevertheless, this is just an
> unneccessary source of confusion. One of them should change name, e.g.
> annotated-modules.xml or jdeps-modules.xml or whatever.
> * Second, it is not documented anywhere that GenerateModulexXml
> requires an modules.xml as input.
> * Third, and this links into the bullet above, this dependency is not
> explicit but hidden away with unnessary shuffling and
> hard-to-understand shuffling of files as result. A better solution, I
> believe, is two modify GenerateModulesXml to require a path to the
> input modules.xml as an argument, in addition to the output file. That
> way, the dependency will be obvious, and we can just point to
> $(TOPDIR)/modules.xml instead of copying it around.
> * And fourth, all old comments etc refering to old placements of the
> files should be corrected.
> * Finally, I'm also not entirely happy with the placement of
> modules.xml in the root directory. Erik has told me that the intention
> is that this file will ultimately be created dynamically at build
> time, and when that happens, it will just be a build by-product which
> can be placed elsewhere. If this is indeed the case, I can live with
> some temporary extra cluttering of the top-level directory.
While I agree it's messy at the moment, this is an area that is
temporary and will change drastically. Added as optional to general cleanup.
> *** More major undertakings ***
> * The file GensrcProperties.gmk is not split according to modules. I
> understand why this is harder to do and why it was not done for this
> milestone; nevertheless I believe it should be done in a followup patch.
Created "Split GensrcProperties.gmk into separate modules"
> * GensrcCLDR.gmk is not ideal. It generates source for multiple
> modules, and the output is separated afterwards. Fixing this will
> probably means some modification to the java helper tools.
While I think it would be good to split this in principle, I'm not sure
it's worth pursuing. The english locale is in the base module and the
others are in a different module. I would leave this as it is for now.
> * This code that previously was in jdk/make/CopyIntoClasses has now
> unfortunately moved this very specific logic up into the top repo. In
> fact, the top/make/CompileJavaModules.gmk now contain module-specific
> data such as "java.base_COPY := .icu .dat .spp
> content-types.properties". This should really be split into
> module-specific files and pushed down once again to the jdk/make
> directory, maybe into the copy directory.
Created "Move java and copy specific information in
CompileJavaModules.gmk to module specific makefiles"
> * I think the jdk/make/data directory could (and should) be separated
> on module level, e.g. jdk/make/data/java.base/... etc. Or, maybe, that
> the contents should even be moved out into like
> src/java.base/share/data, since the contents of that directory in a
> way is "source code" (only just not Java or native source code), and
> not really part of the build system.
Created "Move jdk/make/data to module specific src dirs after source
> *** Issues that was not introduced now, but should be fixed at some
> point ***
> * In CopyCommon, the variables LIBDIR and INCLUDEDIR has very
> generic-sounding name even though they are very specific. These names
> are not new, but since the definition is now in a different file than
> the uses of it, the badly chosen names makes it even harder to
> understand the code.
> * The platform-specific correspondance, OPENJDK_TARGET_OS_INCLUDE =
> $(INCLUDEDIR)/$(OPENJDK_TARGET_OS) is also bad from several
> perspectives: It is a variant of INCLUDEDIR but it does not end in
> DIR, it should not start with OPENJDK since it's not a global
> variable, and finally it uses incorrect assignment (= instead of :=).
Added to general cleanup.
> * In Awt2dLibraries.gmk, for libsplashscreen, the logic for the three
> included external graphics libraries (gif, jpeg and png) is highly
> asymmetric, which make it hard to understand. It seems likely that at
> least the libjpeg handling is broken.
Created "Cleanup source and makefile logic for libsplashscreen and
libjavajpeg after source restructure"
> * The BUILD_LIBT2K is used only by the Oracle closed build, and the
> definition should move to the closed sources.
Added to general cleanup
More information about the build-dev