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

fix: ensure module toolchains are dev dependencies #9

Closed
wants to merge 1 commit into from
Closed

fix: ensure module toolchains are dev dependencies #9

wants to merge 1 commit into from

Conversation

jrandolf
Copy link

@jrandolf jrandolf commented May 15, 2024

These cause errors when a user tries to declare their own toolchain using the default repository. For example:

protoc = use_extension("@toolchains_protoc//protoc:extensions.bzl", "protoc")
protoc.toolchain(
    version = "v27.0-rc3",
)
use_repo(protoc, "toolchains_protoc_hub")

@CLAassistant
Copy link

CLAassistant commented May 15, 2024

CLA assistant check
All committers have signed the CLA.

@jrandolf
Copy link
Author

jrandolf commented May 15, 2024

So after investigating things further, I was able to fix this using

protoc.toolchain(
    name = "protoc_toolchains",
    version = "v27.0-rc3",
)
use_repo(protoc, "protoc_toolchains")

which makes sense because protoc_toolchains is a different repo that doesn't clash with the default one.

However, I find this counter intuitive to other stable toolchains such as rust and nodejs. I suggest we either remove the default one and enforce users to pick their own (which is almost always the way to do things to ensure there are no surprises with versioning) or we somehow remove/decouple the com_google_protobuf attribute from the toolchain and require users to choose their protobuf type implementation (this is what's causing the clash in the repro).

@alexeagle
Copy link
Member

Here is where the logic is intended to allow the root module to pick the version
https://github.com/aspect-build/toolchains_protoc/blob/main/protoc/extensions.bzl#L18-L21

@alexeagle
Copy link
Member

Let's start with an issue rather than a PR since we don't agree on what the problem is nor the expected behavior.

@alexeagle alexeagle closed this May 24, 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.

None yet

3 participants