JDK 14 RFR: 8216354: Syntax error in toolchain_windows.m4

[Simon Tooke]

On 9/12/2019 4:00 PM, Langer, Christoph wrote:
> Hi,
>
> I've also played with this already and support Simon's patch.
>
> Simon, shall I sponsor it for you?

Yes please, (and thank you). 

-Simon

>
> Best regards
> Christoph
>
>> -----Original Message-----
>> From: build-dev <build-dev-bounces at openjdk.java.net> On Behalf Of Erik
>> Joelsson
>> Sent: Donnerstag, 12. September 2019 17:10
>> To: David Holmes <david.holmes at oracle.com>; Simon Tooke
>> <stooke at redhat.com>; build-dev <build-dev at openjdk.java.net>
>> Subject: Re: JDK 14 RFR: 8216354: Syntax error in toolchain_windows.m4
>>
>> Hello David,
>>
>> The initial report was about this construct not working at all, as Bash
>> will not expand wildcards inside of quotes:
>>
>> "$with_ucrt_dll_dir/*.dll"
>>
>> In 13 I changed that to remove the quotes to make the statement work at
>> all. The drawback then, as Simon pointed out, was that if
>> $with_ucrt_dll_dir contains a space, then it won't work. When declaring
>> JDK-8216354 as already fixed, I did not consider the case with space.
>>
>> In my opinion, the proposed construct is the preferred one. The set of
>> dlls are known and do not contain spaces. It's however common practice
>> on Windows to install these dlls in directories that contain spaces. I
>> would support backporting this fix as far back as anyone wants to,
>> though the patch will not apply cleanly since different update lines
>> currently have different variants of the statement.
>>
>> Note that internally at Oracle, this code path is not used in any
>> official builds, so we are unaffected by this change, which is also why
>> we have not detected it.
>>
>> /Erik
>>
>> On 2019-09-11 15:07, David Holmes wrote:
>>> Hi Simon, Erik,
>>>
>>> I'm a bit confused. Initially 8216354 was reported as already being
>>> fixed by 8215445. But the fix proposed here actually reverts a change
>>> from 8215445 which means that far from fixing this issue, 8215445
>>> actually introduced it - correct?
>>>
>>> But 8215445 was only fixed in 13 - no backports - so it can't be
>>> responsible for any problem in 8u or 11u. So some other bug must have
>>> originally fixed this before 13, but was not itself backported to 8u
>>> or 11u.
>>>
>>> However the original version of the code was introduced by 8202557 in
>>> JDK 11 and was backported to 8u202/8u211. So the code there should be
>>> okay - no?
>>>
>>> The original code (still in 8u, I didn't check 11u) was:
>>>
>>> "$with_ucrt_dll_dir/*.dll"
>>>
>>> and the current proposed fix is:
>>>
>>> "$with_ucrt_dll_dir/"*.dll
>>>
>>> which suggests the new fix is less robust if the dll name were to
>>> contain a space rather than the path (but that's probably not good
>>> practice anyway). Or is there some further subtlety I'm missing here?
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>
>>> On 12/09/2019 3:55 am, Simon Tooke wrote:
>>>> This is a trivial fix that has been sitting around since JDK8.
>>>>
>>>> At one time, 8216354 was marked "won't fix", but I've seen people
>>>> (including myself) run up against this issue enough that I think it
>>>> should be addressed.  It's been reopened, and I have a webrev ready that
>>>> I've tested in the jdk repo, on Windows.  It fixes the issue as
>>>> described in the bug description.  I have tested the patch using a
>>>> Cygwin build but not a WSL build.
>>>>
>>>> This patch alters a change introduced in
>>>>
>> https://hg.openjdk.java.net/jdk/jdk/annotate/50677f43ac3d/make/autocon
>> f/toolchain_windows.m4#l746
>>>> That change introduced an issue that prevented the use of directories
>>>> with spaces.
>>>>
>>>> If this patch is accepted, I intend to backport it to 11u and 8u.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8216354
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~stooke/webrevs/jdk-8216354-
>> jdk14/00/
>>>> Thank you,
>>>>
>>>> -Simon
>>>>
>>>>
>>>>