Skip to content

Commit c0306a8

Browse files
alexcrichtonalexcrichton
committed
Avoid updating registry when adding existing deps
Cargo previously erroneously updated the registry whenever a new dependency was added on a crate which already exists in the DAG. This commit fixes this behavior by ensuring that if the new dependency matches a previously locked version it uses that instead. This commit involved a bit of refactoring around this logic to be a bit more clear how the locking and "falling back to the registry" is happening. Closes #2895 Closes #2931
1 parent d8936af commit c0306a8

File tree

3 files changed

+180
-113
lines changed

3 files changed

+180
-113
lines changed

‎src/cargo/core/registry.rs

+53-59
Original file line numberDiff line numberDiff line change
@@ -201,22 +201,24 @@ impl<'cfg> PackageRegistry<'cfg> {
201201
Ok(ret)
202202
}
203203

204-
// This function is used to transform a summary to another locked summary if
205-
// possible. This is where the concept of a lockfile comes into play.
206-
//
207-
// If a summary points at a package id which was previously locked, then we
208-
// override the summary's id itself, as well as all dependencies, to be
209-
// rewritten to the locked versions. This will transform the summary's
210-
// source to a precise source (listed in the locked version) as well as
211-
// transforming all of the dependencies from range requirements on imprecise
212-
// sources to exact requirements on precise sources.
213-
//
214-
// If a summary does not point at a package id which was previously locked,
215-
// we still want to avoid updating as many dependencies as possible to keep
216-
// the graph stable. In this case we map all of the summary's dependencies
217-
// to be rewritten to a locked version wherever possible. If we're unable to
218-
// map a dependency though, we just pass it on through.
219-
fn lock(&self, summary: Summary) -> Summary {
204+
/// This function is used to transform a summary to another locked summary
205+
/// if possible. This is where the concept of a lockfile comes into play.
206+
///
207+
/// If a summary points at a package id which was previously locked, then we
208+
/// override the summary's id itself, as well as all dependencies, to be
209+
/// rewritten to the locked versions. This will transform the summary's
210+
/// source to a precise source (listed in the locked version) as well as
211+
/// transforming all of the dependencies from range requirements on
212+
/// imprecise sources to exact requirements on precise sources.
213+
///
214+
/// If a summary does not point at a package id which was previously locked,
215+
/// or if any dependencies were added and don't have a previously listed
216+
/// version, we still want to avoid updating as many dependencies as
217+
/// possible to keep the graph stable. In this case we map all of the
218+
/// summary's dependencies to be rewritten to a locked version wherever
219+
/// possible. If we're unable to map a dependency though, we just pass it on
220+
/// through.
221+
pub fn lock(&self, summary: Summary) -> Summary {
220222
let pair = self.locked.get(summary.source_id()).and_then(|map| {
221223
map.get(summary.name())
222224
}).and_then(|vec| {
@@ -229,51 +231,43 @@ impl<'cfg> PackageRegistry<'cfg> {
229231
None => summary,
230232
};
231233
summary.map_dependencies(|dep| {
232-
match pair {
233-
// If we've got a known set of overrides for this summary, then
234-
// one of a few cases can arise:
235-
//
236-
// 1. We have a lock entry for this dependency from the same
237-
// source as it's listed as coming from. In this case we make
238-
// sure to lock to precisely the given package id.
239-
//
240-
// 2. We have a lock entry for this dependency, but it's from a
241-
// different source than what's listed, or the version
242-
// requirement has changed. In this case we must discard the
243-
// locked version because the dependency needs to be
244-
// re-resolved.
245-
//
246-
// 3. We don't have a lock entry for this dependency, in which
247-
// case it was likely an optional dependency which wasn't
248-
// included previously so we just pass it through anyway.
249-
Some(&(_, ref deps)) => {
250-
match deps.iter().find(|d| d.name() == dep.name()) {
251-
Some(lock) => {
252-
if dep.matches_id(lock) {
253-
dep.lock_to(lock)
254-
} else {
255-
dep
256-
}
257-
}
258-
None => dep,
259-
}
234+
// If we've got a known set of overrides for this summary, then
235+
// one of a few cases can arise:
236+
//
237+
// 1. We have a lock entry for this dependency from the same
238+
// source as it's listed as coming from. In this case we make
239+
// sure to lock to precisely the given package id.
240+
//
241+
// 2. We have a lock entry for this dependency, but it's from a
242+
// different source than what's listed, or the version
243+
// requirement has changed. In this case we must discard the
244+
// locked version because the dependency needs to be
245+
// re-resolved.
246+
//
247+
// 3. We don't have a lock entry for this dependency, in which
248+
// case it was likely an optional dependency which wasn't
249+
// included previously so we just pass it through anyway.
250+
//
251+
// Cases 1/2 are handled by `matches_id` and case 3 is handled by
252+
// falling through to the logic below.
253+
if let Some(&(_, ref locked_deps)) = pair {
254+
let locked = locked_deps.iter().find(|id| dep.matches_id(id));
255+
if let Some(locked) = locked {
256+
return dep.lock_to(locked)
260257
}
258+
}
261259

262-
// If this summary did not have a locked version, then we query
263-
// all known locked packages to see if they match this
264-
// dependency. If anything does then we lock it to that and move
265-
// on.
266-
None => {
267-
let v = self.locked.get(dep.source_id()).and_then(|map| {
268-
map.get(dep.name())
269-
}).and_then(|vec| {
270-
vec.iter().find(|&&(ref id, _)| dep.matches_id(id))
271-
});
272-
match v {
273-
Some(&(ref id, _)) => dep.lock_to(id),
274-
None => dep
275-
}
276-
}
260+
// If this dependency did not have a locked version, then we query
261+
// all known locked packages to see if they match this dependency.
262+
// If anything does then we lock it to that and move on.
263+
let v = self.locked.get(dep.source_id()).and_then(|map| {
264+
map.get(dep.name())
265+
}).and_then(|vec| {
266+
vec.iter().find(|&&(ref id, _)| dep.matches_id(id))
267+
});
268+
match v {
269+
Some(&(ref id, _)) => dep.lock_to(id),
270+
None => dep
277271
}
278272
})
279273
}

‎src/cargo/ops/resolve.rs

+32-54
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use std::collections::{HashMap, HashSet};
1+
use std::collections::HashSet;
22

33
use core::{PackageId, PackageIdSpec, SourceId, Workspace};
44
use core::registry::PackageRegistry;
@@ -55,6 +55,36 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
5555
.filter(|s| !s.is_registry()));
5656
}
5757

58+
// In the case where a previous instance of resolve is available, we
59+
// want to lock as many packages as possible to the previous version
60+
// without disturbing the graph structure. To this end we perform
61+
// two actions here:
62+
//
63+
// 1. We inform the package registry of all locked packages. This
64+
// involves informing it of both the locked package's id as well
65+
// as the versions of all locked dependencies. The registry will
66+
// then takes this information into account when it is queried.
67+
//
68+
// 2. The specified package's summary will have its dependencies
69+
// modified to their precise variants. This will instruct the
70+
// first step of the resolution process to not query for ranges
71+
// but rather for precise dependency versions.
72+
//
73+
// This process must handle altered dependencies, however, as
74+
// it's possible for a manifest to change over time to have
75+
// dependencies added, removed, or modified to different version
76+
// ranges. To deal with this, we only actually lock a dependency
77+
// to the previously resolved version if the dependency listed
78+
// still matches the locked version.
79+
if let Some(r) = previous {
80+
for node in r.iter().filter(|p| keep(p, to_avoid, &to_avoid_sources)) {
81+
let deps = r.deps_not_replaced(node)
82+
.filter(|p| keep(p, to_avoid, &to_avoid_sources))
83+
.cloned().collect();
84+
registry.register_lock(node.clone(), deps);
85+
}
86+
}
87+
5888
let mut summaries = Vec::new();
5989
for member in ws.members() {
6090
try!(registry.add_sources(&[member.package_id().source_id()
@@ -75,59 +105,7 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
75105
}
76106
}
77107

78-
// If we don't have a previous instance of resolve then we just need to
79-
// resolve our entire summary (method should be Everything) and we just
80-
// move along to the next member.
81-
let r = match previous {
82-
Some(r) => r,
83-
None => {
84-
summaries.push((member.summary().clone(), method));
85-
continue
86-
}
87-
};
88-
89-
// In the case where a previous instance of resolve is available, we
90-
// want to lock as many packages as possible to the previous version
91-
// without disturbing the graph structure. To this end we perform
92-
// two actions here:
93-
//
94-
// 1. We inform the package registry of all locked packages. This
95-
// involves informing it of both the locked package's id as well
96-
// as the versions of all locked dependencies. The registry will
97-
// then takes this information into account when it is queried.
98-
//
99-
// 2. The specified package's summary will have its dependencies
100-
// modified to their precise variants. This will instruct the
101-
// first step of the resolution process to not query for ranges
102-
// but rather for precise dependency versions.
103-
//
104-
// This process must handle altered dependencies, however, as
105-
// it's possible for a manifest to change over time to have
106-
// dependencies added, removed, or modified to different version
107-
// ranges. To deal with this, we only actually lock a dependency
108-
// to the previously resolved version if the dependency listed
109-
// still matches the locked version.
110-
for node in r.iter().filter(|p| keep(p, to_avoid, &to_avoid_sources)) {
111-
let deps = r.deps_not_replaced(node)
112-
.filter(|p| keep(p, to_avoid, &to_avoid_sources))
113-
.cloned().collect();
114-
registry.register_lock(node.clone(), deps);
115-
}
116-
117-
let summary = {
118-
let map = r.deps_not_replaced(member.package_id()).filter(|p| {
119-
keep(p, to_avoid, &to_avoid_sources)
120-
}).map(|d| {
121-
(d.name(), d)
122-
}).collect::<HashMap<_, _>>();
123-
124-
member.summary().clone().map_dependencies(|dep| {
125-
match map.get(dep.name()) {
126-
Some(&lock) if dep.matches_id(lock) => dep.lock_to(lock),
127-
_ => dep,
128-
}
129-
})
130-
};
108+
let summary = registry.lock(member.summary().clone());
131109
summaries.push((summary, method));
132110
}
133111

‎tests/registry.rs

+95
Original file line numberDiff line numberDiff line change
@@ -1147,3 +1147,98 @@ Caused by:
11471147
attempting to make an HTTP request, but --frozen was specified
11481148
"));
11491149
}
1150+
1151+
#[test]
1152+
fn add_dep_dont_update_registry() {
1153+
let p = project("foo")
1154+
.file("Cargo.toml", r#"
1155+
[project]
1156+
name = "bar"
1157+
version = "0.5.0"
1158+
authors = []
1159+
1160+
[dependencies]
1161+
baz = { path = "baz" }
1162+
"#)
1163+
.file("src/main.rs", "fn main() {}")
1164+
.file("baz/Cargo.toml", r#"
1165+
[project]
1166+
name = "baz"
1167+
version = "0.5.0"
1168+
authors = []
1169+
1170+
[dependencies]
1171+
remote = "0.3"
1172+
"#)
1173+
.file("baz/src/lib.rs", "");
1174+
p.build();
1175+
1176+
Package::new("remote", "0.3.4").publish();
1177+
1178+
assert_that(p.cargo("build"), execs().with_status(0));
1179+
1180+
t!(t!(File::create(p.root().join("Cargo.toml"))).write_all(br#"
1181+
[project]
1182+
name = "bar"
1183+
version = "0.5.0"
1184+
authors = []
1185+
1186+
[dependencies]
1187+
baz = { path = "baz" }
1188+
remote = "0.3"
1189+
"#));
1190+
1191+
assert_that(p.cargo("build"),
1192+
execs().with_status(0)
1193+
.with_stderr("\
1194+
[COMPILING] bar v0.5.0 ([..])
1195+
[FINISHED] [..]
1196+
"));
1197+
}
1198+
1199+
#[test]
1200+
fn bump_version_dont_update_registry() {
1201+
let p = project("foo")
1202+
.file("Cargo.toml", r#"
1203+
[project]
1204+
name = "bar"
1205+
version = "0.5.0"
1206+
authors = []
1207+
1208+
[dependencies]
1209+
baz = { path = "baz" }
1210+
"#)
1211+
.file("src/main.rs", "fn main() {}")
1212+
.file("baz/Cargo.toml", r#"
1213+
[project]
1214+
name = "baz"
1215+
version = "0.5.0"
1216+
authors = []
1217+
1218+
[dependencies]
1219+
remote = "0.3"
1220+
"#)
1221+
.file("baz/src/lib.rs", "");
1222+
p.build();
1223+
1224+
Package::new("remote", "0.3.4").publish();
1225+
1226+
assert_that(p.cargo("build"), execs().with_status(0));
1227+
1228+
t!(t!(File::create(p.root().join("Cargo.toml"))).write_all(br#"
1229+
[project]
1230+
name = "bar"
1231+
version = "0.6.0"
1232+
authors = []
1233+
1234+
[dependencies]
1235+
baz = { path = "baz" }
1236+
"#));
1237+
1238+
assert_that(p.cargo("build"),
1239+
execs().with_status(0)
1240+
.with_stderr("\
1241+
[COMPILING] bar v0.6.0 ([..])
1242+
[FINISHED] [..]
1243+
"));
1244+
}

0 commit comments

Comments
 (0)