RFR: JDK-8034769: Move logutil in corba to make/tools
erik.joelsson at oracle.com
Thu Feb 13 13:33:38 UTC 2014
New webrev: http://cr.openjdk.java.net/~erikj/8034769/webrev.corba.02/
* Moved all the buildtools to the same structure as in the jdk repo.
* Deleted a leftover Makefile that was sitting in the logutil source
* Moved the idl deletes to a variable with proper line breaks.
On 2014-02-12 14:34, Erik Joelsson wrote:
> On 2014-02-12 13:24, Magnus Ihse Bursie wrote:
>> On 12 feb 2014, at 12:58, Alan Bateman <Alan.Bateman at oracle.com> wrote:
>>>> On 12/02/2014 10:42, Erik Joelsson wrote:
>>>> Here is another source location cleanup, this time in the corba
>>>> repo. The 6 classes in com/sun/tools/corba/se/logutil are only used
>>>> as a build tool during compilation of the corba repo. These files
>>>> should be moved to the corba/make/tools/src directory so that it's
>>>> not confused with product code.
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8034769
>>>> Webrev: http://cr.openjdk.java.net/~erikj/8034769/webrev.corba.01/
>>> This looks okay to me. Just one comment (it's really a question) is
>>> whether this is consistent with the jdk repository. In the jdk
>>> repository then the location is make/src/classes.
>> While tools can be somewhat clearer, I think it's good to mimick the
>> jdk. And src actually says that this is something needing
>> compilation; a "tools" dir could just hold shellscripts etc. So I'd
>> recommend to go for make/src/classes here as well.
>> Also, if you don't think it's too much work, Erik, can you move the
>> source code to the same package structure as the jdk build tools? I.e
>> sun.build.tools or whatever it is. While I think it is too lon,
>> (sun.buildtools, would have been enough), consistency is worth more.
> I had a feeling you would ask for this, so sure, will fixup the
> structure too.
>>> In passing I see the DELETES value in GensrcCorba.gmk is huge and I
>>> wonder why it wasn't split over multiple lines. It makes
>>> side-by-side diffs difficult.
>> Wow! Didn't see that one before! I'm not a fan of a hard limit on 80
>> chars, but that... that's just ridiculous. :-)
>> It should definitely be moved to a variable and be properly split
>> into multiple lines. And what is it for? Can we generate it instead?
> That's a leftover from very early build infra days. I think it was
> left that way intentionally to stand out. I agree that we should fix it.
More information about the build-dev