RFR [9] Modular Source Code

Thu Aug 14 07:07:17 UTC 2014

Hello Mike,

Thanks for the comments. See inline.

On 2014-08-13 23:29, Mike Duigou wrote:
> There's a lot to review here. This is not a complete review but hopefully contributes to our review "coverage". I am focusing on the top project in this set of comments.
>
> - --with-output-sync seems like it should be on by default if available. Downside? This could also be split out from the jigsaw changes if there is any interest in reducing the patch size.
There are downsides yes. The way this works is that make will buffer 
output from all commands and print them when done. Depending on level 
that's when the command line is done, the recipe is done, or the 
complete submake. I played around with having it default for a while. 
For a normal build this isn't too confusing once you get used to it, but 
it really made running tests annoying as the output from jtreg would all 
be buffered until it was done. Also, a hotspot developer that was to 
build hotspot from the root repo would see nothing until the whole build 
was complete. I simply believe we need some more experience with this 
before we can decide on a better default behavior. Possibly we need to 
do the hotspot makefile rewrite first so that the hotspot build can be 
split into smaller logical chunks from the top level.

I wanted the output-sync feature to be in this from the start to make 
JPRT logs easier to look at, as the feature will be default turned on 
there. Especially debug logs become very hard to read without it.
>
> - what is TESTMAKE_OUTPUTDIR for? (ugh, more outputdir dirs...)
While doing this work, I had the need for adding features to 
SetupArchive (mainly to support multiple source dirs for jars). It 
quickly turned quite nasty and to make development easier, I added 
specific test cases for this macro in a separate makefile structure. 
This is the output directory for those tests.
> - spec.gmk.in: Can we have a separate assignment for JAVA_TOOL_FLAGS_SMALL? It is nice to be able to see every AC_SUBST somewhere solo.
Certainly, that makes sense. Must have just missed it.
> - jdk-options.m4: should with_cacerts_file being empty not merit an error? what does the empty default do?
The default is to use the bundled one. I didn't like having that default 
being defined in configure. I think it better belongs in the makefile 
handling the file.
> - javadoc.gmk: retire JDK_IMPSRC, JDK_GENSRC, JDK_SHARE_CLASSES and JDK_SHARE_SRC
We should certainly clean up javadoc.gmk properly, but I thought that 
was out of scope for this patch. You do have a point in that those 
variables are now unused so no longer need to be declared.
> - javadoc.gmk: JAVADOC_CMD should perhaps use (currently non-existant) JAVA_TOOL_FLAGS_BIG or at least JAVA_FLAGS_BIG?
I agree, but that should be a separate change.
> - MakeHelpers: CleanComponent should call strip on the $1 argument to $(RM) so that it is deleting what it promises to be deleting. Or it could check to make sure $(words $1) is 1
Strip clears leading and trailing whitespace. The reason I added a strip 
to the echo is that in some cases the macro was called with a space 
after comma and in some not. The output looked weird when the echo line 
sometimes had 2 spaces before the component name instead of one. It 
should not matter to the rm command line however.
> - modules.xml "Changes to this file will require review by Committers to Project Jigsaw." Will this be true after integration into jdk9/dev repo?
>
> - modules.list seems to be redundant with modules.xml but there don't seem to be any measures to ensure that they remain in sync. Even a comment in modules.xml would help. This kind of problem has been a source of errors in the past.
They are redundant yes. modules.list is used by make to extract 
dependency information and modules.xml is used by the verification tool. 
In jigsaw development both files were dynamically created during the 
build process and here we simply committed static versions of them. 
Ideally we should only need one, but it's a temporary solution anyway. 
We would need to create a tool to extract dependency information from 
the xml to make.
> - What is TestMake.gmk and associated for?
These are my new tests for the common makefile logic. It's far from a 
complete coverage, but it has already helped me greatly.
> jdk project:
>
> - I am slightly unsettled by the number of makefiles and putting them all in to the same directory. Will they eventually be moved into their modules?
This is something I'm thinking about and would like to discuss. I think 
that ideally the makefiles for specific modules should be moved to 
module specific directories. For now I kept the existing task based 
directory structure.
> More to come but first I want to build it!
Go for it!

/Erik
> On Aug 12 2014, at 07:10 , Chris Hegarty <chris.hegarty at oracle.com> 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/
>>
>> Due to the significant impact of these changes, a JDK 9 promotion has been tentatively reserved for their integration. All comments are welcome, although given the nature of the changes then we might have to create separate issues in JIRA to address some of them later in jdk9/dev..
>>
>> -Chris.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8051619
>> [2] https://bugs.openjdk.java.net/browse/JDK-8051618
>> [3] http://hg.openjdk.java.net/jigsaw/stage