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

Revert "Add new darwin CC toolchain for tests (#3460)" #3499

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

linzhp
Copy link
Contributor

@linzhp linzhp commented Mar 28, 2023

This reverts #3460.

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?
Revert cd98170 to unblock rules_go release

Which issues(s) does this PR fix?

Fixes #3498

Other notes for review
@keith FYI

@linzhp linzhp requested a review from fmeum March 28, 2023 20:10
@keith
Copy link
Member

keith commented Mar 28, 2023

if you have only the command line tools you won't be able to run those tests starting with bazel 7.x. So this isn't a long term fix.

@linzhp
Copy link
Contributor Author

linzhp commented Mar 28, 2023

if you have only the command line tools you won't be able to run those tests starting with bazel 7.x

Can you give more context on that? Does it only affect those tests or Go packages in general? //go/tools/releaser is a pretty simple Go package, so I assume if affects all Go packages. It would be a pretty big migration effort to install full XCode on all dev machines in a big corporation.

@linzhp
Copy link
Contributor Author

linzhp commented Mar 28, 2023

Also, we should move towards a direction of hermetic C++ compilers (e.g., using https://github.com/uber/bazel-zig-cc) . Overriding a generic @bazel_tools//tools/cpp:toolchain with a local toolchain further breaks the hermeticity, right?

@keith
Copy link
Member

keith commented Mar 28, 2023

The toolchain you're removing here is only needed for the test that uses an objc_library, so otherwise you should be able to disable it. The problem is the env var isn't enough because of this config:

https://github.com/bazelbuild/rules_go/blob/13aa4706394734625e441c9334d88dc3bd566e40/.bazelrc#L1-L4

so we also need a way to opt out of that in this case 🤔

@linzhp
Copy link
Contributor Author

linzhp commented Mar 28, 2023

We can have a different buildkite step to add those toolchains and run the objc_library.

@linzhp linzhp merged commit 4660427 into bazel-contrib:master Mar 28, 2023
@linzhp linzhp deleted the revert_cpp branch March 28, 2023 20:59
@keith
Copy link
Member

keith commented Mar 28, 2023

sounds good, note that this change might break rules_go in the bazel downstream pipeline

@linzhp
Copy link
Contributor Author

linzhp commented Mar 28, 2023

Should we add those flags to bazel downstream pipeline too?

@keith
Copy link
Member

keith commented Mar 28, 2023

i think it gets them from your .bazelci config

tingilee pushed a commit to tingilee/rules_go that referenced this pull request Jul 19, 2023
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.

//go/tools/releaser doesn't build on macOS
4 participants