bazelbuild / rules_cc

C++ Rules for Bazel
https://bazel.build
Apache License 2.0
189 stars 94 forks source link

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

Closed meteorcloudy closed 1 year ago

meteorcloudy commented 1 year ago

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: https://github.com/bazelbuild/rules_swift/issues/1115#issuecomment-1728219962

comius commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

https://github.com/bazelbuild/apple_support/pull/265

meteorcloudy commented 1 year ago

Thanks for updating the doc!