RFR: JDK-8036611: Cleanup of handling of properties and other java resources in the build

Thu Mar 6 14:41:45 UTC 2014

Comments inline.

/Magnus

> On 6 mar 2014, at 10:50, Erik Joelsson <erik.joelsson at oracle.com> wrote:
> 
> Thanks for the comments.
> 
> Here is a new webrev: http://cr.openjdk.java.net/~erikj/8036611/webrev.02/

Looks good to me now!

> 
>> On 2014-03-05 12:16, Magnus Ihse Bursie wrote:
>> Hi Erik,
>> 
>> Good work!
>> 
>> Some comment:
>> * In JavaCompilation.gmk:
>> add_files_to_copy_and_clean should be named only "..to_clean", otherwise it sounds as it is handling the copy part too. This was bad even before your fix, but then nobody really used that code. Could you please fix that?
> Done
>> I have a hard time figuring out that sed expression, including your additions. Do you mind trying to document its intentions?
> Done

Thanks for both. 

>> * In CompileCorba.gmk;
>> The removed JAR line looks weird. Is it an webrev artifact or was it dangling like that in the original makefile? How in the world come make didn't complain about it?
> It was unintentionally left there in my last change and this was a cleanup. It probably worked because there was no active left parentheses so the extra two right ones where just considered part of the value of the arbitrary variable "JAR".

Weird. But you're probably right. Darn that 70's syntax. :-)

>> The copy of zh_XX is done in GensrcCorba, but the original file is created in CompileCorba. Does that really work? On a clean compile? Looks incorrect to me.
> The original file is in the source tree and I generate the zh_HK version by copying it to gensrc. Then in CompileCorba all the properties files exist as source files for the clean. I could have opted to copy the cleaned file in CompileCorba instead but I thought this was easier. I realize it's not consistent with how it's done in the jdk repo though.

Aha, I see now that you copy the properties file. I misread it as the java file. Ok.

>> 
>> * In CopyIntoClasses:
>> Is there a reason the swingbeaninfo gif files are not stored in the corresponding source directory? (I have a vague feeling this one been up before for discussion...)
> The rest of swingbeans is generated source so I guess it makes sense to some that the resources also be in the make/data dir. I would prefer to have them in the corresponding source dir, but would rather not take that fight now and potentially hold up this patch.

Fair enough. There's still plenty left to cleanup for everyone. :) 

>> I'm not clear about the handling of zh_XX files here. First, you have an install-file %-pattern rule. Then you have an COPY_EXTRA addition. Are both needed? Why? Is it to create dependencies on the copied files?
>> 
>> Is COPY_EXTRA really ALSO_DEPEND_ON_THESE_TARGETS?
> If you look in CompileJavaClasses.gmk, from which CopyIntoClasses.gmk is included, COPY_EXTRA is simply added to the dependency list at the end, so yes, that's what it is. A pattern rule in itself will not be very useful without some targets being depended on.

Right. I got it confused with the COPY and CLEAN variables being passed into the macro call.

>> 
>> Have you confirmed that the CLEAN_FILES list does not intersect with the properties files listed for compilation?
> Yes, I handled this one line at a time from the old GensrcProperties, and I've run a lot of compares on the resulting jars during this effort.
>> * In GensrcProperties:
>> 
>> There was a comment on us not handling removed properties correctly in incremental builds. That's still applicable, right?
> Where was this comment? We don't handle removed files well in most cases since that's not how make normally works. Automatically removing files in the output requires keeping books on which files generated what and to have a separate remove step in the makefile. Fredrik wrote this for jar file generation so it probably works when removing java files today. If removing a C file, it will not be linked, but the object file will still be around.
>> 
>> The zh_XX code (again): there are two pattern rules with the same target. Do we alternatively copy generated and pre-generated files from src/share/classes? What if they clash? (I'm no good at how make resolved pattern rules.) If the zh_TW file is checked in and not generated, shouldn't we check in a copy as the zh_HK file as well?
> There are some pre-generated java properties bundles in the source tree yes. I chose not to change anything regarding them. As for clashing, the first rule that matches will be used. In this case I very much doubt we will ever see a clash.
>> 
>> In convert_tw_to_hk, what kind of sed expression is that? Never seen one of those; what does it do?
> I'm pretty sure it was just picked up from the old build. My understanding is that it searches for a line matching the regexp "class" and if found does a single search replace for "_zh_TW" to "_zh_HK" on that line.

Aha, it's a combined /class/ and s/zh_TW/zh_HK/. I see. Thought it was some fancy four-way operator. 

> 
> /Erik
>> /Magnus
>> 
>>> 4 mar 2014 kl. 16:49 skrev Erik Joelsson <erik.joelsson at oracle.com>:
>>> 
>>> Hello,
>>> 
>>> Here is a rather big patch that mainly cleans up build logic for properties files and other java resources. For a complete list of what was done, please see the bug.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8036611
>>> 
>>> Webrev: http://cr.openjdk.java.net/~erikj/8036611/webrev.01/
>>> 
>>> /Erik
> 