RFR [9] Modular Source Code

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 [1].
>
> 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 [1] and JEP 200 "The Modular JDK" [2], or by browsing the staging forest [3].
>
> Webrevs:
>   http://cr.openjdk.java.net/~chegar/8054834/00/
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 
jdk/make/data/bundles.

* 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 
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.)

* 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.
$(JDK_OUTPUTDIR)/modules/jdk.jdi/META-INF/services/com.sun.jdi.connect.Connector 
\
$(JDK_OUTPUTDIR)/classes/META-INF/services/com.sun.jdi.connect.Connector \

* 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 
$(JDK_OUTPUTDIR)/modules/jdk.dev/com/sun/tools/jdeps/resources/modules.xml, 
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 
$(JDK_OUTPUTDIR)/btclasses/build/tools/module/modules.xml
** Then the GenerateModulesXml tool is executed.
** The tool will open and read this file using 
GenerateModulesXml.class.getResourceAsStream("modules.xml").
** The tool will generate output to a new file, specified to be 
$(JDK_OUTPUTDIR)/modules/jdk.dev/com/sun/tools/jdeps/resources/modules.xml.
** Afterwards, the separate tool $(JDK_OUTPUTDIR)/bin/jdeps is executed, 
which will pick up the 
$(JDK_OUTPUTDIR)/modules/jdk.dev/com/sun/tools/jdeps/resources/modules.xml, 
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 
system.

*** 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 :=).

* 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.

/Magnus