Opened 3 years ago

Closed 2 years ago

#10441 closed bug (fixed)

msys native python testsuite support doesn't work in some situations

Reported by: ezyang Owned by:
Priority: high Milestone: 7.10.2
Component: Test Suite Version: 7.10.1
Keywords: Cc:
Operating System: Windows Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D952
Wiki Page:

Description

On a vanilla Windows 8.1, msys2 GHC installation with only msys python installed, the testsuite driver is broken. I know of one other person whose testsuite is also not working. The usual error message is:

cd ./typecheck/should_compile &&  "C:/msys64/home/ezy.exe" -c tc055.hs -fforce-recomp -dcore-lint -dcmm-lib -rtsopts -fno-warn-tabs -fno-ghci-history -fno-warnrr 2>&1
'..\timeout\install-inplace\bin\timeout.exe" "300" "cexternal command,
operable program or batch file.
Compile failed (status 256) errors were:

I have diagnosed the problem to the fact that we are passing a complex command string to cmd with adequately quoting it. In particular, when I inspect the command line that Python is attempting to invoke, it looks something like this:

cmd \c 'timeout.exe 300 "cd . && "inplace/ghc-stage2" -c blah blah"'

Notice, in particular, that the quoted GHC executable was not inside the timeout invocation is not double-quoted, so cmd does NOT parse the command the way you want it to.

Curiously enough, when I remove this fix (introduced by commit 101c62e26286353dd3fac1ef54323529b64c9902), the msys Python runner works fine. So I don't even know what this commit was intended to fix; it seems to have done more harm than good

Here are some details about my configuration:

ezyang@cutlass MSYS ~/ghc-validate/testsuite
$ python2 --version
Python 2.7.9

ezyang@cutlass MSYS ~/ghc-validate/testsuite
$ bash --version
GNU bash, version 4.3.33(3)-release (x86_64-pc-msys)
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>


Change History (8)

comment:1 Changed 3 years ago by ezyang

Oh yeah, my msys2 build is apparently 20150202.

comment:2 Changed 3 years ago by ezyang

Oh, here is a more accurate report from pacman -Q

msys2-keyring r8.3864337-1
msys2-runtime 2.1.0.16351.cd3184b-1
msys2-runtime-devel 2.1.0.16351.cd3184b-1
msys2-w32api-headers 5.0.0.4456.c8b6742-1
msys2-w32api-runtime 5.0.0.4455.32db221-1

comment:3 Changed 2 years ago by thomie

I partially understand what's going on.

In commit 5258566ee5c89aa757b0cf1433169346319c018f I simplified the string formatting and program path quoting in the testsuite driver. One change is that the path of the GHC executable is now double quoted instead of single quoted.

(This is done before it reaches the Python part of the testsuite driver, in mk/test.mk. I just noticed an unrelated bug here with empty program paths, which I'll fix shortly).

This change is apparently not compatible with the escaping that passThroughCmd does. Before (good):

>> passThroughCmd(["timeout", "300", "cd . && 'inplace/ghc-stage2' -c foo"])[2]
timeout "300" "cd . && 'inplace/ghc-stage2' -c foo"

After (bad):

>> passThroughCmd(['timeout', '300', 'cd . && "inplace/ghc-stage2" -c foo'])[2]
timeout "300" "cd . && "inplace/ghc-stage2" -c foo"

If everything works after removing the function passThroughCmd introduced in 101c62e26286353dd3fac1ef54323529b64c9902, I suggest doing that.

# When running under native msys Python, any invocations of non-msys binaries, # including timeout.exe, will have their arguments munged according to some # heuristics

Maybe the heuristics changed in a new version of msys, or the change from single quoting to double quoting somehow effects it?.

Otherwise changing '"%s"' to "'%s'" might work.

comment:4 Changed 2 years ago by ezyang

If old msys is doing some terrible quoting, but new msys does not, can we just error if someone tries to use an old msys? I looked at the changelog for msys but didn't see anything that indicated that the heuristic changed. I see no documentation anywhere on the net about the heuristic. This makes me very grumpy.

comment:5 Changed 2 years ago by thomie

Differential Rev(s): Phab:D952
Status: newpatch

comment:6 Changed 2 years ago by Thomas Miedema <thomasmiedema@…>

In bb9967121f2383b857680b47b6bc20607f8fd1ff/ghc:

Revert "The test runner now also works under the msys-native Python."

To make the test runner work under msys-native Python...

Commit 5258566ee5c89aa757b0cf1433169346319c018f broke the msys testsuite
driver (#10441). It changed the quoting of `config.compiler` from single
quotes to double quote, which turns out to not be compatible with what
the function `passThroughCmd` expected.

We could fix `passThroughCmd` to handle the case where `config.compiler`
is double quoted, and scatter some notes around to make sure the quoting
done in various places of the testsuite driver stay compatible.

Instead, this commit reverts 101c62e26286353dd3fac1ef54323529b64c9902,
which introdced the function `passThroughCmd` in the first place
(#9626). ezyang reports that doing this revert fixes the testsuite
driver for him using the the following version of msys2:

  msys2-keyring r8.3864337-1
  msys2-runtime 2.1.0.16351.cd3184b-1
  msys2-runtime-devel 2.1.0.16351.cd3184b-1
  msys2-w32api-headers 5.0.0.4456.c8b6742-1
  msys2-w32api-runtime 5.0.0.4455.32db221-1

Ideally we'd know what minimum version of msys2 we require, but for now
this fix is better than nothing.

Only gintas ever reported the original problem, and he actually
mentioned shortly afterwards: "This may have been fixed by a recent
release of msys2, but I am not sure."

Differential Revision: https://phabricator.haskell.org/D952

comment:7 Changed 2 years ago by thomie

Milestone: 7.12.17.10.2
Status: patchmerge
Version: 7.117.10.1

Please merge to ghc-7.10, as that branch also contains the patch that changed the quoting (dde3a2378e3961f7eed82d07d2f1e904878cc2b0, #10164).

comment:8 Changed 2 years ago by thoughtpolice

Resolution: fixed
Status: mergeclosed

Merged to ghc-7.10.

Note: See TracTickets for help on using tickets.