RFR  Modular Source Code
Magnus Ihse Bursie
magnus.ihse.bursie at oracle.com
Fri Aug 15 08:52:36 UTC 2014
On 2014-08-12 16:10, Chris Hegarty wrote:
> This is a review request for the Initial changes for JEP 201: Modular Source Code .
> There are a number of individuals responsible for these changes. Some, possibly not all, are explicitly listed in the To section of this mail, and they will help address any comments arising from this review request.
> For the purposes of review, the actual source file moves have been omitted from the webrev below, with the exception of any source file that has a change to it’s actual content. The new location of the source files can be determined from JEP 201  and JEP 200 "The Modular JDK" , or by browsing the staging forest .
These are indeed the single most significant changes to the build system
since the "new" build system was introduced.
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.
* 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
* 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.)
* 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.
* 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.
*** 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.
*** 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.
*** 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.
* 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.
* 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.
* 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
*** Issues that was not introduced now, but should be fixed at some
* 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 :=).
* 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.
* The BUILD_LIBT2K is used only by the Oracle closed build, and the
definition should move to the closed sources.
More information about the build-dev