This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Remove redundant call to cmake when building host tools.
ClosedPublic

Authored by hintonda on Nov 19 2017, 1:48 PM.

Event Timeline

hintonda created this revision.Nov 19 2017, 1:48 PM
hintonda edited the summary of this revision. (Show Details)Nov 19 2017, 2:22 PM
beanz accepted this revision.Nov 27 2017, 9:50 AM

LGTM.

This revision is now accepted and ready to land.Nov 27 2017, 9:50 AM
This revision was automatically updated to reflect the committed changes.

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?

hintonda reopened this revision.Nov 28 2017, 7:16 PM

Reopen in order to fix builtbot failure.

This revision is now accepted and ready to land.Nov 28 2017, 7:16 PM
hintonda updated this revision to Diff 124686.Nov 28 2017, 7:16 PM

Only pass LLVM_TARGETS_TO_BUILD=Native for host tools, since that's
all that needs to be built.

beanz accepted this revision.Nov 29 2017, 2:56 PM

Seems reasonable to me.

This revision was automatically updated to reflect the committed changes.
hintonda reopened this revision.Dec 1 2017, 12:31 PM

Reopen: breaking multi-stage bots.

This revision is now accepted and ready to land.Dec 1 2017, 12:31 PM
This revision was automatically updated to reflect the committed changes.

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.

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.

hintonda reopened this revision.Dec 2 2017, 11:01 AM

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.

This revision is now accepted and ready to land.Dec 2 2017, 11:01 AM
hintonda updated this revision to Diff 125270.Dec 2 2017, 11:02 AM

Also pass CMAKE_(C|CXX)_COMPILER to add_custom_command.

This revision was automatically updated to reflect the committed changes.
labath added a subscriber: labath.Dec 6 2017, 3:34 AM

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?

bogner added a subscriber: bogner.Dec 6 2017, 3:36 PM

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?

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.

bogner added a comment.Dec 6 2017, 3:52 PM

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.

Actually, ignore this for now. This isn't exactly what's going wrong and I need to look at it a bit more.

labath added a comment.Dec 7 2017, 3:17 AM

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.

Actually, ignore this for now. This isn't exactly what's going wrong and I need to look at it a bit more.

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.