RFR: JDK-8189095; Import JMC from artifactory using Jib and main makefiles

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Thu Oct 12 14:23:21 UTC 2017


On 2017-10-12 15:40, Erik Joelsson wrote:
> Unfortunately, it didn't stay as easy as that. After hitting snag 
> after snag, I finally decided to implement some kind of general 
> support for file names with spaces in them, with support in CacheFind 
> and SetupCopyFiles, as well as the various install-file variants. This 
> got a little bit more messy than I would have liked, but at least I 
> added a test for SetupCopyFiles to verify the functionality. Also note 
> that this only works fully with gnu make 4.0 or later. With 3.81, I 
> only get spaces to work in the leaf file and not in any directory. We 
> certainly don't want to encourage anyone to use file names with spaces 
> anywhere however, so even limited support is ok IMO.
>
> While working on the tests I found some problems with the current 
> tests that I also fixed to get a clean baseline for these changes.
>
> Webrev: http://cr.openjdk.java.net/~erikj/8189095/webrev.02/

Hm.

It's not really pretty. (Apart from MacBundles.gmk, that one got really 
cleaner.). It might be the best solution to a hairy problem, though. :-&

I'm not sure I'm so fond of the S2Q and Q2S names. I realize you wanted 
something short, but it looks really cryptic. Spelling out 
"SpaceToQuestionMark" is not the answer either. How about "EncodeSpaces" 
and "DecodeSpaces", or something like that?

It's very nice that you added the test (and fixed the test suite!). 
Kudos! However, it took me some time to realize why you needed a 
TestCommon.gmk when we already had a TestMakeBase.gmk. Maybe drop the 
Test prefix from the utilities, more like CommonUtils.gmk? The fact that 
it resides in test/make/ makes it clear that it is for testing, but 
having no Test- prefix doesn't implicate that we're actually *testing* 
something.

/Magnus

> /Erik
>
>
> On 2017-10-11 18:44, Erik Joelsson wrote:
>> Please review this small fix for the mac-bundles target. The current 
>> solution does not handle files with spaces in them. By changing this 
>> to a single rule with a recursive copy, any filenames that includes 
>> spaces will be handled correctly.
>>
>> Webrev: http://cr.openjdk.java.net/~erikj/8189095/webrev.01/
>>
>> /Erik
>>
>




More information about the build-dev mailing list