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

wrapped_clang is not built with the requested Xcode versions #8902

Open
keith opened this issue Jul 16, 2019 · 23 comments · Fixed by MobileNativeFoundation/rules_xcodeproj#1100
Labels
not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) platform: apple team-Rules-CPP Issues for C++ rules type: bug

Comments

@keith
Copy link
Member

keith commented Jul 16, 2019

Description of the problem / feature request:

Currently wrapped_clang is built as part of the local_config_cc setup:

xcrun_result = repository_ctx.execute([
"env",
"-i",
"xcrun",
"clang",
"-std=c++11",
"-lc++",
"-o",
"wrapped_clang",
wrapped_clang_src_path,
], 30)

Currently it doesn't take into account what you pass for --xcode_version. Because of this if you change your xcode-select on macOS and build with a different version of Xcode than is requested with this flag, you produce a different wrapped_clang. This ends up breaking remote cache compatibility with other versions even if the build would otherwise be done with the correct Xcode version.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

  • sudo xcode-select -s /Applications/Xcode-10.2.1.app
  • bazel build some_ios_target --xcode_version=10.2.1 --remote_http_cache=...
  • bazel clean --expunge
  • sudo xcode-select -s /Applications/Xcode-11beta3.app
  • bazel build some_ios_target --xcode_version=10.2.1 --remote_http_cache=...

With the second build, you will get nearly no remote cache hits because wrapped_clang differs as an input to every action, even though you requested the same Xcode version from the command line.

What operating system are you running Bazel on?

macOS

What's the output of bazel info release?

release 0.28.0

@keith keith changed the title Build wrapped_clang with different Xcode versions invalidates cache wrapped_clang is not built with the requested Xcode versions Jul 16, 2019
@keith
Copy link
Member Author

keith commented Jul 16, 2019

This would be less of an issue if changing Xcode versions globally with xcode-select invalidated wrapped_clang, but this requires a clean --expunge to recover from

@iirina iirina added team-Rules-CPP Issues for C++ rules untriaged labels Jul 16, 2019
@scentini scentini added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels Jul 22, 2019
@keith
Copy link
Member Author

keith commented Aug 16, 2019

@scentini do you have a suggestion for how to fix this? Having to clean --expunge whenever switching versions here is a pain for our large team

@scentini
Copy link
Contributor

cc/ @hlopko

@scentini scentini added P2 We'll consider working on this in future. (Assignee optional) and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) labels Aug 23, 2019
@keith
Copy link
Member Author

keith commented Nov 21, 2019

@hlopko do you have any suggestion for how to fix this?

@brentleyjones
Copy link
Contributor

This should also potentially consider the DEVELOPER_DIR environment variable, per my duped issue: #11716.

@keith
Copy link
Member Author

keith commented Jul 6, 2020

Would passing that through cause invalidation when the user's Xcode version changed? Maybe if we correctly accessed it in the workspace rule it could help?

@brentleyjones
Copy link
Contributor

If the user set their Xcode version that way, then yes. But if all they did was change xcode-select -s, I don't believe so.

@keith
Copy link
Member Author

keith commented Jul 6, 2020

Yea, I was at least thinking that in your bazel wrapper you could always specify that var if it would fix this

@brentleyjones
Copy link
Contributor

brentleyjones commented Jul 6, 2020

Ohh, yeah, it could read the set value and then export it. The workspace rule can state that it should invalidate based on the environment variable. I think that would work nicely.

It would differ from --xcode_version though.

bazel-io pushed a commit that referenced this issue Oct 22, 2020
Resolves #11716, which partially addresses #8902. With this change, and some careful environment manipulation (probably in a wrapper script) you can ensure that `wrapped_clang` is built with the correct version of Xcode.

Closes #11719.

PiperOrigin-RevId: 338465098
@oquenchil oquenchil added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: bug and removed P2 We'll consider working on this in future. (Assignee optional) labels Nov 19, 2020
@oquenchil
Copy link
Contributor

If the PR above didn't fix this issue, please re-open.

@brentleyjones
Copy link
Contributor

We can specify the DEVELOPER_DIR in a bazel wrapper now after #11719, but I'm not sure if @keith's original issue was fixed.

@keith
Copy link
Member Author

keith commented Nov 19, 2020

Using DEVELOPER_DIR in a wrapper around bazel definitely works around the original issue, but it would be nice if it was solved for everyone without having to manually do that.

@oquenchil oquenchil reopened this Nov 23, 2020
@keith
Copy link
Member Author

keith commented Jan 5, 2021

One thing I realized with the DEVELOPER_DIR workaround is that if the user overwrites their Xcode.app with a newer version, the DEVELOPER_DIR will not change, but the version of Xcode will. So that's a case where this workaround isn't enough 😞

@jerrymarino
Copy link
Contributor

@keith one way to sidestep this is to just use a custom local_config_cc. e.g.

bazel fetch :*  && ditto $(bazel info execution_root)/../../external/local_config_cc tools/local_config_cc
# WORKSPACE
local_repository(name="local_config_cc", path="tools/local_config_cc")

YMMV for different host arch than x86_64 🤔

It might be possible to factor out the binaries into their own input to the cc rules rather than compiling in the repo rule, but I don't know the long term plans for this / overlap with rules_cc off the top of my head!

@keith
Copy link
Member Author

keith commented Jan 11, 2021

copying that in is definitely an interesting idea, we also run some things in our build on Linux so I think we'd have to do that in a cross platform way though. Definitely handling the arch problem (although you could check fat binaries in there instead if you only supported macOS) would be necessary too

I think doing the input route is easier said than done because it'd be a bit of a circular dependency

@jerrymarino
Copy link
Contributor

