-
-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Take care of word split concerns in wine-msvc.sh #15
Conversation
@@ -1,7 +1,7 @@ | |||
#!/bin/bash | |||
EXE="$1" | |||
EXE=$1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing the quotes here - aren't these necessary? (I do see that I had missed them in the invocation at the bottom, but afaik they should be necessary in both places, right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing the quotes here - aren't these necessary? (I do see that I had missed them in the invocation at the bottom, but afaik they should be necessary in both places, right?)
According to the Bash manual, expansions, including word splitting, happen after a command line has been split into tokens (aka lexing). In this case, the command line is recognized as a single assignment command a(EXE) <- b($1)
in the interpreter before word splitting, so whether to add a pair of double quotes or not will not affect the meaning of this command.
Oh, I didn't know that bash had such a convention - ok, then I guess the change should be ok - I wasn't aware of that bash specific detail. I generally try to write strictly posix-only shell scripts, but here I had to make them bash specific for other reasons, and making them properly handle arguments with spaces of course is a good extra benefit.
Oh, so I already have patches for cmake and ninja, for building properly with these tools, see e.g. https://gitlab.kitware.com/mstorsjo/cmake/-/commits/msvc and https://github.com/mstorsjo/ninja/commits/cross-msvc. Please have a look at these and see what things you've done differently. I've been using these patches (in one form or another) for a couple years already, with a couple different projects, including cross compiling LLVM with MSVC. (The patches for cmake makes it use |
The trick to get I am not familiar with what can go wrong on Windows in the second part. If you can describe it more here, maybe we can look into that. P.S. In our repo (https://github.com/pangine/msvc-wine), we added cmake and ninja without any patches. It's just adding shell script wrappers to the official Windows binaries. Reading your patches, I think we haven't run into problems yet because so far we are still compiling relatively simple packages (so we have not run into hard cases like "msvc mangles case in deps output"). Our latest progress as of this writing is in the "script-improvements" branch. We are still testing and we will likely be rebasing onto 20.04 and re-test. Right now we are not yet confident that our patches are correct and so we are not ready to issue any further PR. However, that branch can build the following packages in an essentially "out of the box" manner, i.e., we simply follow the developer-provided build instructions. When we submit any PR in the future, we will note at least one package we use for testing that PR and the commands used for the build.
|
I see - that explains it. I don't run the windows binary versions of it, but I run the native build tools, just calling the wine wrapped individual build tools. That way it interacts better with cases where some build tools (invoked by ninja) are native and some are run in wine. The same tradeoff was brought up in ninja-build/ninja#1735, and there were a few others who also are into the same use case, of running wine wrapped compilers within a native unix build system. I'm not entirely sure I wholeheartedly agree with the direction of bringing in these tools wrapped in wine - maybe as an optional add-on somehow - or just kept in your fork.
That happens if the source directories contain both upper and lower case, and you build it with an older compiler (IIRC it was fixed in some of the first releases of MSVC 2019). If you run ninja as a native windows executable, it does all path lookups case insensitively, so the whole issue disappears in that case. |
This PR uses the array feature in bash to preserve word splitting in
wine-msvc.sh
. An example assuming a suitablecmake
wrapper is in place:Before: The word split would not be preserved when re-splitting the
ARGS
string in Line 19.After: The word split is preserved.
There are several other possible solutions to this problem, but we feel using an array is the cleanest since we are using bash here. We also converted the script to follow the word split behavior of bash (e.g., not quoting in line 2 because bash does not split in normal variable assignments).
P.S. We are adding support for parallel compilation (start
mspdbsrv.exe
to enable/Zi
), cmake, ninja, etc. in our repo. The goal is enable out-of-the-box compilation of more packages. Right now we are still in the testing phase and we plan to issue PRs once we know the patches are stable with our test cases. Please feel free to take a look and give us feedback. Thanks!