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