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

configure_make: support autotools cross-compilation #1247

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

novas0x2a
Copy link
Contributor

@novas0x2a novas0x2a commented Aug 7, 2024

This teaches the autotools-based configure script to pass --host when
cross-compilation is detected. This fixes at least part of #1082. This
functionality is off by default (because configure_make can be used for
non-autotools builds) and can be enabled by passing configure_xcompile
to configure_make.

This functionality is also extended to the built_tools that use autoconf
(pkg-config and make).

It also does a bit of generalization to make it easier to support more
platforms in the future, and adds s390x to the detected arches.

@novas0x2a novas0x2a force-pushed the autotools-xcompile branch 2 times, most recently from 95620ea to 389ea88 Compare August 8, 2024 05:18
@novas0x2a novas0x2a marked this pull request as ready for review August 8, 2024 05:35
@novas0x2a
Copy link
Contributor Author

@jsharpe looks like this probably works now! The only failing builds are the ones that seem to be failing on main, too. (I've tested this in our internal setup on linux, and @matt-sm should be testing on darwin).

Copy link
Member

@jsharpe jsharpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - will wait on @matt-sm to test on darwin though as I had some questions about whether the macos triplet names were correct..


install_cmd = ["./make install"]

xcompile_options = detect_xcompile(ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks correct but when does the built make tool (or pkgconfig?) ever get cross compiled? I would have thought that it was always in the exec configuration and so would always be built natively?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In rfcc that's true, but due to annoying historical reasons, we build make for both the exec and the target. For autoconf, anything that doesn't do the --host thing when cross-compiling causes it to just break, so I defaulted to adding it everywhere that used autotools (we don't build pkg-config for the target, I was just being consistent to minimize surprise)

foreign_cc/private/cmake_script.bzl Show resolved Hide resolved
foreign_cc/private/framework/platform.bzl Show resolved Hide resolved
foreign_cc/private/framework/platform.bzl Show resolved Hide resolved
Comment on lines 182 to 199
elif os == "macos":
if arch == "aarch64":
return "aarch64-apple-macos"
elif arch == "x86_64":
return "x86_64-apple-macos"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is macos correct? On my mac at least the triplet names used for gcc installed via homebrew are aarch64-apple-darwin23

Copy link
Contributor Author

@novas0x2a novas0x2a Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the convention for homebrew is ${arch}-apple-darwin${version}. Other examples we saw internally (from running config.guess on those platforms) were (aarch64|x86_64)-apple-darwin23.(4|5).0 and I was slightly nervous about lying about the library version, so I checked to see what config.sub would do (since that's what autotools uses to normalize) and it uses macos as the default os name, but then also preserves the darwinXX when it sees it:

$ ./config.sub-63acb96f92473ceb5e21d873d7c0aee266b3d6d3 aarch64-apple-macos
aarch64-apple-macos
$ ./config.sub-63acb96f92473ceb5e21d873d7c0aee266b3d6d3 aarch64-apple
aarch64-apple-macos
$ ./config.sub-63acb96f92473ceb5e21d873d7c0aee266b3d6d3 aarch64-apple-darwin123
aarch64-apple-darwin123

I took this as a sign that macos was fine. A slightly more correct way to guess the host might actually be to ship config.guess and run it as an action to see what it says, and then use that, but since there's no good way to do that reliably for the target tuple, I biased towards internally-consistent normalization.

One other consideration is, I don't currently pass --build, just --host. This means it's possible that autoconf's cross_compiling flag will get set to true when it shouldn't be (like in the darwin case above). I did that because it's more likely to result in a working result when the target's not in our tuple, but it does also mean passing --host aarch64-apple-darwin23 when config.guess thinks it's aarch64-apple-darwin23.4 triggers cross-compiling mode. The fix for this would be to pass both --host and --build all the time, so our consistent normalization prevents that from being a problem. (I did confirm this in our CI, our darwin builds do end up emitting checking whether we are cross compiling... yes)

I'm happy to adjust this to whichever variation you think is best! After having written this out, I'm actually more inclined to pass --build under every scenario we pass --host and rely on other users updating the data as-needed, but I'll defer to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did think of a disadvantage of passing --build: there's no way (that I know of) for bazel to guess which libc the compiler is expecting, because there aren't consistent libc constraints to use. I don't think this actually breaks anything, though, because it appears that when autoconf finds musl it actually internally replaces the triplet with gnu anyway.

I think I'll make the change anyway (it can't break any existing code because autotools would already be breaking in that case) but just something to keep an eye on.

@novas0x2a
Copy link
Contributor Author

LGTM - will wait on @matt-sm to test on darwin though as I had some questions about whether the macos triplet names were correct..

Mentioned in the review comments above, but I ended up running this in our darwin CI and it worked.

@novas0x2a novas0x2a force-pushed the autotools-xcompile branch 2 times, most recently from 920c1d6 to 95cc3b1 Compare August 9, 2024 01:42
@novas0x2a
Copy link
Contributor Author

I pushed a new version with a few fixes, including fixing two brown-paper-bag fixes (I had two bugs that cancelled each other out, that I've now fixed)

This teaches the autotools-based configure script to pass --host when
cross-compilation is detected. This fixes at least part of bazel-contrib#1082. This
functionality is off by default (because configure_make can be used for
non-autotools builds) and can be enabled by passing configure_xcompile
to configure_make.

This functionality is also extended to the built_tools that use autoconf
(pkg-config and make).

It also does a bit of generalization to make it easier to support more
platforms in the future, and adds s390x to the detected arches.
Copy link
Member

@jsharpe jsharpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I'm sure we'll get fixes to the triple logic if people run into it when cross compiling if we've missed something there...

@jsharpe jsharpe merged commit 10d47d4 into bazel-contrib:main Aug 12, 2024
2 checks passed
@allsey87
Copy link
Contributor

@novas0x2a could you add a brief example here of how to use xcompile_options with configure_make?

I might make another PR to add the wasm32 CPU and emscripten OS to the supported platforms.

@novas0x2a
Copy link
Contributor Author

novas0x2a commented Aug 13, 2024

@allsey87:

@novas0x2a could you add a brief example here of how to use xcompile_options with configure_make?

Looks like the docs generation hasn't run yet, but, the version that landed just has a bool, configure_xcompile; defaults to false, if you set it to true it will pass --host and --build (as detected by bazel) to configure. Note that this is causing rfcc to detect the cross-compilation, but only if bazel is already operating in cross-compile mode.

I might make another PR to add the wasm32 CPU and emscripten OS to the supported platforms.

It'd need to match the upstream tokens:

@allsey87
Copy link
Contributor

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