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

Clean up and remove toolchain transition code #14127

Open
katre opened this issue Oct 18, 2021 · 19 comments
Open

Clean up and remove toolchain transition code #14127

katre opened this issue Oct 18, 2021 · 19 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability Issues for Configurability team type: feature request

Comments

@katre
Copy link
Member

katre commented Oct 18, 2021

The toolchain transition is now the default: it's time to clean up the code and remove it.

  1. Remove uses of incompatible_use_toolchain_transition.
  2. Remove uses of the flag incompatible_override_toolchain_transition.
  3. Remove the code and the flag!
@katre katre added type: feature request P2 We'll consider working on this in future. (Assignee optional) team-Configurability Issues for Configurability team labels Oct 18, 2021
@katre katre self-assigned this Oct 18, 2021
@keith
Copy link
Member

keith commented Oct 27, 2021

What is the recommendation for rules that want to continue supporting bazel 4.x?

@katre
Copy link
Member Author

katre commented Oct 27, 2021

Bazel 5.0 has been cut and will be released shortly. If you want to delay merging these PRs until that happens and support Bazel 4.x until then, that's fine. If you want to reject this PR and continue to support Bazel 4.x, that is also fine. However, note that the incompatible_use_toolchain_transition parameter will be removed before Bazel 6.0 is released.

