Add use_repo support for isolated module extension usages

[fmeum]

Complements bazelbuild/bazel#18529, which introduces support for marking use_extension calls in module files with isolate = True. Such usages will be isolated from all other usages and maintain their own distinct set of repositories, hence the need to manipulate their use_repo declarations 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.

[fmeum]

@Wyverald @vladmos Could you review?

[Wyverald]

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?

[fmeum]

I tried to clarify this in the README (and added test coverage), let me know if it reads too "mathy" now.

[Wyverald]

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.

[fmeum]

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 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.

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 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.

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:

1. 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).
2. 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.
3. 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.

[Wyverald]

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.

[fmeum]

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_repo clause 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:

1. 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.
2. 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?

[Wyverald]

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.

[vladmos]

Thanks!

[fmeum]

@vladmos Please don't merge yet, @Wyverald's comments from above have led to changes in Bazel that will also require me to change this PR.

[vladmos]

Ok, please ping when the PR is ready.

[fmeum]

@vladmos I updated the PR, including the description, to match the new way of identifying isolated module extensions.

[fmeum]

@Wyverald Do you think we should merge this now to support the experimental isolate feature as well as the more user-friendly syntax or rather wait for it to be stabilized? There is no explicit mention of isolated usages in the API, so if we decide to drop that support later we would only need a doc update.

[Wyverald]

I'm okay with merging this -- after all we do output a buildozer command for users who enable isolate experimentally, and it'd be nice to not tell them lies :)

[fmeum]

@vladmos Could you merge this and, if you have the time, publish a release?

[vladmos]

@fmeum I'll cut a release likely on Thursday