Skip to content
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

Merged
merged 1 commit into from
May 27, 2020

Conversation

maverickwoo
Copy link
Contributor

This PR uses the array feature in bash to preserve word splitting in wine-msvc.sh. An example assuming a suitable cmake wrapper is in place:

cmake -B build -G "NMake Makefiles" -DCMAKE_BUILD_TYPE=RELWITHDEBINFO .

Before: The word split would not be preserved when re-splitting the ARGS string in Line 19.

# cmake -B build -G "NMake Makefiles" -DCMAKE_BUILD_TYPE=RELWITHDEBINFO .
CMake Error: Could not create named generator NMake
[...]

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!

@@ -1,7 +1,7 @@
#!/bin/bash
EXE="$1"
EXE=$1
Copy link
Owner

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

Copy link

@Cryspia Cryspia May 29, 2020

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.

@mstorsjo
Copy link
Owner

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

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.

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!

Oh, so /Zi works if you just start that process manually? Have you looked into how hard it'd be to fix whatever is wrong when it doesn't work automatically like on windows?

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 /Z7 instead of /Zi.)

@mstorsjo mstorsjo merged commit b953f99 into mstorsjo:master May 27, 2020
@maverickwoo
Copy link
Contributor Author

Oh, so /Zi works if you just start that process manually? Have you looked into how hard it'd be to fix whatever is wrong when it doesn't work automatically like on windows?

The trick to get /Zi to work under wine would be to install winbind. That would however bring another problem, namely the first build will not return because wine will see that mspdbsrv.exe was started in the build but is still running at the end of the build. Our current solution is to start mspdbsrv.exe before the build starts.

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.

  1. 7zip [nmake]
  2. capstone [cmake]
  3. pcre2 [cmake]
  4. putty [nmake]
  5. vim [nmake]
    (If you have any suggested packages for us to test, please feel free to let us know too. Thanks!)

@maverickwoo maverickwoo deleted the word-split branch May 28, 2020 04:09
@mstorsjo
Copy link
Owner

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.

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.

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").

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants