Skip to content

Revert #1564#1663

Merged
illicitonion merged 5 commits intobazelbuild:mainfrom
illicitonion:zig-repro
Nov 29, 2022
Merged

Revert #1564#1663
illicitonion merged 5 commits intobazelbuild:mainfrom
illicitonion:zig-repro

Conversation

@illicitonion
Copy link
Copy Markdown
Collaborator

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

@illicitonion illicitonion force-pushed the zig-repro branch 2 times, most recently from 3214d86 to 0552cc5 Compare November 18, 2022 20:00
@illicitonion
Copy link
Copy Markdown
Collaborator Author

@keith - I would be interested in your thoughts on the CI failure we get from reverting this PR...

The original PR filtered out the -target flag from the cc_toolchain when propagating to cargo_build_scripts. This is causing problems for toolchains which actually need the -target flag.

AFAICT what's actually currently needed to fix the issues with building iOS applications is to pass --cpu=ios_x86_64 on the command line, and that this is effectively a dupe of bazelbuild/bazel#16784 - that Bazel decides what cc_toolchain and flags &c to configure based on --cpu rather than --platforms...

Is there a nicer way of making the iOS stuff work that doesn't require setting the --cpu flag? Or do you have thoughts on what kind of manipulating of cc_toolchain stuff we should be doing to satisfy iOS builds? I'm pretty convinced that just stripping out -target is incorrect, but it's a little sad if setting --cpu is required as a workaround...

Otherwise, presumably we should start setting --cpu on both of these CI builds?

ios_examples:
name: iOS Examples
platform: macos
working_directory: examples/ios
build_targets:
- "//..."
ios_build:
name: iOS build script cross compile test
platform: macos
working_directory: examples/ios_build
test_flags:
- "--platforms=//:ios_x86_64"
test_targets:
- "//..."
- the first one doesn't appear to need it because the code involved isn't complex enough (or we don't have sufficiently pedantic asserts), but the second does seem to...

@UebelAndre
Copy link
Copy Markdown
Collaborator

Seems a test needs to be updated for iOS?

@illicitonion
Copy link
Copy Markdown
Collaborator Author

Seems a test needs to be updated for iOS?

Yeah - the test is fixed by adding --cpu=ios_x86_64 to the iOS example build, which I think is the correct fix for the issue, but I don't know the iOS stuff well enough to know if there's a better one...

@dae
Copy link
Copy Markdown
Contributor

dae commented Nov 23, 2022

Sorry about the unintended breakage; no objections from me on reverting the change.

@UebelAndre
Copy link
Copy Markdown
Collaborator

@dae would you be able to confirm what the right solution is for #1663 (comment) ?

@dae
Copy link
Copy Markdown
Contributor

dae commented Nov 25, 2022

If @illicitonion confirmed the test I added passes when --cpu=ios_x86_64 is provided, that sounds like the best solution, as it addresses the problem at the source, and it's the solution I would have used had I known specifying a platform was not enough.

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.
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
Copy Markdown
Collaborator Author

/cc again @keith post-Thanksgiving - I'm adding a --cpu flag to the iOS Rust example bazelrc files - does that look reasonable to you, or is there a better way to make the iOS toolchain pieces work?

@keith
Copy link
Copy Markdown
Member

keith commented Nov 28, 2022

What part of this change breaks it in that way (or maybe it was just broken at HEAD?)

For ios_build I think an issue is not having a platform_mappings file, did the issue appear for both ios and ios_build? (not sure why those are separate either

@keith
Copy link
Copy Markdown
Member

keith commented Nov 28, 2022

Overall I guess I don't think passing that flag hurts for various examples here, but I'm not sure what impact this might have on downstream users

@illicitonion
Copy link
Copy Markdown
Collaborator Author

Aha - yeah, it looks like we were missing the platform_mappings file, and instead were directly setting --platforms in the CI for the ios_build example - we were only seeing the issue in that one repo. Thanks for looking!

I added a platform_mappings, and changed the .bazelci file to set the flags mentioned in the platform_mappings file instead of the one it sets. This fixes things up :)

@illicitonion illicitonion merged commit 29233e3 into bazelbuild:main Nov 29, 2022
@illicitonion illicitonion deleted the zig-repro branch November 29, 2022 11:34
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.

4 participants