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

Allow python_register_toolchains.name to be the resolved interpreter repo #656

Merged
merged 4 commits into from
Mar 16, 2022

Conversation

UebelAndre
Copy link
Contributor

@UebelAndre UebelAndre commented Mar 15, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Currently documentation suggests that users write the following in their WORKSPACE file to produce a python toolchain

load("@rules_python//python:repositories.bzl", "python_register_toolchains")

python_register_toolchains(
    name = "python3_9",
    # We recommend using the same version your team is already standardized on.
    python_version = "3.9",
)

This then creates a "magic" repository called python3_9_resolved_interpreter, which is based on the name value. I call it magic because it's generally the next thing users want to use in their workspaces (needing to load dependencies via pip_repository rules) but will need to read docs to find the _resolved_interpreter name. I personally find it much more intuitive to allow anything that's called in a WORKSPACE and has a name parameter to actually create a repository matching that name. This makes things more discoverable and easier to maintain.

Issue Number: N/A

What is the new behavior?

This change introduces a second resolved_interpreter_os_alias rule which just uses the name attribute as it's name instead. This has the added benefit of allowing users to wrap the macro in maybe to avoid redundant calls to these repository rules.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@UebelAndre UebelAndre marked this pull request as ready for review March 15, 2022 22:15
@UebelAndre UebelAndre mentioned this pull request Mar 15, 2022
12 tasks
@f0rmiga
Copy link
Collaborator

f0rmiga commented Mar 15, 2022

I don't like this, but I'll let others chime in. Without the resolved_interpreter suffix, it implies that a toolchain is the host interpreter, which is not true.

cc @mattem @alexeagle @thundergolfer

@UebelAndre
Copy link
Contributor Author

UebelAndre commented Mar 15, 2022

I don't like this, but I'll let others chime in. Without the resolved_interpreter suffix, it implies that a toolchain is the host interpreter, which is not true.

cc @mattem @alexeagle @thundergolfer

Thanks for the quick response!

I mostly don't like the feeling that the only repository rule I'll generally interact with is magically created for me. In most cases, name = "foo" produces a repository and I think this is the most appropriate.

I'm not married to this change though but do think it's less burden-of-knowledge, which I like.

@thundergolfer thundergolfer requested review from f0rmiga and removed request for andyscott, brandjon and lberki March 16, 2022 01:55
@thundergolfer
Copy link

thundergolfer commented Mar 16, 2022

I do agree with @UebelAndre that this breaks Bazel user expectations about repository rules:

A custom repository rule can be used just like a native repository rule. It has a mandatory name attribute and every target present in its build files can be referred as @<name>//package:target where <name> is the value of the name attribute. - bazel.build/rules/repository_rules

Currently the rule doesn't produce a @<name> external repository, which is unusual.

@alexeagle
Copy link
Contributor

This goes all the way back to the template
https://github.com/bazel-contrib/rules-template/blob/main/mylang/repositories.bzl#L70-L74
which I wrote after revamping the toolchains design in rules_nodejs. It has this issue as well.

I don't see a good reason why that macro doesn't include the given name in its expansion, like the macro style guide indicates. Naming the "default" repository as the thing most users want to use makes sense to me. In many cases users don't think about the nuances of host/target/exec platforms anyway. More sophisticated users who do care about that will likely be able to read documentation explaining that the default repository is the one that toolchain selection would pick for the host.

Copy link
Collaborator

@mattem mattem left a comment

Choose a reason for hiding this comment

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

personally find it much more intuitive ...

Yeah, I generally agree with this sentiment, and the thing the user cares about is the "resolved" version, not strictly the toolchain (which is kinda abstracted anyway).

An alternative is to add further alias targets manually, eg //python:default to be the interpreter etc.
eg at one client we have multiple interpreter versions in a single graph, so //python:default, //python:py37 etc are all aliased to the interpreter bin, and can create a more intuitive bazel run statement.

@alexeagle alexeagle merged commit e18e1a3 into bazelbuild:main Mar 16, 2022
@UebelAndre UebelAndre deleted the maybe2 branch March 16, 2022 17:25
alexeagle added a commit to bazel-contrib/rules_nodejs that referenced this pull request Dec 12, 2022
Generally it's a bad practice that the user calls a repository rule (nodejs_register_toolchains) with a name, and this doesn't result in a repository with that name.

Leave the old _host variant around to make this a non-breaking change.

We did the same fix in python: bazelbuild/rules_python#656

Fixes #3375
alexeagle added a commit to bazel-contrib/rules_nodejs that referenced this pull request Dec 12, 2022
Generally it's a bad practice that the user calls a repository rule (nodejs_register_toolchains) with a name, and this doesn't result in a repository with that name.

Leave the old _host variant around to make this a non-breaking change.

We did the same fix in python: bazelbuild/rules_python#656

Fixes #3375
alexeagle added a commit to bazel-contrib/rules_nodejs that referenced this pull request Dec 12, 2022
Generally it's a bad practice that the user calls a repository rule (nodejs_register_toolchains) with a name, and this doesn't result in a repository with that name.

Leave the old _host variant around to make this a non-breaking change.

We did the same fix in python: bazelbuild/rules_python#656

Fixes #3375
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