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

Forward @bazel_tools//tools/sh:toolchain_type to rules_shell #24748

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Dec 18, 2024

This is required for compatibility with modules that rely on the auto-registered sh_toolchain to obtain a shebang, e.g. rules_js.

Also remove implementations and toolchain registrations in favor of forwarders.

This is required for compatibility with modules that rely on the auto-registered `sh_toolchain` to obtain a shebang, e.g. `rules_js`.

Also remove implementations and toolchain registrations in favor of forwarders.
@fmeum fmeum requested a review from a team as a code owner December 18, 2024 16:50
@fmeum fmeum requested review from gregestren and meteorcloudy and removed request for a team and gregestren December 18, 2024 16:50
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 18, 2024

FYI @comius

@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Dec 18, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 18, 2024

@bazel-io fork 8.0.1

@fmeum fmeum changed the title Forwards @bazel_tools//tools/sh:toolchain_type to rules_shell Forward @bazel_tools//tools/sh:toolchain_type to rules_shell Dec 18, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 18, 2024

FYI @alexeagle as this may break rules_{,node}js users with Bazel 8.0.0

@alexeagle
Copy link
Contributor

Thank you! I don't think we saw that report yet but perhaps the problem is unreachable due to other Bazel 8 errors.

@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Dec 19, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Dec 23, 2024
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Dec 23, 2024
This is required for compatibility with modules that rely on the auto-registered `sh_toolchain` to obtain a shebang, e.g. `rules_js`.

Also remove implementations and toolchain registrations in favor of forwarders.

Closes bazelbuild#24748.

PiperOrigin-RevId: 709149200
Change-Id: I758cf403c24f6b5e6f00bbf5b4e0313a30773f78
@fmeum fmeum deleted the fix-shell-rules branch December 25, 2024 20:38
github-merge-queue bot pushed a commit that referenced this pull request Jan 2, 2025
…ll` (#24793)

This is required for compatibility with modules that rely on the
auto-registered `sh_toolchain` to obtain a shebang, e.g. `rules_js`.

Also remove implementations and toolchain registrations in favor of
forwarders.

Closes #24748.

PiperOrigin-RevId: 709149200
Change-Id: I758cf403c24f6b5e6f00bbf5b4e0313a30773f78

Commit
720e15e

Co-authored-by: Fabian Meumertzheim <[email protected]>
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 8.0.1 RC1. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=8.0.1rc1. Thanks!

@BoleynSu
Copy link
Contributor

I am seeing cannot load '@@rules_shell+//shell:sh_toolchain.bzl': no such file
Seems to me the path in the PR is wrong. It should be shell/toolchains:sh_toolchain.bzl

def _sh_toolchain_impl(ctx):
"""sh_toolchain rule implementation."""
return [platform_common.ToolchainInfo(path = ctx.attr.path)]
load("@rules_shell//shell:sh_toolchain.bzl", _sh_toolchain = "sh_toolchain")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmeum this seems to be wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants