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

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Thu Oct 12 15:02:59 UTC 2017

On 2017-10-12 16:52, Erik Joelsson wrote:
> Hello,
> http://cr.openjdk.java.net/~erikj/8189095/webrev.03/
> Renamed TestCommon.gmk to UtilsForTests.gmk.
> Renamed the macros to EncodeSpace and DecodeSpace.
> Fixed a compile problem on Solaris. ($(@D) and $(dir $@) don't behave 
> exactly alike. I believe the former does not leave a trailing space, 
> so mixing them does not allow for string comparisons.)

Looks good to me now. Except it's still horrible. :-)


> /Erik
> On 2017-10-12 16:23, Magnus Ihse Bursie wrote:
>> 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