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

Fix build scripts targeting the wrong architecture #1564

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

dae
Copy link
Contributor

@dae dae commented Sep 25, 2022

PR #1081 started passing CFLAGS to build scripts, which cargo does too. But unfortunately Bazel's cpp toolchain adds an explicit target to the CFLAGS it provides, and that target uses the host platform instead of the target platform. When building a crate like zstd on a Mac with iOS as the target, this ends up in the crate being built for macOS instead of iOS.

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Is this specific to a particular toolchain you're using? It'd be great to add a test for this (as I can see breaking it in the future), but I don't think we currently have any tests using a custom toolchain... We do have an example which builds an iOS app on macos on CI, which is currently passing, though...

@dae
Copy link
Contributor Author

dae commented Sep 30, 2022

I've added a small check for this to the examples/ios folder - if the first commit is reverted, the test will fail. Note it only affects build scripts that compile C(PP) code, such as blake3/zstd.

@illicitonion
Copy link
Collaborator

Thanks! Sorry, I meant an end-to-end example, rather than a targeted unit test - could we e.g. add a zstd dependency to the example iOS app? Presumably this should fail to build without your change (hence the need for the change)?

@dae dae marked this pull request as draft October 4, 2022 05:40
@dae
Copy link
Contributor Author

dae commented Oct 4, 2022

I've spent about 3 hours trying to get an end-to-end example based on crates_universe working, but don't understand why the library is not available to the sh_test(). Any help would be appreciated.

@dae dae force-pushed the no-target branch 5 times, most recently from 9d6afd4 to f572e7b Compare October 7, 2022 00:05
@dae
Copy link
Contributor Author

dae commented Oct 7, 2022

Ok, finally figured it out - had to use a filegroup due to bazelbuild/bazel#4519.

PR bazelbuild#1081 started passing CFLAGS to build scripts, which cargo does
too. But unfortunately Bazel's cpp toolchain adds an explicit target to
the CFLAGS it provides, and that target uses the host platform instead
of the target platform. When building a crate like blake3 or zstd on a
Mac with iOS as the target, this ends up in the crate being built for
macOS instead of iOS.
@dae dae marked this pull request as ready for review October 7, 2022 00:27
Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Thanks!

@illicitonion illicitonion merged commit da3d522 into bazelbuild:main Oct 7, 2022
@duarten
Copy link
Contributor

duarten commented Nov 17, 2022

This PR breaks toolchains that set CFLAGS and do expect it to work. For example, I have a zig toolchain set up, which I'm using for cross compilation. I need to set the target when creating the cc_toolchain_config_info, so it correctly flows to the zig binary when invoked by bazel. This seems more like a bug in the ios toolchain than with forwarding the CFLAGS.

@illicitonion
Copy link
Collaborator

@duarten Sorry for the breakage! Do you have a test-case you could supply to repro the issue?

When to forward what things from the toolchain is a bit of an awkward dance (which is why @dae added a test-case for the iOS case) - it'd be great to get better coverage here to help us work out exactly when we should/shouldn't be forwarding what - particularly, that would help us to highlight an iOS toolchain bug.

@duarten
Copy link
Contributor

duarten commented Nov 18, 2022

I pushed a repro here. To repro, run bazel build //....

It contains a rust program that's using ring. Ring attempts to compile a file in arm assembly, which will immediately cause a build failure. I'm using zig for cross compilation, and I need the -target in the cflags to inform zig of the target architecture. Since that switch is being omitted, zig is instructed to compile the assembly file for the exec target, which fails.

illicitonion added a commit to illicitonion/rules_rust that referenced this pull request Nov 18, 2022
It looks like the reported issue is probably related to the ios
toolchain in use, rather than our general toolchain flag propagation.
illicitonion added a commit to illicitonion/rules_rust that referenced this pull request Nov 18, 2022
It looks like the reported issue is probably related to the ios
toolchain in use, rather than our general toolchain flag propagation.
illicitonion added a commit to illicitonion/rules_rust that referenced this pull request Nov 18, 2022
It looks like the reported issue is probably related to the ios
toolchain in use, rather than our general toolchain flag propagation.
@illicitonion
Copy link
Collaborator

@duarten Thanks for the repro, it's been incredibly useful! I've opened #1663 and asked for some feedback from our Rust+iOS expert in working out exactly what else we need to do to resolve this.

@duarten
Copy link
Contributor

duarten commented Nov 18, 2022

Thanks @illicitonion! :)

illicitonion added a commit to illicitonion/rules_rust that referenced this pull request Nov 28, 2022
It looks like the reported issue is probably related to the ios
toolchain in use, rather than our general toolchain flag propagation.
illicitonion added a commit that referenced this pull request Nov 29, 2022
* Add example workspace cross-compiling with zig

We've run into a number of issues, both in rustc and in cargo build
scripts, where we're not properly propagating cc_toolchain information
to compiles and particularly link actions. These have often been hard to
test because we don't have an example set-up with a non-default C++
toolchain.

By wiring up a zig cross-compiling toolchain, we have a concrete example
we can work with. It's also an interesting potential example for folks
who want to set something similar up.

* Revert #1564

It looks like the reported issue is probably related to the ios
toolchain in use, rather than our general toolchain flag propagation.

* Add platform_mappings and --cpu flag to ios_build example
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.

None yet

3 participants