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

Undetected cyclic dependency between module extensions #17564

Closed
aherrmann opened this issue Feb 23, 2023 · 2 comments
Closed

Undetected cyclic dependency between module extensions #17564

aherrmann opened this issue Feb 23, 2023 · 2 comments
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug untriaged

Comments

@aherrmann
Copy link
Contributor

Description of the bug:

It's possible to construct a cyclic dependency between bzlmod module extensions that goes undetected by Bazel but instead causes tags defined in MODULE.bazel to be ignored.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Check out https://github.com/aherrmann/repro-bzlmod-module-extension-cycle (66f4ab2 at the time of writing) and follow the steps in the README.

In that example module_a defines two extensions extension_1 and extension_2 in separate files extension_1.bzl and extension_2.bzl. The file implementing extension_2 loads a file that is generated by a repository rule that extension_1 invokes. Things work well so long as each extension is loaded (use_extension) from its dedicated file.

However, module_a includes extensions.bzl which re-exports both extensions. Trying to use this file to load the extensions from module_a yields an error message about cyclic dependencies as expected. However, loading this file from module_b raises no such error and instead causes the tags of extension_1 defined in module_b to be ignored.

Which operating system are you running Bazel on?

Ubuntu 22.04

What is the output of bazel info release?

release 6.0.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

Related to the more general question of whether module extensions can depend on files generated by other module extensions.

@sgowroji sgowroji added type: bug team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. untriaged area-Bzlmod Bzlmod-specific PRs, issues, and feature requests labels Feb 24, 2023
@Wyverald
Copy link
Member

This is currently WAI, due to the fact that re-exporting an extension makes it a distinct extension.

The dependency graph (if you do the "Faulty behavior" part of your README) looks like this: (if you s/extension/ext)

