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

feat: expose toolchains #721

Closed
wants to merge 1 commit into from

Conversation

rickvanprim
Copy link

@rickvanprim rickvanprim commented Jan 12, 2024

New Feature

Exposed toolchains so that they can be used independently of registering them with the repo rules.

Test plan

Checked that my code is able to build while using the new public toolchain rules.

@CLAassistant
Copy link

CLAassistant commented Jan 12, 2024

CLA assistant check
All committers have signed the CLA.

@alexeagle
Copy link
Collaborator

Since we have tar I'm generally supportive of adding more common tools

Copy link
Collaborator

@thesayyn thesayyn left a comment

Choose a reason for hiding this comment

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

I don't see where these tools "sed" and "grep" come from

@thesayyn
Copy link
Collaborator

i am beginning to think that we should have another ruleset called rules_bash where bring in these tools hermetically. bazel-lib is becoming bloated.

@alexeagle
Copy link
Collaborator

https://github.com/tweag/rules_sh is already maintained by some Nix experts 😉

@rickvanprim
Copy link
Author

@thesayyn this PR doesn't have actually fetching the tools yet. I wanted buy in on adding this before I put that work in. Plus I'm using this with Nix so I don't actually need that part for my stuff 😅

Regarding rules_bash (or rules_sh), how would you feel about something like toolchains (much like platforms) that just contains the toolchain_types, the providers, and the rules for instantiating the toolchain, so there's a common definition. Actual fetching of the toolchains would belong in other repos. I am discussing this a little bit with @alexeagle and Matt Clarkson on Slack 🙂

@alexeagle
Copy link
Collaborator

I think the toolchain_types have to go somewhere else
bazel-contrib/SIG-rules-authors#91

@rickvanprim rickvanprim marked this pull request as ready for review February 19, 2024 05:56
@rickvanprim rickvanprim changed the title Expose Toolchains and add grep/sed toolchains. Expose Toolchains and add ripgrep/sd toolchains. Feb 19, 2024
@rickvanprim rickvanprim changed the title Expose Toolchains and add ripgrep/sd toolchains. Expose Toolchains Jun 26, 2024
@rickvanprim rickvanprim changed the title Expose Toolchains feat: expose toolchains Jun 26, 2024
@rickvanprim
Copy link
Author

@alexeagle / @thesayyn greatly scoped this down to just exporting the toolchain rules. Also happy to close this and just import them from private in my code if that's preferred.

@alexeagle
Copy link
Collaborator

I don't see on this thread an answer to: why would you want to load the toolchain symbols independently from registering them? Any increase to the public API surface constrains what maintainers are able to change, so it should come with an accompanying end-user benefit.

@rickvanprim
Copy link
Author

I updated the overall PR description to cover the motivation, which is registering these toolchains without the binaries being provided by aspect_bazel_lib. If that doesn't align with the goals here, we can just close this, and I will just directly import them from the private directory.

@alexeagle
Copy link
Collaborator

Sorry we never got to this one. Now that it's in bazel-contrib we have a wider maintainer base. Maybe @fmeum has an opinion about the toolchain_type's being exposed as public API, but I'm still inclined to say it's rare enough that you should just use the private API and deal with unlikely breaking changes

@fmeum
Copy link
Member

fmeum commented Sep 19, 2024

Until the community stabilizes on best practices for "bunch of files" toolchains, I would also consider it too risky to expose these publicly (see bazelbuild/bazel#19645).

@rickvanprim
Copy link
Author

Thanks @alexeagle and @fmeum. I'll just use it privately 🙂

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.

5 participants