Skip to content

Commit c62be1e

Browse files
awelcawelc
authored
[move-package] Support dependency overrides (#11181)
## Description When building a dependency graph, different versions of the same (transitively) dependent package can be encountered. If this is indeed the case, a single version must be chosen by the developer to be the override, and this override must be specified in a manifest file whose package dominates all the conflicting "uses" of the dependent package. These overrides must taken into consideration during the dependency graph construction and this PR implements the relevant changes to dependency graph construction algorithm. For additional details see the doc-comments in the code as well as the PR this one is based on (move-language/move#1023) which contains a discussion of issues encountered during development of this algorithm. ## Test Plan A comprehensive test suite is attached. --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [x] user-visible impact ### Release notes This change allows a developer to override transitive dependencies of a package they are developing to avoid conflicts that could otherwise arise.
1 parent 8cadd7b commit c62be1e

File tree

164 files changed

+4833
-160
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

164 files changed

+4833
-160
lines changed

‎tools/move-package/src/resolution/dependency_graph.rs

+475-111
Large diffs are not rendered by default.

‎tools/move-package/src/source_package/manifest_parser.rs

+17-1
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,11 @@ pub fn parse_dependency(dep_name: &str, mut tval: TV) -> Result<PM::Dependency>
334334
.transpose()?;
335335
let version = table.remove("version").map(parse_version).transpose()?;
336336
let digest = table.remove("digest").map(parse_digest).transpose()?;
337+
let dep_override = table
338+
.remove("override")
339+
.map(parse_dep_override)
340+
.transpose()?
341+
.map_or(false, |o| o);
337342

338343
let kind = match (
339344
table.remove("local"),
@@ -350,7 +355,10 @@ pub fn parse_dependency(dep_name: &str, mut tval: TV) -> Result<PM::Dependency>
350355
bail!("Local source path not a string")
351356
};
352357

353-
PM::DependencyKind::Local(local)
358+
PM::DependencyKind::Local(
359+
// with allow_cwd_parent set to true, it never fails
360+
PM::normalize_path(local, true /* allow_cwd_parent */).unwrap(),
361+
)
354362
}
355363

356364
(None, subdir, Some(git_url), None) => {
@@ -433,6 +441,7 @@ pub fn parse_dependency(dep_name: &str, mut tval: TV) -> Result<PM::Dependency>
433441
subst,
434442
version,
435443
digest,
444+
dep_override,
436445
}))
437446
}
438447

@@ -502,6 +511,13 @@ fn parse_digest(tval: TV) -> Result<PM::PackageDigest> {
502511
Ok(PM::PackageDigest::from(digest_str))
503512
}
504513

514+
fn parse_dep_override(tval: TV) -> Result<PM::DepOverride> {
515+
if !tval.is_bool() {
516+
bail!("Invalid dependency override value");
517+
}
518+
Ok(tval.as_bool().unwrap())
519+
}
520+
505521
// check that only recognized names are provided at the top-level
506522
fn warn_if_unknown_field_names(table: &toml::map::Map<String, TV>, known_names: &[&str]) {
507523
let mut unknown_names = BTreeSet::new();

‎tools/move-package/src/source_package/parsed_manifest.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ pub type NamedAddress = Symbol;
1616
pub type PackageName = Symbol;
1717
pub type FileName = Symbol;
1818
pub type PackageDigest = Symbol;
19+
pub type DepOverride = bool;
1920

2021
pub type AddressDeclarations = BTreeMap<NamedAddress, Option<AccountAddress>>;
2122
pub type DevAddressDeclarations = BTreeMap<NamedAddress, AccountAddress>;
@@ -55,6 +56,7 @@ pub struct InternalDependency {
5556
pub subst: Option<Substitution>,
5657
pub version: Option<Version>,
5758
pub digest: Option<PackageDigest>,
59+
pub dep_override: DepOverride,
5860
}
5961

6062
#[derive(Debug, Clone, Eq, PartialEq)]
@@ -154,7 +156,7 @@ impl Default for DependencyKind {
154156
/// or is prefixed by accesses to parent directories when `allow_cwd_parent` is false.
155157
///
156158
/// Returns the normalized path on success.
157-
fn normalize_path(path: impl AsRef<Path>, allow_cwd_parent: bool) -> Result<PathBuf> {
159+
pub fn normalize_path(path: impl AsRef<Path>, allow_cwd_parent: bool) -> Result<PathBuf> {
158160
use Component::*;
159161

160162
let mut stack = Vec::new();

‎tools/move-package/tests/test_dependency_graph.rs

+32-8
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// SPDX-License-Identifier: Apache-2.0
33

44
use std::{
5-
collections::BTreeSet,
5+
collections::{BTreeMap, BTreeSet},
66
fs::{self, File},
77
io::Write,
88
path::PathBuf,
@@ -184,7 +184,15 @@ fn merge_simple() {
184184
)
185185
.expect("Reading inner");
186186

187-
assert!(outer.merge(inner, Symbol::from("")).is_ok());
187+
assert!(outer
188+
.merge(
189+
Symbol::from("A"),
190+
Symbol::from("A"),
191+
inner,
192+
Symbol::from(""),
193+
&BTreeMap::new(),
194+
)
195+
.is_ok());
188196

189197
assert_eq!(
190198
outer.topological_order(),
@@ -214,7 +222,15 @@ fn merge_into_root() {
214222
)
215223
.expect("Reading inner");
216224

217-
assert!(outer.merge(inner, Symbol::from("")).is_ok());
225+
assert!(outer
226+
.merge(
227+
Symbol::from("Root"),
228+
Symbol::from("A"),
229+
inner,
230+
Symbol::from(""),
231+
&BTreeMap::new(),
232+
)
233+
.is_ok());
218234

219235
assert_eq!(
220236
outer.topological_order(),
@@ -243,7 +259,7 @@ fn merge_detached() {
243259
)
244260
.expect("Reading inner");
245261

246-
let Err(err) = outer.merge(inner, Symbol::from("")) else {
262+
let Err(err) = outer.merge(Symbol::from("OtherDep"), Symbol::from("A"), inner, Symbol::from(""), &BTreeMap::new()) else {
247263
panic!("Inner's root is not part of outer's graph, so this should fail");
248264
};
249265

@@ -267,7 +283,7 @@ fn merge_after_calculating_always_deps() {
267283
)
268284
.expect("Reading inner");
269285

270-
let Err(err) = outer.merge(inner, Symbol::from("")) else {
286+
let Err(err) = outer.merge(Symbol::from("A"),Symbol::from("A"), inner, Symbol::from(""), &BTreeMap::new()) else {
271287
panic!("Outer's always deps have already been calculated so this should fail");
272288
};
273289

@@ -295,7 +311,15 @@ fn merge_overlapping() {
295311
)
296312
.expect("Reading inner");
297313

298-
assert!(outer.merge(inner, Symbol::from("")).is_ok());
314+
assert!(outer
315+
.merge(
316+
Symbol::from("B"),
317+
Symbol::from("A"),
318+
inner,
319+
Symbol::from(""),
320+
&BTreeMap::new(),
321+
)
322+
.is_ok());
299323
}
300324

301325
#[test]
@@ -319,7 +343,7 @@ fn merge_overlapping_different_deps() {
319343
)
320344
.expect("Reading inner");
321345

322-
let Err(err) = outer.merge(inner, Symbol::from("")) else {
346+
let Err(err) = outer.merge(Symbol::from("B"),Symbol::from("A"), inner, Symbol::from(""), &BTreeMap::new()) else {
323347
panic!("Outer and inner mention package A which has different dependencies in both.");
324348
};
325349

@@ -347,7 +371,7 @@ fn merge_cyclic() {
347371
)
348372
.expect("Reading inner");
349373

350-
let Err(err) = outer.merge(inner, Symbol::from("")) else {
374+
let Err(err) = outer.merge(Symbol::from("B"), Symbol::from("Root"), inner, Symbol::from(""), &BTreeMap::new()) else {
351375
panic!("Inner refers back to outer's root");
352376
};
353377

‎tools/move-package/tests/test_sources/dep_dev_dep_diamond/Move.resolved

+13-3
Original file line numberDiff line numberDiff line change
@@ -69,27 +69,31 @@ ResolvedGraph {
6969
),
7070
version: None,
7171
resolver: None,
72+
overridden_path: false,
7273
},
7374
"B": Package {
7475
kind: Local(
7576
"deps_only/B",
7677
),
7778
version: None,
7879
resolver: None,
80+
overridden_path: false,
7981
},
8082
"C": Package {
8183
kind: Local(
8284
"deps_only/C",
8385
),
8486
version: None,
8587
resolver: None,
88+
overridden_path: false,
8689
},
8790
"D": Package {
8891
kind: Local(
8992
"deps_only/D",
9093
),
9194
version: None,
9295
resolver: None,
96+
overridden_path: false,
9397
},
9498
},
9599
always_deps: {
@@ -142,6 +146,7 @@ ResolvedGraph {
142146
subst: None,
143147
version: None,
144148
digest: None,
149+
dep_override: false,
145150
},
146151
),
147152
},
@@ -154,6 +159,7 @@ ResolvedGraph {
154159
subst: None,
155160
version: None,
156161
digest: None,
162+
dep_override: false,
157163
},
158164
),
159165
},
@@ -189,6 +195,7 @@ ResolvedGraph {
189195
subst: None,
190196
version: None,
191197
digest: None,
198+
dep_override: false,
192199
},
193200
),
194201
},
@@ -266,33 +273,36 @@ ResolvedGraph {
266273
"A": Internal(
267274
InternalDependency {
268275
kind: Local(
269-
"./deps_only/A",
276+
"deps_only/A",
270277
),
271278
subst: None,
272279
version: None,
273280
digest: None,
281+
dep_override: false,
274282
},
275283
),
276284
"C": Internal(
277285
InternalDependency {
278286
kind: Local(
279-
"./deps_only/C",
287+
"deps_only/C",
280288
),
281289
subst: None,
282290
version: None,
283291
digest: None,
292+
dep_override: false,
284293
},
285294
),
286295
},
287296
dev_dependencies: {
288297
"B": Internal(
289298
InternalDependency {
290299
kind: Local(
291-
"./deps_only/B",
300+
"deps_only/B",
292301
),
293302
subst: None,
294303
version: None,
295304
digest: None,
305+
dep_override: false,
296306
},
297307
),
298308
},

‎tools/move-package/tests/test_sources/dep_good_digest/Move.resolved

+3-1
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ ResolvedGraph {
2323
),
2424
version: None,
2525
resolver: None,
26+
overridden_path: false,
2627
},
2728
},
2829
always_deps: {
@@ -104,7 +105,7 @@ ResolvedGraph {
104105
"OtherDep": Internal(
105106
InternalDependency {
106107
kind: Local(
107-
"./deps_only/other_dep",
108+
"deps_only/other_dep",
108109
),
109110
subst: Some(
110111
{
@@ -117,6 +118,7 @@ ResolvedGraph {
117118
digest: Some(
118119
"6A88B7888D6049EB0121900E22B6FA2C0E702F042C8C8D4FD62AD5C990B9F9A8",
119120
),
121+
dep_override: false,
120122
},
121123
),
122124
},

‎tools/move-package/tests/test_sources/diamond_problem_backflow_resolution/Move.resolved

+9-2
Original file line numberDiff line numberDiff line change
@@ -51,20 +51,23 @@ ResolvedGraph {
5151
),
5252
version: None,
5353
resolver: None,
54+
overridden_path: false,
5455
},
5556
"B": Package {
5657
kind: Local(
5758
"deps_only/B",
5859
),
5960
version: None,
6061
resolver: None,
62+
overridden_path: false,
6163
},
6264
"C": Package {
6365
kind: Local(
6466
"deps_only/C",
6567
),
6668
version: None,
6769
resolver: None,
70+
overridden_path: false,
6871
},
6972
},
7073
always_deps: {
@@ -123,6 +126,7 @@ ResolvedGraph {
123126
),
124127
version: None,
125128
digest: None,
129+
dep_override: false,
126130
},
127131
),
128132
},
@@ -171,6 +175,7 @@ ResolvedGraph {
171175
),
172176
version: None,
173177
digest: None,
178+
dep_override: false,
174179
},
175180
),
176181
},
@@ -238,17 +243,18 @@ ResolvedGraph {
238243
"A": Internal(
239244
InternalDependency {
240245
kind: Local(
241-
"./deps_only/A",
246+
"deps_only/A",
242247
),
243248
subst: None,
244249
version: None,
245250
digest: None,
251+
dep_override: false,
246252
},
247253
),
248254
"B": Internal(
249255
InternalDependency {
250256
kind: Local(
251-
"./deps_only/B",
257+
"deps_only/B",
252258
),
253259
subst: Some(
254260
{
@@ -259,6 +265,7 @@ ResolvedGraph {
259265
),
260266
version: None,
261267
digest: None,
268+
dep_override: false,
262269
},
263270
),
264271
},

‎tools/move-package/tests/test_sources/diamond_problem_dep_conflict/Move.resolved

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Failed to resolve dependencies for package 'Root': Resolving dependencies for package 'B': Conflicting dependencies found:
2+
C = { local = "deps_only/C", version = "2.0.0" }
3+
C = { local = "deps_only/C", version = "1.0.0" }

‎tools/move-package/tests/test_sources/diamond_problem_dep_conflict/Move.toml

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
[package]
2+
name = "Root"
3+
version = "0.0.0"
4+
5+
[dependencies]
6+
A = { local = "./deps_only/A" }
7+
B = { local = "./deps_only/B" }

‎tools/move-package/tests/test_sources/diamond_problem_dep_conflict/deps_only/A/Move.toml

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
[package]
2+
name = "A"
3+
version = "0.0.0"
4+
5+
[dependencies]
6+
C = { local = "../C", version = "2.0.0" }

‎tools/move-package/tests/test_sources/diamond_problem_dep_conflict/deps_only/B/Move.toml

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
[package]
2+
name = "B"
3+
version = "0.0.0"
4+
5+
[dependencies]
6+
C = { local = "../C", version = "1.0.0" }

‎tools/move-package/tests/test_sources/diamond_problem_dep_conflict/deps_only/C/Move.toml

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
[package]
2+
name = "C"
3+
version = "0.0.0"

‎tools/move-package/tests/test_sources/diamond_problem_dep_external_conflict/Move.resolved

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Failed to resolve dependencies for package 'Root': Adding dependencies from ../resolvers/successful.sh for dependency 'A' in 'Root': Conflicting dependencies found:
2+
ADep = { local = "deps_only/ADep", version = "1.0.0" }
3+
ADep = { local = "deps_only/ADep" } # Resolved by ../resolvers/successful.sh

0 commit comments

Comments
 (0)