EXT[@module_a//:exts.bzl%ext_1]    EXT[@module_a//:exts.bzl%ext_2]
                      V             V
                  FILE[@module_a//:exts.bzl]
                   V                V
FILE[@module_a//:ext_1.bzl]        FILE[@module_a//:ext_2.bzl]
                 ^                  V
                 |               FILE[@[@ext_1 from @module_a]//:defs.bzl] (*)
                 |                  V
              EXT[@module_a//:ext_1.bzl%ext_1]

And there are no cycles in this graph.

The node marked with (*) is telling: @module_a//:ext_2.bzl has a load("@ext_1//:defs.bzl", ...), in which the @ext_1 uses the definition from module_a instead of module_b (since @module_a//:ext_2.bzl is hosted in module_a). So @ext_1 actually ends up referring to the one in @module_a//:ext_1.bzl (bottom node), not @module_a//:exts.bzl (top-left node).

The missing tags thing is also WAI, since EXT[@module_a//:exts.bzl%ext_1] and EXT[@module_a//:ext_1.bzl%ext_1] are distinct extensions: the former used by module_b, and the latter used by module_a.


The question is then -- why does re-exporting an extension make it a distinct extension? The answer is a bit tricky... Right now, extensions are identified solely through the .bzl file they're defined in, and the name of the variable they're assigned to. If we wanted to ID an extension based on reference equality, we'd need to load the .bzl files defining them, and evaluate those .bzl files.

We need to ID the extensions in order to figure out the repo mapping; that is, use_repo(ext, "repo") is translated to a repo mapping entry @repo for @@module~1.2 is @@ext_module~3.4~ext~repo. To do this we need to know the "canonical location" of the definition of ext, which means that we'd need to fetch ext_module@3.4 (if extension ID is reference equality), or at least finish module version resolution so we know to use version 3.4 for ext_module (if extension ID is location+name).

And we need the repo mapping to parse any labels (this means evaluating any BUILD files, parsing label-typed command-line flags, etc).

All this together means that:

  • Extension ID is location+name (status quo): we can start parsing labels as soon as module version resolution finishes.
  • Extension ID is reference equality: we can start parsing labels after fetching all modules that host any extension used by any module in the dependency graph, and evaluating some .bzl files in those modules.

Given the above, and the fact that "re-exporting an extension makes it a distinct extension" shouldn't really be harmful in practice, I'm going to close this as WAI.

@Wyverald Wyverald closed this as not planned Won't fix, can't repro, duplicate, stale Feb 28, 2023
@aherrmann
Copy link
Contributor Author

aherrmann commented Feb 28, 2023

@Wyverald Big thank you for the in depth answer!
I didn't realize that the ID was location + name, thanks for clarifying and motivating that fact.
UPDATE: I've opened a PR to extend the documentation in #17634.

aherrmann added a commit to aherrmann/bazel that referenced this issue Mar 1, 2023
Closes bazelbuild#17633

Adds a section to the module extension documentation about the identity
of module extensions as described in
bazelbuild#17564 (comment).
copybara-service bot pushed a commit that referenced this issue Mar 1, 2023
Closes #17633

Adds a section to the module extension documentation about the identity of module extensions as described in #17564 (comment).

Closes #17634.

PiperOrigin-RevId: 513206202
Change-Id: I8cdc8ef836b0a119911b04aa7efb4f9882c72f54
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
Closes bazelbuild#17633

Adds a section to the module extension documentation about the identity of module extensions as described in bazelbuild#17564 (comment).

Closes bazelbuild#17634.

PiperOrigin-RevId: 513206202
Change-Id: I8cdc8ef836b0a119911b04aa7efb4f9882c72f54
Wyverald pushed a commit that referenced this issue Jul 12, 2023
Closes #17633

Adds a section to the module extension documentation about the identity of module extensions as described in #17564 (comment).

Closes #17634.

PiperOrigin-RevId: 513206202
Change-Id: I8cdc8ef836b0a119911b04aa7efb4f9882c72f54
Wyverald added a commit that referenced this issue Jul 12, 2023
* ModqueryExecutor output logic

- `show` implementation added (mostly uses `Query`'s `TargetOutputter` which is now publicly exposed instead of package-private).

- `text`, `json` and `graphviz dot` outputters added

- Unit testing for `ModqueryExecutor`output logic

#15365

PiperOrigin-RevId: 542325901
Change-Id: I155a326465355432fbb4436b28aecc0697c3ffab

* Add module extensions to modquery

- Now includes extension usages and repositories inside query graphs along related options
- `show` now supports extension-generated repos
- Added new subcommand `show_extension` which displays the list of repos generated by that extension and its usages by each module
- Since this CL introduces a new argument type to modquery (`<extension>`), refactored modquery argument parsing logic (see `ModuleArg` and `ExtensionArg`). For a user-friendly description, see the `modquery.txt` file.
- Added some basic Python integration tests for modquery (more to come).

#15365

Co-authored-by: Xùdōng Yáng <wyverald@gmail.com>
PiperOrigin-RevId: 547524086
Change-Id: If1364f01c3be871343edcd5cee94b1180b4b930f

* New documentation for external deps
Rewrote the "external dependencies" and "Bzlmod" pages, organizing them into a subcategory of pages on external dependencies instead.

PiperOrigin-RevId: 506646517
Change-Id: Ib3f1d6fb8c33c06e723aeef2eb0a5b7a223cc487

* Document module extension identity

Closes #17633

Adds a section to the module extension documentation about the identity of module extensions as described in #17564 (comment).

Closes #17634.

PiperOrigin-RevId: 513206202
Change-Id: I8cdc8ef836b0a119911b04aa7efb4f9882c72f54

* Minor verbiage changes about MVS + bzlmod

This paragraph was a bit misleading, particularly if you are skimming the docs rather than reading top to bottom. Out of context, it sounds like coexistence of multiple major versions of the same module works, when really the next paragraph clarifies that its the opposite.

I tweaked things a tad to make it comparison to Go a bit more obvious at the start of the paragraph.

I think it might be worth reworking this entire page to be less dependent on Go semantics and simple explain MVS + bzlmod semantics for bzlmod directly (I say this as a bazel user without a ton of go modules experience, so the comparisons aren't useful for all audiences).

Closes #17766.

PiperOrigin-RevId: 516532030
Change-Id: Ie8a327511c9c26697b2f45501ebf6bbc2b0e2944

* add "archiveType" source.json property to set the http_archive's "typ…

…e" value.

Closes #17789.

PiperOrigin-RevId: 517408668
Change-Id: I7f89db0d2587cde3ff9d77c8657d162981cf32dc

* Add a best practices section for extensions

PiperOrigin-RevId: 528789449
Change-Id: I1d1e57493fee0e805d953178bd18679fd7e040e4

* Allow overrides in non-root modules

They're simply ignored.

RELNOTES: Overrides specified by non-root modules no longer cause an error, and are silently ignored instead. They were originally treated as an error to allow for the future possibility of overrides in the transitive dependency graph working together; but we've deemed that infeasible (and even if it was, it'd be so complicated and confusing to users that it would not be a good addition).
PiperOrigin-RevId: 529095596
Change-Id: I8b9b7b570b405ee757554accf791d8e4c1ff6528

* Add links to external useful Bzlmod doc and talk

PiperOrigin-RevId: 529706168
Change-Id: Iece19039f41bdc80f4acd2621e0d1f5b2ce27ed0

* Add Bzlmod Migration Guide to bazel.build

Based on https://docs.google.com/document/d/1JtXIVnXyFZ4bmbiBCr5gsTH4-opZAFf5DMMb-54kES0/edit#heading=h.5mcn15i0e1ch, but with more code snippets as examples and re-organized the guide structure.

PiperOrigin-RevId: 543742388
Change-Id: If77ff96b7686f206dd09f5c2453151dee149b087

* `mod` command docs

#15365

Co-authored-by: Xùdōng Yáng <wyverald@gmail.com>
PiperOrigin-RevId: 547554906
Change-Id: I02ab16fe4044888bb13b652e2e75dbfa4f55ead1

* Rename `modquery` to `mod`

- Since we're considering adding more subcommands that don't exactly "query", such as `bazel mod upgrade` etc.
- `bazel modquery show` is renamed to `bazel mod show_repo` (`show_extension` is unchanged)
- `bazel modquery tree` is renamed to `bazel mod graph`.

#15365

Co-authored-by: Xùdōng Yáng <wyverald@gmail.com>
PiperOrigin-RevId: 547553222
Change-Id: I74145fdb87c05761692391e6ba47ce8d975f90a9

* fix test

---------

Co-authored-by: andyrinne12 <andreisolo@google.com>
Co-authored-by: andyrinne12 <andysolo2000@gmail.com>
Co-authored-by: Andreas Herrmann <andreas.herrmann@tweag.io>
Co-authored-by: Andy Hamon <and.ham95@gmail.com>
Co-authored-by: Jon Landis <jjjhhhlll@gmail.com>
Co-authored-by: Richard Levasseur <rlevasseur@google.com>
Co-authored-by: Googler <pcloudy@google.com>
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 team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug untriaged
Projects
None yet
Development

No branches or pull requests

3 participants