-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Consider public dependencies when choosing a version in cargo add (#1… #15966
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
base: master
Are you sure you want to change the base?
Conversation
src/cargo/ops/cargo_add/mod.rs
Outdated
} else { | ||
let latest = | ||
get_latest_dependency(spec, &dependency, honor_rust_version, gctx, registry)?; | ||
|
||
if dependency.name != latest.name { | ||
gctx.shell().warn(format!( | ||
"translating `{}` to `{}`", | ||
dependency.name, latest.name, | ||
))?; | ||
dependency.name = latest.name; // Normalize the name | ||
let (package_set, resolve) = resolve_ws(ws, true)?; | ||
let public_source = if spec | ||
.manifest() | ||
.unstable_features() | ||
.require(Feature::public_dependency()) | ||
.is_ok() | ||
{ | ||
get_public_dependency( | ||
manifest, | ||
ws, | ||
section, | ||
gctx, | ||
&dependency, | ||
package_set, | ||
resolve, | ||
) | ||
} else { | ||
None | ||
}; | ||
if let Some((registry, public_source)) = public_source { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this belong here or in get_existing_dependency
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since fuzzy_lookup calls that several times in a loop it's hard to know where to put the resolve_ws call. Could introduce some state outside the callback but it'll be a bit messy.
The new method get_dependencies doesn't take a dep key but instead returns all deps. We'll sometimes need this in coming patches, and when we don't need it it's easy to have the caller filter.
9ef6002
to
807e114
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By resolving before Adding, the output becomes unclear where the locking ends and the manifest editing begins
Since this is unstable, we could possibly punt on this
let pkg_ids_and_reqs = resolve.deps(dep_pkgid).filter_map(|(id, deps)| { | ||
deps.iter() | ||
.find(|dep| { | ||
dep.is_public() | ||
&& dep.kind() == DepKind::Normal | ||
&& dep.package_name() == dependency.name.as_str() | ||
}) | ||
.map(|dep| (id, dep.version_req().clone())) | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appear to only go 1 deep for public dependencies but they can be recursive
What does this PR try to resolve?
This change considers public dependencies when adding a new dependency. For example, if you depend on
foo
, which depends publicly onbar 1.0
, and you runcargo add bar
, you will now getbar 1.0
even ifbar 2.0
is available.Fixes #13038
How to test and review this PR?
The test suite has been updated with an example scenario. More tests might be warrented to see how this interacts with other features though.