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

[Erik Joelsson]

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/autoconf/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
>>
>>
>>