Differential D40229
[cmake] Remove redundant call to cmake when building host tools. Authored by hintonda on Nov 19 2017, 1:48 PM.
Details
Remove the redundant, config-time call to cmake when The config-time call to cmake is redundant because it will always get
Diff Detail
Event TimelineComment Actions Had to roll this back due to bot breakage (r319268). The problem was due to passing a cmake list, ';' separated, to add_custom_command, which evaluated the list variable and removed the ';' characters. So, I think the correct fix, would be to not pass LLVM_TARGETS_TO_BUILD. Does that sound reasonable? Comment Actions Only pass LLVM_TARGETS_TO_BUILD=Native for host tools, since that's Comment Actions Due to the complexities of multi-stage builds, removing the extra step of running cmake for native tools at config time doesn't seem to be worth the effort. So, I'm abandoning this patch for now. Comment Actions Just wanted to add a note for posterity: It seems the cmake passes a few variables, e.g., CMAKE_(C|CXX)_COMPILER, when calling execute_process, but not when calling add_custom_command. Since it's unclear -- to me -- which variables get passed and under what conditions, it's not feasible to replace execute_process with add_custom_command without a fuller understanding of what is and isn't passed. Comment Actions Re-opening. Cmake sets CC and CXX during config time so users can run configure, or cmake in this case, with the same compiler (see discussion by Brad King here: https://gitlab.kitware.com/cmake/cmake/issues/16356). Since these are the only environment variables set, we can fix the bots by passing CMAKE_(C|CXX)_COMPILER to add_custom_command. Comment Actions I think this breaks any non-trivial cross-compilation scenario. When compiling for android, we compile the android binaries with the compiler in the android ndk (so CMAKE_C**_COMPILER points to the android ndk). However, this compiler is not capable of compiling any host binaries. So the way we make this work is by setting the host compilers via -DCROSS_TOOLCHAIN_FLAGS_NATIVE. This worked because command line has precedence over the compiler specified by the environment variables. However, this now appends -DCMAKE_C**_COMPILER, which will override whatever compiler we have specified via cross toolchain flags. I am not sure we ever want to be specifying the native compiler, as this is likely to be different in any non-trivial cross-compilation scenario, but if we're going to do that, could we at least put CROSS_TOOLCHAIN_FLAGS last so that this can be overridden by the user who needs it? Comment Actions I'm getting a similar problem where passing in CMAKE_CXX_COMPILER breaks my native build, except it's harder to work around because I don't specify CROSS_TOOLCHAIN_FLAGS. I'm building for a device, and specify a C++ compiler for that build, but that compiler isn't the correct one for building my native tools. When no compiler was specified on the cross-compile-native cmake invocation, this worked, but now this ends up using the wrong compiler and failing. I could probably pass in the default compiler to CROSS_TOOLCHAIN_FLAGS_NATIVE to work around this, but that's awkward since I just want to use cmake's native logic to find it. Comment Actions Actually, ignore this for now. This isn't exactly what's going wrong and I need to look at it a bit more. Comment Actions I'm not sure how you managed to get that to work before this change, as what then used to happen is that cmake would put the compilers into CC and CXX environment variables, and the sub-cmake would then pick those up (that is what this patch is trying to emulate with the -D flags). I have uploaded a patch (D40947) which makes these flags optional. This way, you should be able to get a more-or-less pristine setup by forcing the CROSS_TOOLCHAIN_FLAGS_NATIVE variable to an empty string. If we also go through with Don's earlier idea to leave CMAKE_C(XX)_COMPILER flags empty by default in cross-compilation scenarios (which I'm starting to like now), this should even work for you out-of-the-box. |