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

"Optional" and "nodep" external dependencies #25214

Open
Wyverald opened this issue Feb 5, 2025 · 9 comments
Open

"Optional" and "nodep" external dependencies #25214

Wyverald opened this issue Feb 5, 2025 · 9 comments
Assignees
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request

Comments

@Wyverald
Copy link
Member

Wyverald commented Feb 5, 2025

These are two related feature requests for external dependencies.

  • Optional dependencies: A module might request a dependency on another module, but only if the other module already exists in the dep graph by some other means.
    • We've traditionally rejected this feature request on the grounds that it can usually be solved by splitting the module. That is, the part of the module that uses the optional dep can usually be split into a separate module, and the separate module can specify the dep while the original module leaves it out. This is a more parsimonious solution, at the cost of some maintenance burden (setting up an extra module is often a nontrivial amount of work).
    • A new use case was brought up, where a module wishes to open up the visibility of some of its targets to a sibling module. This sibling module can optionally be used along the original module, and needs to access some of its internals. This use case cannot be solved by splitting the original module up. The current workaround requires the original module to open up the visibility to everyone by using //visibility:public, which is less than ideal.
  • nodep dependencies: Somewhat of an oxymoron, this is a weaker version of the feature above, where a module does not request a dependency on another module, but if the other module does somehow exist in the dep graph, the original module expects it to have a minimal version spec.
    • This can be useful to split protobuf up into protobuf-core and the various language-specific bindings protobuf-<LANG>. The language bindings need to maintain version parity with the core runtime, but we don't want protobuf-core to depend on all possible protobuf-<LANG> modules (which would defeat the whole purpose of splitting).
    • This is achievable with optional deps, but may be useful to have as a separate feature anyway for maximum clarity/hygiene. (maybe with repo_name=""?)
@Wyverald Wyverald added area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request labels Feb 5, 2025
@Wyverald Wyverald self-assigned this Feb 5, 2025
@Wyverald
Copy link
Member Author

Wyverald commented Feb 5, 2025

cc @meteorcloudy @fmeum for thoughts

copybara-service bot pushed a commit to google/jsinterop-generator that referenced this issue Feb 6, 2025
…to elemental2 and not pollute the dep graph in Bzlmod. This is related to issue bazelbuild/bazel#25214.

PiperOrigin-RevId: 723713089
@fmeum
Copy link
Collaborator

fmeum commented Feb 6, 2025

A new use case was brought up, where a module wishes to open up the visibility of some of its targets to a sibling module.

This should be doable today: Module A can provide a module extension that creates a repo with the "protected" targets and fail if any module not called B uses that extension. I'm using that pattern for the googleapis rules register.

This does require more effort, but for the use case in the linked commit, that would seem reasonable. I'm worried of optional deps becoming too common if it's too easy to add one.

@fmeum
Copy link
Collaborator

fmeum commented Feb 6, 2025