jerrymarino commented Jan 11, 2021

@keith in order to make the binary an input to the toolchain w/o a circle you'll need another rule to compile it: e.g. a rule that consumes only the relevant xcode config bits and not the custom toolchain.

As the toolchain will be specific to macOS, you might need to override local_config_cc conditionally when on macOS to use the hardcoded one. You might be able to get away with switching the entire generated toolchain with --override_repository instead of having to create a cross platform one.

@jerrymarino
Copy link
Contributor

For what it's worth, it seems like making wrapped_clang binaries an input to the toolchain seems like good way to fix this outside of forking the generated one

jerrymarino added a commit to jerrymarino/bazel that referenced this issue Feb 9, 2021
This compiles wrapped_clang and wrapped_clang_pp inside of an action.
bazelbuild#8902 has a larger definition
of the issue.

This action needs the DEVELOPER_DIR as an input in order to select the
correct clang. This is passed in from apple_common.XcodeVersionConfig.

Note that this works for local execution and needs to address the TODO
for cc_wrapper.sh in tools/cpp/osx_cc_configure.bzl.
@jerrymarino
Copy link
Contributor

I've put up a WIP commit to test out compiling the wrappers with a rule that depends on the Xcode env. It looks like we can compile this with the apple_common.apple_host_system_env( and then point the tool path at the output of the rule.

It seems like this is the right approach - plus fixing the last TODO is addressed on that commit. Does anyone see any downsides to compiling this with a rule? If not I can prepare that commit for a PR cc @keith @hlopko

@brentleyjones
Copy link
Contributor

apple_host_system_env is nice, since it includes XCODE_VERSION_OVERRIDE, would would be different for different Xcode versions, even with the same DEVELOPER_DIR.

@jerrymarino
Copy link
Contributor

@brentleyjones yep passing the entire Xcode env is how I was looking to solve it for good. This is similar to how other C compiler rules work 👍 #11716 alone won't help the use case where people use Xcode mac app store to install Xcode to `/Applications/Xcode.app

The 3 ways I know of to fix this are:

  • clean the repo cache when the Xcode version changes
  • manage the toolchain binaries in another way ( e.g. commit them ) - likely this is what others are doing
  • compile them with the apple_host_system_env and pass paths into the toolchain

brentleyjones added a commit to MobileNativeFoundation/rules_xcodeproj that referenced this issue Sep 14, 2022
This is three changes:
- On the command-line, set `--xcode_version` to whatever `xcode-select` is set to
- In Xcode, set `--xcode_version` to `$XCODE_PRODUCT_BUILD_VERSION`
- For both, set `--repo_env=USE_CLANG_CL= $XCODE_PRODUCT_BUILD_VERSION` to work around bazelbuild/bazel#8902
brentleyjones added a commit to MobileNativeFoundation/rules_xcodeproj that referenced this issue Sep 14, 2022
This is three changes:
- On the command-line, set `--xcode_version` to whatever `xcode-select`
is set to
- In Xcode, set `--xcode_version` to `$XCODE_PRODUCT_BUILD_VERSION`
- For both, set `--repo_env=USE_CLANG_CL= $XCODE_PRODUCT_BUILD_VERSION`
to work around bazelbuild/bazel#8902
brentleyjones added a commit to MobileNativeFoundation/rules_xcodeproj that referenced this issue Sep 14, 2022
While `bazel_build.sh` should still have `--xcode_version`, to ensure that Bazel uses the version of Bazel you are building inside of, setting `--xcode_version` based on `xcode-select` can counter-act some people's use of `--xcode_config`. We leave the `--repo_env` though to at least try to fix bazelbuild/bazel#8902.
brentleyjones added a commit to MobileNativeFoundation/rules_xcodeproj that referenced this issue Sep 14, 2022
While `bazel_build.sh` should still have `--xcode_version`, to ensure
that Bazel uses the version of Xcode you are building inside of, setting
`--xcode_version` based on `xcode-select` can counter-act some people's
use of `--xcode_config`. We leave the `--repo_env` though to at least
try to fix bazelbuild/bazel#8902.
chiragramani pushed a commit to chiragramani/rules_xcodeproj that referenced this issue Sep 14, 2022
…iveFoundation#1099)

This is three changes:
- On the command-line, set `--xcode_version` to whatever `xcode-select`
is set to
- In Xcode, set `--xcode_version` to `$XCODE_PRODUCT_BUILD_VERSION`
- For both, set `--repo_env=USE_CLANG_CL= $XCODE_PRODUCT_BUILD_VERSION`
to work around bazelbuild/bazel#8902
chiragramani pushed a commit to chiragramani/rules_xcodeproj that referenced this issue Sep 14, 2022
While `bazel_build.sh` should still have `--xcode_version`, to ensure
that Bazel uses the version of Xcode you are building inside of, setting
`--xcode_version` based on `xcode-select` can counter-act some people's
use of `--xcode_config`. We leave the `--repo_env` though to at least
try to fix bazelbuild/bazel#8902.
@brentleyjones
Copy link
Contributor

To recap since this has bitten me again: if you have --xcode_version set, you need to make sure DEVELOPER_DIR or xcode-select -s is set to the same version. Otherwise your wrapped_clang binary might mismatch. On a mismatch you probably won't get any remote cache hits.

We probably want a fix that uses apple_host_system_env as mentioned above.

Copy link

github-actions bot commented Jan 1, 2024

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Jan 1, 2024
@brentleyjones
Copy link
Contributor

@bazelbuild/triage not stale

@iancha1992 iancha1992 added not stale Issues or PRs that are inactive but not considered stale and removed stale Issues or PRs that are stale (no activity for 30 days) labels Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) platform: apple team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants