Add use_repo support for isolated module extension usages#1171
Add use_repo support for isolated module extension usages#1171vladmos merged 2 commits intobazelbuild:masterfrom
use_repo support for isolated module extension usages#1171Conversation
buildozer/README.md
Outdated
| * `use_repo_remove [dev] <extension .bzl file> <extension name> <repo(s)>`: | ||
| with `dev_dependency = True` will be considered instead. If `index` is | ||
| specified, the `index`-th (0-based) extension usage with `isolate = True` | ||
| will be considered instead, otherwise such usages are ignored. |
There was a problem hiding this comment.
it's a bit unclear to me just from reading this whether the index includes dev usages (or non-dev usages if I'm calling use_repo_add dev ...). Could you clarify?
IOW, with this MODULE.bazel file:
usage1 = use_extension("//:ext.bzl", "ext", isolate=True)
usage2 = use_extension("//:ext.bzl", "ext", isolate=True, dev_dependency=True)
usage3 = use_extension("//:ext.bzl", "ext", isolate=True)
Does use_repo_add //:ext.bzl ext:1 refer to usage2 or usage3?
There was a problem hiding this comment.
I tried to clarify this in the README (and added test coverage), let me know if it reads too "mathy" now.
There was a problem hiding this comment.
hmm. so the answer to my previous question is usage3, right?
this is maybe a bit late, but I think making the answer usage2 would be simpler to explain (and probably to implement). It also allows us to remove isDevUsage from IsolationKey. Sorry for not bringing this up earlier.
Another idea I want to bring up is -- could we somehow allow just use_repo_add usage1 <repo(s)>? (Maybe with a different syntax or something, since with the way the dev keyword works right now, this would be ambiguous.) This syntax should be much more user-friendly (compare use_repo_add //:ext.bzl ext:1 <repo(s)> against use_repo_add usage1 <repo(s)>), and easier to implement (at least on the buildozer side; on the Bazel side, this would require using the export hook to know the name of the variable the proxy value is assigned to). This would also make buildozer unable to work with code like use_repo(use_extension("//:ext.bzl","ext"), "repo_name"), which is a tradeoff I'm more than willing to make.
There was a problem hiding this comment.
hmm. so the answer to my previous question is
usage3, right?
Yes, that's right.
this is maybe a bit late, but I think making the answer
usage2would be simpler to explain (and probably to implement). It also allows us to removeisDevUsagefromIsolationKey. Sorry for not bringing this up earlier.
I see pros and cons for both ways of counting as well as for having isDevUsage in the key. But I like your next idea much better, so I will not list them for now. :-)
Another idea I want to bring up is -- could we somehow allow just
use_repo_add usage1 <repo(s)>? (Maybe with a different syntax or something, since with the way thedevkeyword works right now, this would be ambiguous.) This syntax should be much more user-friendly (compareuse_repo_add //:ext.bzl ext:1 <repo(s)>againstuse_repo_add usage1 <repo(s)>), and easier to implement (at least on the buildozer side; on the Bazel side, this would require using the export hook to know the name of the variable the proxy value is assigned to). This would also make buildozer unable to work with code likeuse_repo(use_extension("//:ext.bzl","ext"), "repo_name"), which is a tradeoff I'm more than willing to make.
That's a great idea, it's much nicer than having to count usages, regardless of whether distinguished by dev/non-dev.
Taking this one step further, what do you think of the following plan:
- Require isolated extension proxies to be exported and enforce that their exported names do not collide within one module file (including dev usages even if ignoring their effects).
- Use the exported name to form the extensionUniqueName, replacing the count. This is still unique, but much easier to trace back to a particular usage and also stable across --ignore_dev_dependency flips.
- Allow
use_repo_add usage1 <repo(s)>in buildozer. It should be unambiguous as valid extension labels always start with@or/, whereas valid repo or extension proxy names don't.
What I am not sure about is whether to allow the new command also for non-isolated usages. Its semantics could be confusing as use_repo can't really be updated for a single proxy in isolation if there are multiple proxies for a single extension usage. On the other hand this should be rare and the syntax is very nice and simple for humans to produce.
There was a problem hiding this comment.
Require isolated extension proxies to be exported
Like I said previously, I'm okay with requiring this for all proxies.
enforce that their exported names do not collide within one module file
I think that's already enforced -- IIRC you can't rebind top-level names to different values in Starlark.
Use the exported name to form the extensionUniqueName, replacing the count.
SGTM.
What I am not sure about is whether to allow the new command also for non-isolated usages. Its semantics could be confusing as use_repo can't really be updated for a single proxy in isolation if there are multiple proxies for a single extension usage.
I mean, if there are multiple proxies for a single extension, the semantics are already unclear today (which proxy are we updating the use_repo clause for?). So I'm very much okay with the name of one such proxy standing in for all of them.
There was a problem hiding this comment.
Require isolated extension proxies to be exported
Like I said previously, I'm okay with requiring this for all proxies.
Just to make sure there is no misunderstanding: What you said above is that you are okay with requiring a proxy to be exported for buildozer to work. But what I'm referring to here is requiring this for Bazel to accept the module file, which for non-isolated usages could be seen as a (minor) backwards incompatible change.
enforce that their exported names do not collide within one module file
I think that's already enforced -- IIRC you can't rebind top-level names to different values in Starlark.
That's convenient and actually makes a lot of sense now that I think about it.
What I am not sure about is whether to allow the new command also for non-isolated usages. Its semantics could be confusing as use_repo can't really be updated for a single proxy in isolation if there are multiple proxies for a single extension usage.
I mean, if there are multiple proxies for a single extension, the semantics are already unclear today (which proxy are we updating the
use_repoclause for?). So I'm very much okay with the name of one such proxy standing in for all of them.
The current semantics are that all non-isolated proxies are collected and all their use_repo statements are updated, with new repos being added to the last such statement. That's an arbitrary choice, but it always leads to the correct result.
But with the new command variant, there are two ways this could be implemented:
- Just look at the
use_repos for that particular proxy. That's simple, but if e.g. the repo to remove is imported via a different proxy for the same extension, it won't be seen and thus not removed. - From the given proxy, look up the corresponding extension and consider all proxies for it as before. That requires additional effort, but avoids the situation described in 1.
Based on your last sentence, I assume you would prefer 2?
There was a problem hiding this comment.
Just to make sure there is no misunderstanding: What you said above is that you are okay with requiring a proxy to be exported for buildozer to work. But what I'm referring to here is requiring this for Bazel to accept the module file, which for non-isolated usages could be seen as a (minor) backwards incompatible change.
ah, true -- I don't really mind the minor incompatible change. We could also give unexported extension proxies some really ugly name (like "unexported_a0b467") and (somewhat?) sidestep the problem.
Based on your last sentence, I assume you would prefer 2?
Indeed. All such proxies are merged together in practice anyway. It also saves us some trouble generating the correct use_repo fixup command in Bazel -- otherwise keeping track of which proxy use_repo'd which repo is IMO too much work for a very corner use case.
|
Ok, please ping when the PR is ready. |
|
@vladmos I updated the PR, including the description, to match the new way of identifying isolated module extensions. |
|
@Wyverald Do you think we should merge this now to support the experimental |
|
I'm okay with merging this -- after all we do output a buildozer command for users who enable |
|
@vladmos Could you merge this and, if you have the time, publish a release? |
|
@fmeum I'll cut a release likely on Thursday |
Complements bazelbuild/bazel#18529, which introduces support for marking
use_extensioncalls in module files withisolate = True. Such usages will be isolated from all other usages and maintain their own distinct set of repositories, hence the need to manipulate theiruse_repodeclarations in isolation.Isolated usages are always exported (this is enforced by Bazel) and can thus be conveniently identified via their exported name. Since the
use_repo_add <proxy name> <repos>...form is more friendly for humans (although less friendly for tooling), it is also supported for non-isolated extension usages, where it expands to all proxies that represent the same extension usage.