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

chore: expose toolchains helper as public API #207

Closed
wants to merge 3 commits into from
Closed

Conversation

alexeagle
Copy link
Contributor

It's meant to be used in toolchainization of language rules, for example in rules_python: https://github.com/bazelbuild/rules_python/pull/1577/files#diff-9cb07a0e8453ad8b4f429bee2ccf9de818a78cfb6e5d9bb908d8518ea189e2a7R18

Fixes that PR following the recent breaking change landed here: d4c3498#commitcomment-139420930

@alexeagle alexeagle requested review from comius and a team as code owners March 5, 2024 23:34
alexeagle added a commit to thesayyn/rules_python that referenced this pull request Mar 6, 2024
Relies on bazelbuild/rules_proto#207 landing and being available in a rules_proto release.
@alexeagle
Copy link
Contributor Author

The other answer might be that this helper is intended to be private API, and we should copy-paste logic from it into each ruleset?

@@ -32,3 +32,4 @@ proto_lang_toolchain = _proto_lang_toolchain
# Modules
proto_common = _proto_common
ProtoInfo = _ProtoInfo
toolchains = _toolchains
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer not to expose additional symbols in defs.bzl. Each symbol should be in a separate bzl file. That reduces transitively loaded bzl files and prevents repo, macro, and rule code from needlessly loading each other.

https://docs.google.com/document/d/1L1JFgjpZ7SrBinb24DC_5nTIELeYDacikcme-YcA7xs/edit

Also, exposing symbol toolchains seems to be a very general name.
Also toolchains contains a temporary transitional functionality, doesn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try to explain more clearly:

In language ruleset implementations like py_proto_library, do you expect these three helpers to be available?
https://github.com/bazelbuild/rules_proto/blob/main/proto/proto_common.bzl#L51-L53

  • use_toolchain
  • find_toolchain
  • if_legacy_toolchain

Currently I'm forced to copy-paste some of that logic instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I expect those 3 helpers to be temporary. After toolchain resolution is flipped on and well tested, the helpers may be removed. Since they need to be removed from wherever they are used I believe that copy-pasting is a better approach in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note @fmeum as you recommended them yesterday

alexeagle added a commit to thesayyn/rules_python that referenced this pull request Mar 19, 2024
Relies on bazelbuild/rules_proto#207 landing and being available in a rules_proto release.
@comius comius closed this May 15, 2024
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.

2 participants