aiuto pushed a commit to bazelbuild/rules_pkg that referenced this issue Oct 27, 2021
… enabled by (#452)

default in Bazel 5.0.

This is a step towards removing it entirely.

Part of bazelbuild/bazel#14127.
shahms added a commit to kythe/kythe that referenced this issue Oct 27, 2021
… enabled by (#5118)

default in Bazel 5.0.

This is a step towards removing it entirely.

Part of bazelbuild/bazel#14127.

Co-authored-by: Shahms King <shahms@google.com>
copybara-service bot pushed a commit to google/xls that referenced this issue Oct 27, 2021
… enabled by default.

This is a step towards removing it entirely.

Part of bazelbuild/bazel#14127.

PiperOrigin-RevId: 405968009
@jsharpe
Copy link

jsharpe commented Oct 28, 2021

Bazel 5.0 has been cut and will be released shortly. If you want to delay merging these PRs until that happens and support Bazel 4.x until then, that's fine. If you want to reject this PR and continue to support Bazel 4.x, that is also fine. However, note that the incompatible_use_toolchain_transition parameter will be removed before Bazel 6.0 is released.

So what you're essentially saying here then is that it's not going to be possible for rules to support bazel version 4.x through 6.x in a single release of the ruleset? Given the LTS support windows for bazel 4.x and bazel 6.x overlap isn't this pushing rules authors to end support for 4.x in their rules earlier than LTS support for version 4.x runs out? I know that the LTS model only applies to bazel itself and not the rules but it does have implications on what the wider community decide to support. @alexeagle this is probably the kind of change that the core team need to communicate to the rules authors SIG as, as highlighted above, not all the repos that are affected have been caught.

@alexeagle
Copy link
Contributor

Yes and also, you shouldn't send PRs to rulesets that break their compat with LTS.

copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this issue Oct 28, 2021
… enabled by default.

This is a step towards removing it entirely.

Part of bazelbuild/bazel#14127.

PiperOrigin-RevId: 406138396
Change-Id: I729a1298c5fc14386ac90bfdf8f9dff65777e23a
@katre
Copy link
Member Author

katre commented Oct 28, 2021

Valid points from everyone. I think I've jumped the gun on this change, apologies.

I'm going to close out the PRs (and ask owners of the ones that merged if they want to revert), and I won't send these until later.

I will also leave the incompatible_use_toolchain_resolution argument to rule() as a no-op until just before 6.0 is cut.

This should allow rules to keep the parameter and be compatible with 4.x and 5.x.

However, I do want to get this functionality removed from Bazel so that I can clean up old code and flags in the near future.

@katre
Copy link
Member Author

katre commented Oct 28, 2021

(Note that the google-internal version of this will continue, but any rules which are vendored into google's repo can refuse the change and stay as they are).

@jsharpe
Copy link

jsharpe commented Oct 28, 2021

I guess this does raise the interesting question of what LTS support is in the context of supporting an incompatible flag being turned on for a version.

One could make the case that the incompatible flag in bazel 4.x that requires this argument doesn't fall under LTS support and thus that removing this code from rules targeting 4.x is still a valid thing to do, but guess that is a wider question for discussion by the product team?

copybara-service bot pushed a commit to protocolbuffers/protobuf that referenced this issue Dec 18, 2023
Now that Bazel 7.0 has been released, it's time to remove this tech debt.

This has been a no-op since Bazel 5.0, I've been waiting to remove the code for
two years, it's time.

Part of bazelbuild/bazel#14127.

PiperOrigin-RevId: 591912356
copybara-service bot pushed a commit to protocolbuffers/protobuf that referenced this issue Dec 18, 2023
Now that Bazel 7.0 has been released, it's time to remove this tech debt.

This has been a no-op since Bazel 5.0, I've been waiting to remove the code for
two years, it's time.

Part of bazelbuild/bazel#14127.

PiperOrigin-RevId: 591912356
copybara-service bot pushed a commit to protocolbuffers/protobuf that referenced this issue Dec 18, 2023
Now that Bazel 7.0 has been released, it's time to remove this tech debt.

This has been a no-op since Bazel 5.0, I've been waiting to remove the code for
two years, it's time.

Part of bazelbuild/bazel#14127.

PiperOrigin-RevId: 591940644
katre added a commit to katre/p4c that referenced this issue Dec 18, 2023
Now that Bazel 7.0 has been released, it's time to remove this tech debt.

This has been a no-op since Bazel 5.0, I've been waiting to remove the code for
two years, it's time.

Part of bazelbuild/bazel#14127.
fruffy pushed a commit to katre/p4c that referenced this issue Dec 19, 2023
Now that Bazel 7.0 has been released, it's time to remove this tech debt.

This has been a no-op since Bazel 5.0, I've been waiting to remove the code for
two years, it's time.

Part of bazelbuild/bazel#14127.
fruffy pushed a commit to p4lang/p4c that referenced this issue Dec 19, 2023
Now that Bazel 7.0 has been released, it's time to remove this tech debt.

This has been a no-op since Bazel 5.0, I've been waiting to remove the code for
two years, it's time.

Part of bazelbuild/bazel#14127.
copybara-service bot pushed a commit to protocolbuffers/protobuf that referenced this issue Dec 19, 2023
Now that Bazel 7.0 has been released, it's time to remove this tech debt.

This has been a no-op since Bazel 5.0, I've been waiting to remove the code for
two years, it's time.

Part of bazelbuild/bazel#14127.

PiperOrigin-RevId: 591912367
copybara-service bot pushed a commit to protocolbuffers/protobuf that referenced this issue Dec 19, 2023
Now that Bazel 7.0 has been released, it's time to remove this tech debt.

This has been a no-op since Bazel 5.0, I've been waiting to remove the code for
two years, it's time.

Part of bazelbuild/bazel#14127.

PiperOrigin-RevId: 592219242
katre referenced this issue in bazelbuild/rules_java Dec 19, 2023
PiperOrigin-RevId: 592244091
Change-Id: I0e4a4f2552fa0f30d41d9d84126ff06dbd6cb097
gutron pushed a commit to gutron/rules_elisp that referenced this issue Dec 19, 2023
Now that Bazel 7.0 has been released, it's time to remove this tech debt.

This has been a no-op since Bazel 5.0, and @katre has been waiting to remove the
code for two years, it's time.

Part of bazelbuild/bazel#14127.
katre added a commit to katre/rules_oci that referenced this issue Dec 19, 2023
Now that Bazel 7.0 has been released, it's time to remove this tech debt.

This has been a no-op since Bazel 5.0, I've been waiting to remove the code for
two years, it's time.

Part of bazelbuild/bazel#14127.
phst pushed a commit to phst/rules_elisp that referenced this issue Dec 19, 2023
Now that Bazel 7.0 has been released, it's time to remove this tech debt.

This has been a no-op since Bazel 5.0, and @katre has been waiting to remove the
code for two years, it's time.

Part of bazelbuild/bazel#14127.
katre added a commit to katre/opentitan that referenced this issue Dec 20, 2023
Now that Bazel 7.0 has been released, it's time to remove this tech debt.

This has been a no-op since Bazel 5.0, I've been waiting to remove the code for
two years, it's time.

Part of bazelbuild/bazel#14127.
@katre
Copy link
Member Author

katre commented Dec 20, 2023

Finished most of the cleanup, still waiting on bazelbuild/rules_java#163 and a few google-internal changes before I can submit the change to remove the parameter.

copybara-service bot pushed a commit that referenced this issue Jan 2, 2024
Needed to finish removing `incompatible_use_toolchain_transition`, part of #14127.

Closes #20713.

PiperOrigin-RevId: 595195840
Change-Id: Ie82cfcfe24fd423c42bf87e60d2056bc93b90a5b
katre added a commit to katre/bazel that referenced this issue Feb 28, 2024
from upb.

Part of removing the toolchain transition entirely, bazelbuild#14127.
katre added a commit to katre/bazel that referenced this issue Feb 29, 2024
from upb.

Part of removing the toolchain transition entirely, bazelbuild#14127.
copybara-service bot pushed a commit that referenced this issue Feb 29, 2024
Part of removing the toolchain transition entirely, #14127.

Closes #21516.

PiperOrigin-RevId: 611554677
Change-Id: I364158df3877476f8ca8b3c0c9c8e520208efbae
katre added a commit to katre/bazel that referenced this issue Mar 12, 2024
This removes it from `rule` and `aspect`.

Fixes bazelbuild#14127.

PiperOrigin-RevId: 590266374
Change-Id: I3a946c2387660f4bac65c9bdb0ec92bc7bb909d9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability Issues for Configurability team type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants