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

Remove cc toolchain registration from rules_cc in MODULE.bazel #199

Closed
wants to merge 1 commit into from

Conversation

meteorcloudy
Copy link
Member

@meteorcloudy meteorcloudy commented Sep 21, 2023

The same toolchain is registered by @bazel_tools already, we should not register it again to prevent competing with the apple cc toolchain from apple_support.

Context: bazelbuild/rules_swift#1115 (comment)

The same toolchain is registered by `@bazel_tools` already, we should not register it again to prevent competing with the apple cc toolchain from `apple_support`.
@comius
Copy link
Collaborator

comius commented Sep 21, 2023

In principle: I'd say the toolchain falls naturally into rules_cc and not bazel_tools. I think the registration should be remove from bazel_tools instead.

Is this a temporary solution? Or will Bazel users when bazel_tools are gone, register C++ toolchain in their repos?

@meteorcloudy
Copy link
Member Author

Is this a temporary solution? Or will Bazel users when bazel_tools are gone, register C++ toolchain in their repos?

I think in long term, yes, we should move the toolchain registration to rules_cc and remove the ones in bazel_tools.

But until we figure out a better way to specify toolchain registration priority, I believe it's safer to keep it in bazel_tools. Because we can make sure bazel_tools is always appended at the end of the root modules' MODULE.bazel file, but have no control on the order of rules_cc and apple_support.

/cc @katre @Wyverald

@comius
Copy link
Collaborator

comius commented Sep 21, 2023

Is this a temporary solution? Or will Bazel users when bazel_tools are gone, register C++ toolchain in their repos?

I think in long term, yes, we should move the toolchain registration to rules_cc and remove the ones in bazel_tools.

But until we figure out a better way to specify toolchain registration priority, I believe it's safer to keep it in bazel_tools. Because we can make sure bazel_tools is always appended at the end of the root modules' MODULE.bazel file, but have no control on the order of rules_cc and apple_support.

/cc @katre @Wyverald

Wait. Don't apple users have complete control? MODULE.bazel is the last. Anybody using @rules_apple, just need to put it after @rules_cc.

@meteorcloudy
Copy link
Member Author

meteorcloudy commented Sep 21, 2023

Anybody using @rules_apple, just need to put it after @rules_cc.

That's true! But with this change, they don't need to do that at all. I don't feel super strongly with this change, as there is a workaround, @keith @brentleyjones

@meteorcloudy
Copy link
Member Author

@keith @brentleyjones Is there a case that the end user would actually prefer the vanilla cc toolchain over the apple one? If so, keeping the toolchain registration in rules_cc might actually be better since they can easily control that by adjusting to order of rules_cc and apple_support. But of course, there should be a better solution in long term, maybe as @fmeum suggested here.

@fmeum
Copy link
Contributor

fmeum commented Sep 21, 2023

The toolchain in rules_cc is problematic in any case as it diverged from the one in Bazel itself and may no longer work well with more recent versions of Bazel. I even tried to remove it at some point to prevent confusion.

I would thus recommend:

  1. Remove the toolchain registration in rules_cc now. Whoever wants the vanilla toolchain even with apple_support should just register the one from bazel_tools anyway.
  2. Move the vanilla C++ toolchain from bazel_tools to rules_cc.
  3. Come up with a better long-term solution that allows choosing between multiple registered toolchains.

@brentleyjones
Copy link

brentleyjones commented Sep 21, 2023

Is there a case that the end user would actually prefer the vanilla cc toolchain over the apple one?

For rules_apple users, no. And most won't have a rules_cc bazel_dep, so they won't be able to change the order of it.

@comius
Copy link
Collaborator

comius commented Sep 21, 2023

The toolchain in rules_cc is problematic in any case as it diverged from the one in Bazel itself and may no longer work well with more recent versions of Bazel. I even tried to remove it at some point to prevent confusion.

I would thus recommend:

  1. Remove the toolchain registration in rules_cc now. Whoever wants the vanilla toolchain even with apple_support should just register the one from bazel_tools anyway.
  2. Move the vanilla C++ toolchain from bazel_tools to rules_cc.
  3. Come up with a better long-term solution that allows choosing between multiple registered toolchains.

Is it possible to copy Bazel's toolchain over rules_cc?
I'd prefer if the toolchain is removed from bazel_tools, like that was done for Java.
The reason that this is moving in the right direction and not leaving more things to do later.

@fmeum
Copy link
Contributor

fmeum commented Sep 21, 2023

Is it possible to copy Bazel's toolchain over rules_cc?

Yes, but probably not in time for Bazel 6.4.0. :-)
If it is added to rules_cc with automatic registration, then users will still get into the situation where bazel_dep order matters. That essentially breaks our story around MODULE.bazel being descriptive rather than imperative, so I think we need to tackle 3. before doing 2. and keeping the automatic registration.

@keith
Copy link
Member

keith commented Sep 21, 2023

could we remove the bazel_tools toolchain registration before bazel 7.x? so that way we'd end up in the good final state here sooner rather than later

@Wyverald
Copy link
Member

I'd personally lean towards removing the cc toolchain registration from bazel_tools (and keeping this one in rules_cc) too. It's conceptually cleaner and removes mental burden.

If it is added to rules_cc with automatic registration, then users will still get into the situation where bazel_dep order matters. That essentially breaks our story around MODULE.bazel being descriptive rather than imperative

I wouldn't say that. Declarative =/= order-insensitive. It's not perfect that we don't yet have a more explicit way to choose from registered toolchains, but the BFS order was a conscious design decision from day 1.

@fmeum
Copy link
Contributor

fmeum commented Sep 22, 2023

I wouldn't say that. Declarative =/= order-insensitive. It's not perfect that we don't yet have a more explicit way to choose from registered toolchains, but the BFS order was a conscious design decision from day 1.

Sorry, I didn't mean to say that this is somehow not working as designed. In the absence of any domain-specific information, BFS is a great default and much better than WORKSPACE's "first come, first serve" semantics.

I just think that BFS works much better between different graph depths (e.g. my direct deps' toolchains win over my transitive deps's toolchains) than on the same depth (reorder bazel_deps to decide which toolchain wins), in particular when we are considering adding import capabilities to module files. Still a decent fallback, but IMO we need something better.

@meteorcloudy
Copy link
Member Author

could we remove the bazel_tools toolchain registration before bazel 7.x? so that way we'd end up in the good final state here sooner rather than later

@keith I can try to move cc toolchains to rules_cc before Bazel 7 cut, but that means we do need to depend on rules_cc and apple_support order. Can you help document this somewhere for apple users until we figure out a better solution to control the toolchain registration order?

Since we are heading to the final state anyway, I'll give up on this PR.

@keith
Copy link
Member

keith commented Sep 22, 2023

is the ordering issue only if the user directly depends on both? vs bazel depending on rules_cc but not apple_support, and even apple_support depending on rules_cc (which we could add for this if it would help)

@Wyverald
Copy link
Member

is the ordering issue only if the user directly depends on both?

yes -- if the user only directly depends on apple_support, everything's fine. The other two cases you mentioned aren't affected.

keith added a commit to bazelbuild/apple_support that referenced this pull request Sep 22, 2023
@keith
Copy link
Member

keith commented Sep 22, 2023

keith added a commit to bazelbuild/apple_support that referenced this pull request Sep 22, 2023
@meteorcloudy
Copy link
Member Author

Thanks for updating the doc!

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.

6 participants