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

fix incorrect resolution of aliased git dependencies #391

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hallettj
Copy link

@hallettj hallettj commented Mar 2, 2025

In a case where there are multiple references to the same package name via git, crate2nix does not correctly match locked packages to dependencies. For example consider these dependencies,

hello = { package = "ludndev-hello-world", git = "https://github.com/ludndev/rustlang-hello-world-lib.git", tag = "v0.1.1" }
hello_v010 = { package = "ludndev-hello-world", git = "https://github.com/ludndev/rustlang-hello-world-lib", tag = "v0.1.0" }

The same dependency is referenced at two different git revisions, each with its own alias. (I have essentially the same thing in a current work project.) The lock file for this project contains two locked packages with IDs ludndev-hello-world 0.1.0 and ludndev-hello-world 0.1.1.

Crate2nix should match the dependency alias hello to ludndev-hello-world 0.1.1, and should match hello_v010 to ludndev-hello-world 0.1.0. But what actually happens is that it arbitrarily picks one locked package to match to both dependencies. This is because currently package resolution is done according to package name and dependency version range. In this case the package names are the same, and for git dependencies the inferred version range is always *.

The result is a rustc command that repeats the same --extern flag twice for one of the aliases, and omits the other alias:

Running rustc --crate-name two_references_to_same_git_dep_with_different_versions src/main.rs --crate-type bin -C linker=/nix/store/90h6k8ylkgn81k10190v5c9ldyjpzgl9-gcc-wrapper-12.3.0/bin/cc -C opt-level=3 -C codegen-units=1 --remap-path-prefix=/build=/ --extern hello=/nix/store/dkg5rb48s0vpz8im127z3pf1kn7g68xn-rust_ludndev-hello-world-0.1.0-lib/lib/libludndev_hello_world-b82a5d83cc.rlib --extern hello=/nix/store/dkg5rb48s0vpz8im127z3pf1kn7g68xn-rust_ludndev-hello-world-0.1.0-lib/lib/libludndev_hello_world-b82a5d83cc.rlib --cfg feature="default" --edition 2021 --out-dir target/bin -L dependency=target/deps --cap-lints allow --color always
                                                                                                                                                                                                                                                              ^^^^^^^^^^^^^^^                                                                     ^^^^^                                                ^^^^^^^^^^^^^^^                                                                     ^^^^^
                                                                                                                                                                                                                                                              first reference                                                                    v0.1.0                                                second reference, same alias                                                        same version

In the above case it picked one alias, hello, included that as an extern twice matched to the wrong locked package version. Then you get an error like this,

error[E0433]: failed to resolve: use of undeclared crate or module `hello_v010`
 --> src/main.rs:3:20
  |
3 |     println!("{}", hello_v010::greet(""));
  |                    ^^^^^^^^^^ use of undeclared crate or module `hello_v010`

Here is what the correct rustc command looks like:

Running rustc --crate-name two_references_to_same_git_dep_with_different_versions src/main.rs --crate-type bin -C linker=/nix/store/90h6k8ylkgn81k10190v5c9ldyjpzgl9-gcc-wrapper-12.3.0/bin/cc -C opt-level=3 -C codegen-units=1 --remap-path-prefix=/build=/ --extern hello_v010=/nix/store/dkg5rb48s0vpz8im127z3pf1kn7g68xn-rust_ludndev-hello-world-0.1.0-lib/lib/libludndev_hello_world-b82a5d83cc.rlib --extern hello=/nix/store/rdv019apn35nr8jvkwq78wclk4d0pz23-rust_ludndev-hello-world-0.1.1-lib/lib/libludndev_hello_world-77ca822745.rlib --cfg feature="default" --edition 2021 --out-dir target/bin -L dependency=target/deps --cap-lints allow --color always
                                                                                                                                                                                                                                                              ^^^^^^^^^^^^^^^^^^^^                                                                     ^^^^^                                                ^^^^^^^^^^^^^^^                                                                     ^^^^^
                                                                                                                                                                                                                                                              alias one                                                                                expected version                                     alias two                                                                           expected version

This PR fixes the problem by resolving packages by matching the package source with the dependency source. Source comparison is only done if resolving by version results in multiple matches.

If there are still multiple matches after filtering by version range and by source I included a panic. I think failing fast is the right outcome since the alternative could be building with the wrong package. I haven't seen any cases where this would actually come up. I could make changes to propagate a Result instead of panicking if that is desired.

The source comparison is a little hacky. It is necessary because the cargo_metadata library doesn't provide a cleaner way to match dependencies to packages in this situation. I believe the cargo library handles this case better. From what I can see cargo supplies a package ID for both Dependency and Package, and that might be an unambiguous option for dependency resolution. So switching to cargo might be a better fix. The downsides are a) that is a much bigger change, and b) cargo is a very large dependency.

@hallettj
Copy link
Author

hallettj commented Mar 2, 2025

Well I said I hadn't seen any cases where that panic could come up, and then I quickly ran into it in the same work project. There are two distinct dependencies with the same package name, one is referenced via git, and the other via path. I'll try to follow up with a fix for that case too.

Edit: I fixed the specific case I ran into of an ambiguity between two dependencies referencing the same package name where one is a git reference, and the other is a path reference. The fix is to require that a dependency without a source is matched with a package that also doesn't have a source - path dependencies don't have a source, but git and registry dependencies do. Unfortunately that does not fix cases where there are multiple path references to packages with the same package name. Cargo_metadata doesn't seem to provide enough information for dependencies to disambiguate that situation. We can see if a later version of cargo_metadata adds more information, like a package ID on the Dependency type. But it might be necessary to switch to a different source of information, like the cargo library, for a more robust solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant