RFR [9] Modular Source Code

Fri Aug 15 10:13:45 UTC 2014

Hello, Magnus

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

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 
> jdk/make/data/bundles.
>
> * 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" 
https://bugs.openjdk.java.net/browse/JDK-8055188
> * 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" 
https://bugs.openjdk.java.net/browse/JDK-8055189
> * 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.
> $(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.
>
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 
> $(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.
>
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" 
https://bugs.openjdk.java.net/browse/JDK-8055191
> * 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" 
https://bugs.openjdk.java.net/browse/JDK-8055192
> * 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 
restructure" https://bugs.openjdk.java.net/browse/JDK-8055193
> *** 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" 
https://bugs.openjdk.java.net/browse/JDK-8055194
> * The BUILD_LIBT2K is used only by the Oracle closed build, and the 
> definition should move to the closed sources.
>
Added to general cleanup

/Erik