I do think we should add no-dep deps though and also like the repo_name = "" syntax. If we add them, we should backport them to 7.x so that we can start using them earlier (although technically 7.x would support repo_name = "", it just wouldn't benefit from the no-dep part?).

@meteorcloudy
Copy link
Member

meteorcloudy commented Feb 6, 2025

I actually encountered the same issue a few times and had similar ideas. So in general, I agree we should do it.

Maybe we could achieve this by adding one optional argument to bazel_dep and allowing repo_name to be None?

optional = True means this bazel_dep will only take effect if the same module is already introduced by others.
repo_name = None allows the bazel_dep to set a minimal required version of the module without introducing visibility.
Using together, then it's a no-op if the module doesn't appear anywhere else at all, but has to be a minimal version if it does.

I also agree this has to be backported to Bazel 7 so that future modules still work with Bazel 7.x

@Wyverald
Copy link
Member Author

Wyverald commented Feb 6, 2025

A new use case was brought up, where a module wishes to open up the visibility of some of its targets to a sibling module.

This should be doable today: Module A can provide a module extension that creates a repo with the "protected" targets and fail if any module not called B uses that extension. I'm using that pattern for the googleapis rules register.

While this is quite ingenious, I see a couple of problems with it: 1) it requires protected targets to be moved to a repo generated by a module extension, which can be quite the hassle for development (even if you use local_repository, suddenly stuff like //... doesn't work quite the same, etc); 2) it gives visibility on a per-module basis, which is potentially not good enough.

This does require more effort, but for the use case in the linked commit, that would seem reasonable. I'm worried of optional deps becoming too common if it's too easy to add one.

That's a very valid concern. While I wouldn't say it trumps everything else, it does require the use cases to be very convincing. Also it makes it all the more important that we resolve some big questions surrounding optional deps -- in particular, what happens when the dep is not there? It's fine for the visibility use case, since an invalid label in the visibility spec is simply ignored. But what happens to load("@not_there//:foo.bzl", "bar") in a BUILD file? What happens to ext = use_extension("@not_there//:foo.bzl", "bar")?


re nodep deps, I agree with Yun that repo_name = None seems best. It seems that repo_name = None should basically always be used with optional = True, although we don't necessarily need to enforce that.

@fmeum
Copy link
Collaborator

fmeum commented Feb 6, 2025

  1. it requires protected targets to be moved to a repo generated by a module extension, which can be quite the hassle for development (even if you use local_repository, suddenly stuff like //... doesn't work quite the same, etc)

You could keep the protected targets in place, open up their visibility to the extension repo only and then add aliases to the extension repo. Only the consumer would need to change their reference to the extension repo.

  1. it gives visibility on a per-module basis, which is potentially not good enough.

Do you have an example for this? If you trust another module enough to carve out a visibility exception for it, why would you need to further restrict this on a more granular level? After all, that module might as well declare a publicly visible alias for your target to circumvent that. This sounds like it would only come up in a corporate monorepo-turned-polyrepo based on local_path_override, which is not a use case we have been encouraging so far. :-)

@Wyverald
Copy link
Member Author

Wyverald commented Feb 6, 2025

  1. it requires protected targets to be moved to a repo generated by a module extension, which can be quite the hassle for development (even if you use local_repository, suddenly stuff like //... doesn't work quite the same, etc)

You could keep the protected targets in place, open up their visibility to the extension repo only and then add aliases to the extension repo. Only the consumer would need to change their reference to the extension repo.

This gets out of hand fast. If there are multiple targets, you'd need multiple aliases; if entire packages have default_visibility set, then this is no longer even doable.

Either way, this setup suffers from poor usability (again, for a good reason). I'm reminded of the "just write a trivial module extension" advice we had early on, and how use_repo_rule, although an API addition, has been so overwhelmingly popular that we'd definitely call it worthwhile.

  1. it gives visibility on a per-module basis, which is potentially not good enough.

Do you have an example for this? If you trust another module enough to carve out a visibility exception for it, why would you need to further restrict this on a more granular level?

I don't have an example handy. I don't think "trusting another module" necessarily means "trust all of it"; the other module could be published by the same author, and they want granular visibility specs for this link just as they'd want granular visibility specifications for packages inside the module.

After all, that module might as well declare a publicly visible alias for your target to circumvent that.

Well, yes, but the same could be said for another package. If you think of this other module as something the same author controls, it sounds less adversarial.

This sounds like it would only come up in a corporate monorepo-turned-polyrepo based on local_path_override, which is not a use case we have been encouraging so far. :-)

I wouldn't say that's the only use case where this is useful (see above). And also, I don't think we've been discouraging the case, rather that we haven't had a good answer for the visibility problem there.

copybara-service bot pushed a commit to google/j2cl that referenced this issue Feb 6, 2025
…to jsinterop-generator and jsinterop-base and not pollute the dep graph in Bzlmod. This is related to issue bazelbuild/bazel#25214.

PiperOrigin-RevId: 724049143
copybara-service bot pushed a commit to google/jsinterop-generator that referenced this issue Feb 8, 2025
…to elemental2 and not pollute the dep graph in Bzlmod. This is related to issue bazelbuild/bazel#25214.

PiperOrigin-RevId: 724500733
@Wyverald
Copy link
Member Author

small update -- I'm working on a design doc and a proof of concept implementation. Version resolution with optional deps is turning out out to be much more complex than I anticipated. Nevertheless, nodep deps should be fairly good to go. I think we'll probably try to ship nodep deps before optional deps. The doc should be out by tomorrow.

@Wyverald
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request
Projects
None yet
Development

No branches or pull requests

3 participants