Comments inline. /Magnus
> On 6 mar 2014, at 10:50, Erik Joelsson <erik.joels...@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.joels...@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 >
