[windows] Avoid %r for quoting module-cache-path in Windows. #34176

drodriguez · 2020-10-04T02:17:43Z

%r returns a representation of the object that is valid Python syntax.
For numbers and strings this representation is very compatible with
Unix shells, but for Windows, the quoting performed by Python will not
be compatible. For example the Windows path separator \ will be
escaped as \\, which is not necessary for Windows and might make
some tests that try to match path fail.

Python 3.3 has shlex.quote, which was previously available as
pipes.quote in earlier Python versions. pipes.quote was indeed
used in several points of the lit.cfg file. For Windows, one need to
do their own quoting.

This change introduces shell_quote which uses shlex.quote or
pipes.quote in Unix depending on the Python version, and provides an
implementation of shell_quote for Windows. It replaces every usage
of pipes.quotes for shell_quote, and modifies the value of
mcp_opt to use shell_quote and not %r.

This should fix the test Driver\working-directory.swift in the
Windows Visual Studio 2017 builder.

drodriguez · 2020-10-04T02:17:56Z

@swift-ci please smoke test

drodriguez · 2020-10-04T02:19:43Z

I might replace every usage of %r at some point, but for now, let's introduce only one change at the time.

compnerd

The windows path makes me sad :-(. Thank you for cleaning this up. Removing the %r usage sounds like a good idea to me.

compnerd · 2020-10-04T04:03:39Z

Seems that re.replace doesn't work with the windows builders?

drodriguez · 2020-10-04T05:57:47Z

Sorry. I run it in several of the test directories, but not all of them. It should have been s.replace(…) not re.replace(…).

@swift-ci please smoke test

drodriguez · 2020-10-04T05:58:58Z

@swift-ci please test Windows platform

drodriguez · 2020-10-04T17:26:48Z

The problem now is that because the \ are not longer escaped. There are usages of Lit's SubstituteCaptures (https://github.com/apple/llvm-project/blob/1a6659f0e7add3f1c1f5352fbf5b9811bac1adfc/llvm/utils/lit/lit/TestingConfig.py#L167) that use string interpolation with the non-escaped values. Because those strings end up as replacement strings of a regular expression, they have to be escaped for the replacement string (easy to do with .replace('\\', '\\\\')).

But then the working-directory.swift test starts to fail for some reason (FML). It seems that the matching is performed against some output of the Swift driver, which outputs the jobs command lines escaped, but probably escaped for Unix. I have to circle back here. The fix for one particular test might be easier than this, even if this should be good in the long run.

I don't know, however, if the card castle of escaping and unescaping can ever be solved.

PD: I still don't understand why it works in Windows VS2019.

drodriguez · 2020-10-04T19:16:17Z

So I think I understand why it works in the VS 2019 nodes: they have Python 3.7.5 installed (comes from Visual Studio from reading the logs), while the VS 2017 nodes have 3.8.5 (installed with Chocolatey).

For os.path.realpath the documentation says "Changed in version 3.8: Symbolic links and junctions are now resolved on Windows.". When LLVM Lit starts it does a realpath on the lit configuration path which gives T:\swift\test-windows-x86_64 in Python 3.7.5, but C:\jenkins\workspace\oss-swift-windows-x86_64\build\swift\test-windows-x86_64 in Python 3.8.5.

And then enter PathSanitizingFileCheck into the game: it provides both SOURCE_DIR and BUILD_DIR, but BUILD_DIR value comes from CMake, which doesn't do any realpath shenanigans, so when Python 3.8.5 uses paths like C:\jenkins\workspace\oss-swift-windows-x86_64\build\swift\test-windows-x86_64 as the build directory, PathSanitizingFileCheck still uses T:\swift as prefix, and do not matches both.

To put the cherry on top, if one uses realpath for the substitutions of BUILD_DIR, PathSanitizingFileCheck stops working because those paths where always using forward slashes, and realpath changes everything to forward slashes.

Since this problem will start appearing when the VS 2019 nodes update to Python 3.8.5, I don't think the solution is going back to 3.7.5 in the VS 2017 nodes, but the solution is not going to be pretty.

drodriguez · 2020-10-04T20:30:51Z

@swift-ci please test Windows platform

Edit: https://ci-external.swift.org/view/Pull%20Request/job/swift-PR-windows/7663/ but not in GitHub UI yet.

drodriguez · 2020-10-04T20:31:10Z

@swift-ci please smoke test

drodriguez · 2020-10-04T21:34:53Z

@swift-ci please test Windows platform

Edit: https://ci-external.swift.org/view/Pull%20Request/job/swift-PR-windows/7664/

drodriguez · 2020-10-04T21:35:49Z

@swift-ci please smoke test

compnerd · 2020-10-04T21:39:37Z

os.normpath should convert the path to the canonical spelling for the platform - that is / will be converted to \ which might be what needs to be applied here?

drodriguez · 2020-10-05T00:50:27Z

Do not merge yet. The Windows CI failed badly. I have to look and figure out what's happening in that machine (it is quite hard to get things right because I have to copy code back and forth between machines).

os.normpath will work a little better, but I don't think changing LLVM Lit's is the way forward. Besides that, I think realpath has its benefits in this case.

compnerd · 2020-10-07T17:34:51Z

@swift-ci please test Windows platform

%r returns a representation of the object that is valid Python syntax. For numbers and strings this representation is very compatible with Unix shells, but for Windows, the quoting performed by Python will not be compatible. For example the Windows path separator `\` will be escaped as `\\`, which is not necessary for Windows and might make some tests that try to match path fail. Python 3.3 has `shlex.quote`, which was previously available as `pipes.quote` in earlier Python versions. `pipes.quote` was indeed used in several points of the `lit.cfg` file. For Windows, one need to do their own quoting. This change introduces `shell_quote` which uses `shlex.quote` or `pipes.quote` in Unix depending on the Python version, and provides an implementation of `shell_quote` for Windows. It replaces every usage of `pipes.quotes` for `shell_quote`, and modifies the value of `mcp_opt` to use `shell_quote` and not `%r`. This should fix the test `Driver\working-directory.swift` in the Windows Visual Studio 2017 builder.

drodriguez · 2020-10-08T19:14:19Z

Removed the part that adds carets. Seems that Lit already does some massaging of the arguments, and the quoting I introduced was misbehaving with the one Lit implements. I tested having a space in my Python path, like it seems to happen in the build machine.

@swift-ci please test Windows platform

Edit: https://ci-external.swift.org/view/Pull%20Request/job/swift-PR-windows/7781/
Edit 2: And it is 🔵!

drodriguez · 2020-10-08T20:18:36Z

@swift-ci please smoke test

drodriguez · 2020-10-09T06:11:15Z

@swift-ci please smoke test Linux platform

drodriguez · 2020-10-09T15:57:28Z

@swift-ci please test Linux platform

swift-ci · 2020-10-09T17:51:42Z

Build failed
Swift Test Linux Platform
Git Sha - 8470a66

drodriguez · 2020-10-10T04:21:51Z

@swift-ci please test Linux platform

drodriguez requested a review from compnerd October 4, 2020 02:17

compnerd approved these changes Oct 4, 2020

View reviewed changes

drodriguez force-pushed the windows-percent-r-is-bad branch from 7526b2d to 26c436c Compare October 4, 2020 05:56

drodriguez force-pushed the windows-percent-r-is-bad branch from 26c436c to bb9660d Compare October 4, 2020 20:30

drodriguez force-pushed the windows-percent-r-is-bad branch from bb9660d to 0326269 Compare October 4, 2020 21:34

drodriguez force-pushed the windows-percent-r-is-bad branch from 0326269 to 8470a66 Compare October 8, 2020 19:12

drodriguez merged commit 169948f into swiftlang:main Oct 10, 2020

drodriguez deleted the windows-percent-r-is-bad branch October 10, 2020 15:49

[windows] Avoid %r for quoting module-cache-path in Windows. #34176

[windows] Avoid %r for quoting module-cache-path in Windows. #34176

Uh oh!

Conversation

drodriguez commented Oct 4, 2020

Uh oh!

drodriguez commented Oct 4, 2020

Uh oh!

drodriguez commented Oct 4, 2020

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

compnerd commented Oct 4, 2020

Uh oh!

drodriguez commented Oct 4, 2020

Uh oh!

drodriguez commented Oct 4, 2020

Uh oh!

drodriguez commented Oct 4, 2020

Uh oh!

drodriguez commented Oct 4, 2020

Uh oh!

drodriguez commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drodriguez commented Oct 4, 2020

Uh oh!

drodriguez commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drodriguez commented Oct 4, 2020

Uh oh!

compnerd commented Oct 4, 2020

Uh oh!

drodriguez commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compnerd commented Oct 7, 2020

Uh oh!

drodriguez commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drodriguez commented Oct 8, 2020

Uh oh!

drodriguez commented Oct 9, 2020

Uh oh!

drodriguez commented Oct 9, 2020

Uh oh!

swift-ci commented Oct 9, 2020

Uh oh!

drodriguez commented Oct 10, 2020

Uh oh!

Uh oh!

drodriguez commented Oct 4, 2020 •

edited

Loading

drodriguez commented Oct 4, 2020 •

edited

Loading

drodriguez commented Oct 5, 2020 •

edited

Loading

drodriguez commented Oct 8, 2020 •

edited

Loading