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

Optionally infer name in use_extension and use_repo_rule #22168

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Apr 29, 2024

Following the documented best practices for rules and extensions, a symbol named foo_deps should be available from a .bzl file called foo_deps.bzl. While this already reduces cognitive load during "Where do I get this extension from?", it still results in pretty repetitive MODULE.bazel lines:

foo_deps = use_extension("@rules_foo//foo/extensions:foo_deps.bzl", "foo_deps")

Instead, Bazel now uses this convention to infer the name if not given:

foo_deps = use_extension("@rules_foo//foo/extensions:foo_deps.bzl")

This makes typos less likely and also nudges ruleset authors to adopt the new convention by providing nicer syntax.

RELNOTES: use_extension and use_repo_rule now infer the name from the basename of the .bzl file if not provided explicitly.

Following the documented best practices for [rules and extensions](https://docs.google.com/document/d/1L1JFgjpZ7SrBinb24DC_5nTIELeYDacikcme-YcA7xs/edit#heading=h.cy0m49ne5key), a symbol named `foo_deps` should be available from a `.bzl` file called `foo_deps.bzl`. This still results in pretty duplicative `MODULE.bazel` lines:

```
foo_deps = use_extension("@rules_foo//foo/extensions:foo_deps.bzl", "foo_deps")
```

Instead, Bazel now uses this convention to infer the name if not given:

```
foo_deps = use_extension("@rules_foo//foo/extensions:foo_deps.bzl")
```

This makes typos less likely and also nudges ruleset authors to adopt the new convention by providing nice syntax.

RELNOTES: `use_extension` and `use_repo_rule` now infer the name from the basename of the `.bzl` file if not provided explicitly.
@github-actions github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Apr 29, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Apr 29, 2024

Note to self: Make buildozer aware of this too when accepted.

@Wyverald
Copy link
Member

This seems a bit too magical to me; I don't think saving the extra parameter is worth the potential confusion. Open to be convinced otherwise though.

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 30, 2024

My own thoughts on why I personally consider this a net win:

  • Since use_extension differs from load in so many ways already, losing one more similarity with it doesn't matter too much.
  • In a future world where every ruleset follows the style guide, the two arg variant will just no longer be relevant.
  • A number of languages already follow the "filename is main entrypoint" paradigm, for example Java and JS modules.
  • Compared to other approaches such as enumerating all symbols in the file and picking the unique one if it exists, there isn't any dynamic magic at play here, just string manipulation on an explicitly provided argument.

I will try to get more eyes on this by posting about it in the #bzlmod channel.

@cgrindel
Copy link

My two cents. I like this magic. More wizardry and unicorns, please. 🦄🧙‍♂️

@fmeum
Copy link
Collaborator Author

fmeum commented May 8, 2024

@alexeagle @aherrmann @keith @illicitonion for their opinion on this idea

@aherrmann
Copy link
Contributor

I also fear that this is a bit too magical.

On the pro side

  • it does reduce noise and boilerplate a bit,
  • foo = use_extension is different from load in that it only brings one symbol into scope and that is returned and assigned,
  • arguably there is precedent for this kind of thing with the @foo and //bar label short-cuts.

On the cons side

  • it seems to be optimizing more for writing than reading code,
  • it's yet another special case for people to learn,
  • I fear it will raise an expectation that the same kind of short-cut exists for load("@rules_lang//lang/lang_binary.bzl").

Question

  • Why is use_extension so different from load? Why don't we write use_extension("@rules_foo//foo/extensions:foo_deps.bzl", "foo_deps")? That would also eliminate one repetition of foo_deps, and a different name could be assigned with kwargs as with load.

@alexeagle
Copy link
Contributor

Bazel has existing semantics for the default label in a package, which uses the same convention for "the symbol named the same as the package"). I see that as an argument that the same concept could apply in starlark.

I fear it will raise an expectation that the same kind of short-cut exists for load("@rules_lang//lang/lang_binary.bzl").

I had the same thought - every time I load from skylib I'm typing the same thing twice, but no one is making "default symbol to load"

Since use_extension differs from load in so many ways already, losing one more similarity with it doesn't matter too much

I think this does matter quite a bit - it increases the cognitive burden of understanding how Bazel works.

symbol named foo_deps should be available from a .bzl file called foo_deps.bzl.

We still have most repos with extensions.bzl which was the common convention for a year or so - and since they don't plan to grow more extensions it will probably stay that way. In those cases we don't feel this problem.

@aherrmann
Copy link
Contributor

We still have most repos with extensions.bzl which was the common convention for a year or so - and since they don't plan to grow more extensions it will probably stay that way. In those cases we don't feel this problem.

Very good point. AFAIK there is also no way for Bazel modules to change this without incurring a breaking change, see "extension identity" docs. So, it seems unlikely that we'll see a wider shift to the new recommendation soon.

@fmeum
Copy link
Collaborator Author

fmeum commented May 15, 2024

I fear it will raise an expectation that the same kind of short-cut exists for load("@rules_lang//lang/lang_binary.bzl").

That's a very good point. Let's wait until Starlarkification has progressed to the point where these loads are actually found in all build files. If load gets this behavior as a way to reduce boilerplate, then use_extension probably should as well, but if it never gets to that, the increased cognitive load would not be worth it.

I will close this PR for now and revisit it later.

@fmeum fmeum closed this May 15, 2024
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants