[llvm] Fix cmake string expansion in CrossCompile.cmake #138901
Conversation
This fixes an issue where builds on Windows with LLVM_OPTIMIZED_TABLEGEN will fail if CMAKE_MAKE_PROGRAM, CMAKE_C_COMPILER_LAUNCHER or CMAKE_CXX_COMPILER_LAUNCHER are lists rather than paths to a binary; this occurs when one of these programs is passed with arguments instead of just as a path. On non-Windows this works regardless as the sub-cmake command runs in a proper shell, but on Windows it runs differently and so these strings need to be expanded correctly by cmake.
|
This is necessary because CMake won't expand a list into string if the quoted string isn't surrounded by whitespace. If it is, it will correctly expand and add quotes around the resulting string, so moving the string to cover the whole "-D..." option will make cmake do the right thing here. |
| -DCMAKE_MAKE_PROGRAM="${CMAKE_MAKE_PROGRAM}" | ||
| -DCMAKE_C_COMPILER_LAUNCHER="${CMAKE_C_COMPILER_LAUNCHER}" | ||
| -DCMAKE_CXX_COMPILER_LAUNCHER="${CMAKE_CXX_COMPILER_LAUNCHER}" | ||
| "-DCMAKE_MAKE_PROGRAM=${CMAKE_MAKE_PROGRAM}" |
There was a problem hiding this comment.
Do you have an example where you'd want to pass flags to the native build system?
There was a problem hiding this comment.
I didn't have one specifically for the CMAKE_MAKE_PROGRAM, I just thought I'd fx it as well while I was here as the same reasoning would apply there.
We want this for CMAKE_C/CXX_COMPILER_LAUNCHER, where we want to add flags to ccache when using LLVM_CCACHE_BUILD to control certain behaviour especially when building flang. When we tried to make that change it worked on non-Windows but didn't work on Windows, this fixes at least that case. So that's the specific motivation for the change.
There was a problem hiding this comment.
I suppose I can imagine a case where you might want to enable some debug modes in ninja with -d ...
petrhosek
left a comment
petrhosek
left a comment
There was a problem hiding this comment.
Thanks, we have other places in LLVM where we set these variables and ideally we would make the same change consistently everywhere.
| -DCMAKE_CXX_COMPILER_LAUNCHER="${CMAKE_CXX_COMPILER_LAUNCHER}" | ||
| "-DCMAKE_MAKE_PROGRAM=${CMAKE_MAKE_PROGRAM}" | ||
| "-DCMAKE_C_COMPILER_LAUNCHER=${CMAKE_C_COMPILER_LAUNCHER}" | ||
| "-DCMAKE_CXX_COMPILER_LAUNCHER=${CMAKE_CXX_COMPILER_LAUNCHER}" |
There was a problem hiding this comment.
LLVM_TABLEGEN_FLAGS below should also be fixed (added recently, #138086).
Meinersbur
left a comment
Meinersbur
left a comment
There was a problem hiding this comment.
I think the difference is that the surrounding quotes are handled by CMake itself, so they are never seen when passing them as a command line string.
CMake indeed does not use always use cmd.exe to launch programs, but usually it normalizes paths to absolute paths and using forward slashes instead of backslashes, at least for CMAKE_*_COMPILER. Might be different for others.
|
I completely forgot that I'd submitted this as I've been really busy with other things... |
Meinersbur
left a comment
Meinersbur
left a comment
There was a problem hiding this comment.
OK to commit as-is. It's low impact. LLVM_TABLEGEN_FLAGS can be considered unrelated.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/20164 Here is the relevant piece of the build log for the reference |
7 participants
This fixes an issue where builds on Windows with LLVM_OPTIMIZED_TABLEGEN
will fail if CMAKE_MAKE_PROGRAM, CMAKE_C_COMPILER_LAUNCHER or
CMAKE_CXX_COMPILER_LAUNCHER are lists rather than paths to a binary;
this occurs when one of these programs is passed with arguments instead
of just as a path.
On non-Windows this works regardless as the sub-cmake command runs in a
proper shell, but on Windows it runs differently and so these strings
need to be expanded correctly by cmake.