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
