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

[Simon Tooke]

On 9/11/2019 6:07 PM, 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.

The actual history also includes the current jdk14 variant

> line 811: if test -z "$(ls -d $with_ucrt_dll_dir/*.dll 2> /dev/null)";
then

(i.e. no quotes at all; so failing at least the case where there is a space)

If you look later in the file, there are lines that already incorporate
use the proposed fix

> 830: if test -z "$(ls -d "$UCRT_DLL_DIR/"*.dll 2> /dev/null)"; then

...

> 835:  || test -z "$(ls -d "$UCRT_DLL_DIR/"*.dll 2> /dev/null)"; then

And there are some cases in other codepaths that may not handle spaces
in paths either, but I am not addressing them here.

>
> 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?

I have actually tested this fix in a cygwin build where there is a space
in the path.  The directory was

   
"C:\tmp\build-jdk\ojdkbuild\tools\toolchain\sdk10_1607\Redist\ucrt\DLLs\ex
x64"

and the configuration argument:

   
--with-ucrt-dll-dir="%OJDKBUILD_DIR:/=\%\tools\toolchain\sdk10_1607\Redist\ucrt\DLLs\ex
x64"


>
>
> Thanks,
> David
> -----

Thanks for taking a look,

-simon

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