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.)
/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
