-
Notifications
You must be signed in to change notification settings - Fork 7k
[libffi] Support LLVM on Windows by adding quotes around the path to CCAS #46287
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
base: master
Are you sure you want to change the base?
Conversation
The path to CCAS may include spaces. On Windows, the build process runs within a MSYS2 shell, which doesn't deal withs paces in paths very well. Add quotes around the path to fix this. This fixes building libffi with the LLVM toolchain on Windows.
e36a159
to
6ad7e87
Compare
@@ -37,6 +37,8 @@ if(VCPKG_DETECTED_CMAKE_C_COMPILER_ID STREQUAL "MSVC") | |||
if(ccas_options) | |||
vcpkg_list(APPEND options "CCASFLAGS=\${CCASFLAGS}${ccas_options}") | |||
endif() | |||
else() | |||
vcpkg_list(APPEND options "CCAS='${VCPKG_DETECTED_CMAKE_C_COMPILER}'") |
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.
I acknowledge that the PR passes CI for libffi, but...
... this overrides the CCAS
setting from vcpkg_configure_make
which carries "ABIFLAGS" in addition to the tool executable.
In general, autoconf packages are extremely volatile around spaces in paths. These variable values are passed from Windows CMake to Msys2 bash/configure to Msys2 bash/libtool. Quoting and escaping has to be done all the time, and it isn't always done correctly. That's why the directory of cl.exe
is added to PATH
instead.
I would be glad if the problems could be overcome, and maybe the vcpkg-make
port is already better than the old scripts. But we really need to be sure what we need in vcpkg-make
and what we need in individual ports.
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.
This overrides the CCAS setting from vcpkg_configure_make which carries "ABIFLAGS" in addition to the tool executable.
I don't disagree, but the same is being done in the MSVC case, right ? (line 26)
That's why the directory of cl.exe is added to PATH instead.
I assume that could be done; we could parse VCPKG_DETECTED_CMAKE_C_COMPILER
, add the directory to PATH, and then use the executable name for CCAS?
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.
I assume that could be done; we could parse VCPKG_DETECTED_CMAKE_C_COMPILER, add the directory to PATH, and then use the executable name for CCAS?
@qmfrederik This seems worth a try, it avoids the spaces in paths issue that is the root of so many problems.
This PR fixes compiling libffi on Windows, when using the LLVM toolchain.
The path to CCAS may include spaces. On Windows, the build process runs within a MSYS2 shell, which doesn't deal withs paces in paths very well. Add quotes around the path to fix this.
This fixes building libffi with the LLVM toolchain on Windows.
./vcpkg x-add-version --all
and committing the result.