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