Skip to content
This repository has been archived by the owner on May 4, 2024. It is now read-only.

[move-package] Support dependency overrides #1023

Open
wants to merge 10 commits into
base: sui-move
Choose a base branch
from
Open

[move-package] Support dependency overrides #1023

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0eeded6
Added dep override parsing and local path normalization
awelc Mar 28, 2023
6312772
[move-package] Support dependency overrides
awelc Mar 30, 2023
9b461a3
Fixed a linting problem
awelc Mar 30, 2023
b9870ed
Addressing review comments - round one
awelc Apr 17, 2023
aafc655
Addressing review comments - round two
awelc Apr 20, 2023
2ddd632
Refactoring and documentation updates
awelc Apr 20, 2023
2672c71
Finessed some comments
awelc Apr 21, 2023
97267a8
Added more tests and updated comments
awelc Apr 21, 2023
371296b
Removed accidentally added temp files
awelc Apr 21, 2023
fa8a38f
Fixed linting errors
awelc Apr 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
581 changes: 470 additions & 111 deletions language/tools/move-package/src/resolution/dependency_graph.rs
View file
Open in desktop

Large diffs are not rendered by default.

18 changes: 17 additions & 1 deletion language/tools/move-package/src/source_package/manifest_parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,11 @@ pub fn parse_dependency(dep_name: &str, mut tval: TV) -> Result<PM::Dependency>
.transpose()?;
let version = table.remove("version").map(parse_version).transpose()?;
let digest = table.remove("digest").map(parse_digest).transpose()?;
let dep_override = table
.remove("override")
.map(parse_dep_override)
.transpose()?
.map_or(false, |o| o);

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

PM::DependencyKind::Local(local)
PM::DependencyKind::Local(
// with allow_cwd_parent set to true, it never fails
PM::normalize_path(local, true /* allow_cwd_parent */).unwrap(),
)
}

(None, subdir, Some(git_url), None) => {
Expand Down Expand Up @@ -433,6 +441,7 @@ pub fn parse_dependency(dep_name: &str, mut tval: TV) -> Result<PM::Dependency>
subst,
version,
digest,
dep_override,
}))
}

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

fn parse_dep_override(tval: TV) -> Result<PM::DepOverride> {
if !tval.is_bool() {
bail!("Invalid dependency override value");
}
Ok(tval.as_bool().unwrap())
}

// check that only recognized names are provided at the top-level
fn warn_if_unknown_field_names(table: &toml::map::Map<String, TV>, known_names: &[&str]) {
let mut unknown_names = BTreeSet::new();
Expand Down
4 changes: 3 additions & 1 deletion language/tools/move-package/src/source_package/parsed_manifest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub type NamedAddress = Symbol;
pub type PackageName = Symbol;
pub type FileName = Symbol;
pub type PackageDigest = Symbol;
pub type DepOverride = bool;

pub type AddressDeclarations = BTreeMap<NamedAddress, Option<AccountAddress>>;
pub type DevAddressDeclarations = BTreeMap<NamedAddress, AccountAddress>;
Expand Down Expand Up @@ -55,6 +56,7 @@ pub struct InternalDependency {
pub subst: Option<Substitution>,
pub version: Option<Version>,
pub digest: Option<PackageDigest>,
pub dep_override: DepOverride,
}

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

let mut stack = Vec::new();
Expand Down
40 changes: 32 additions & 8 deletions language/tools/move-package/tests/test_dependency_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

use std::{
collections::BTreeSet,
collections::{BTreeMap, BTreeSet},
fs::{self, File},
io::Write,
path::PathBuf,
Expand Down Expand Up @@ -184,7 +184,15 @@ fn merge_simple() {
)
.expect("Reading inner");

assert!(outer.merge(inner, Symbol::from("")).is_ok());
assert!(outer
.merge(
Symbol::from("A"),
Symbol::from("A"),
inner,
Symbol::from(""),
&BTreeMap::new(),
)
.is_ok());

assert_eq!(
outer.topological_order(),
Expand Down Expand Up @@ -214,7 +222,15 @@ fn merge_into_root() {
)
.expect("Reading inner");

assert!(outer.merge(inner, Symbol::from("")).is_ok());
assert!(outer
.merge(
Symbol::from("Root"),
Symbol::from("A"),
inner,
Symbol::from(""),
&BTreeMap::new(),
)
.is_ok());

assert_eq!(
outer.topological_order(),
Expand Down Expand Up @@ -243,7 +259,7 @@ fn merge_detached() {
)
.expect("Reading inner");

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

Expand All @@ -267,7 +283,7 @@ fn merge_after_calculating_always_deps() {
)
.expect("Reading inner");

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

Expand Down Expand Up @@ -295,7 +311,15 @@ fn merge_overlapping() {
)
.expect("Reading inner");

assert!(outer.merge(inner, Symbol::from("")).is_ok());
assert!(outer
.merge(
Symbol::from("B"),
Symbol::from("A"),
inner,
Symbol::from(""),
&BTreeMap::new(),
)
.is_ok());
}

#[test]
Expand All @@ -319,7 +343,7 @@ fn merge_overlapping_different_deps() {
)
.expect("Reading inner");

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

Expand Down Expand Up @@ -347,7 +371,7 @@ fn merge_cyclic() {
)
.expect("Reading inner");

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

Expand Down
16 changes: 13 additions & 3 deletions language/tools/move-package/tests/test_sources/dep_dev_dep_diamond/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,27 +69,31 @@ ResolvedGraph {
),
version: None,
resolver: None,
overridden_path: false,
},
"B": Package {
kind: Local(
"deps_only/B",
),
version: None,
resolver: None,
overridden_path: false,
},
"C": Package {
kind: Local(
"deps_only/C",
),
version: None,
resolver: None,
overridden_path: false,
},
"D": Package {
kind: Local(
"deps_only/D",
),
version: None,
resolver: None,
overridden_path: false,
},
},
always_deps: {
Expand Down Expand Up @@ -142,6 +146,7 @@ ResolvedGraph {
subst: None,
version: None,
digest: None,
dep_override: false,
},
),
},
Expand All @@ -154,6 +159,7 @@ ResolvedGraph {
subst: None,
version: None,
digest: None,
dep_override: false,
},
),
},
Expand Down Expand Up @@ -189,6 +195,7 @@ ResolvedGraph {
subst: None,
version: None,
digest: None,
dep_override: false,
},
),
},
Expand Down Expand Up @@ -266,33 +273,36 @@ ResolvedGraph {
"A": Internal(
InternalDependency {
kind: Local(
"./deps_only/A",
"deps_only/A",
),
subst: None,
version: None,
digest: None,
dep_override: false,
},
),
"C": Internal(
InternalDependency {
kind: Local(
"./deps_only/C",
"deps_only/C",
),
subst: None,
version: None,
digest: None,
dep_override: false,
},
),
},
dev_dependencies: {
"B": Internal(
InternalDependency {
kind: Local(
"./deps_only/B",
"deps_only/B",
),
subst: None,
version: None,
digest: None,
dep_override: false,
},
),
},
Expand Down
4 changes: 3 additions & 1 deletion language/tools/move-package/tests/test_sources/dep_good_digest/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ ResolvedGraph {
),
version: None,
resolver: None,
overridden_path: false,
},
},
always_deps: {
Expand Down Expand Up @@ -104,7 +105,7 @@ ResolvedGraph {
"OtherDep": Internal(
InternalDependency {
kind: Local(
"./deps_only/other_dep",
"deps_only/other_dep",
),
subst: Some(
{
Expand All @@ -117,6 +118,7 @@ ResolvedGraph {
digest: Some(
"6A88B7888D6049EB0121900E22B6FA2C0E702F042C8C8D4FD62AD5C990B9F9A8",
),
dep_override: false,
},
),
},
Expand Down
11 changes: 9 additions & 2 deletions ...e/tools/move-package/tests/test_sources/diamond_problem_backflow_resolution/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,20 +51,23 @@ ResolvedGraph {
),
version: None,
resolver: None,
overridden_path: false,
},
"B": Package {
kind: Local(
"deps_only/B",
),
version: None,
resolver: None,
overridden_path: false,
},
"C": Package {
kind: Local(
"deps_only/C",
),
version: None,
resolver: None,
overridden_path: false,
},
},
always_deps: {
Expand Down Expand Up @@ -123,6 +126,7 @@ ResolvedGraph {
),
version: None,
digest: None,
dep_override: false,
},
),
},
Expand Down Expand Up @@ -171,6 +175,7 @@ ResolvedGraph {
),
version: None,
digest: None,
dep_override: false,
},
),
},
Expand Down Expand Up @@ -238,17 +243,18 @@ ResolvedGraph {
"A": Internal(
InternalDependency {
kind: Local(
"./deps_only/A",
"deps_only/A",
),
subst: None,
version: None,
digest: None,
dep_override: false,
},
),
"B": Internal(
InternalDependency {
kind: Local(
"./deps_only/B",
"deps_only/B",
),
subst: Some(
{
Expand All @@ -259,6 +265,7 @@ ResolvedGraph {
),
version: None,
digest: None,
dep_override: false,
},
),
},
Expand Down
3 changes: 3 additions & 0 deletions language/tools/move-package/tests/test_sources/diamond_problem_dep_conflict/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Failed to resolve dependencies for package 'Root': Resolving dependencies for package 'B': Conflicting dependencies found:
C = { local = "deps_only/C", version = "2.0.0" }
C = { local = "deps_only/C", version = "1.0.0" }
7 changes: 7 additions & 0 deletions language/tools/move-package/tests/test_sources/diamond_problem_dep_conflict/Move.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "Root"
version = "0.0.0"

[dependencies]
A = { local = "./deps_only/A" }
B = { local = "./deps_only/B" }
6 changes: 6 additions & 0 deletions .../tools/move-package/tests/test_sources/diamond_problem_dep_conflict/deps_only/A/Move.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[package]
name = "A"
version = "0.0.0"

[dependencies]
C = { local = "../C", version = "2.0.0" }
6 changes: 6 additions & 0 deletions .../tools/move-package/tests/test_sources/diamond_problem_dep_conflict/deps_only/B/Move.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[package]
name = "B"
version = "0.0.0"

[dependencies]
C = { local = "../C", version = "1.0.0" }
3 changes: 3 additions & 0 deletions .../tools/move-package/tests/test_sources/diamond_problem_dep_conflict/deps_only/C/Move.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[package]
name = "C"
version = "0.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If both A and B agreed on the version of C and it didn't match the version that C reported, I would expect the build to fail, is that what you've found in practice? If not and the version is just ignored, then we should construct a test case that picks versions of C from distinct packages, rather than tweaking the version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These versions are not taken into consideration at all. The only versions taken into consideration are those mentioned in the dependency specification. In other words if A depends on C in the following way:

C = { local = "../C", version = "1.0.0" }

and B depends on C in the following way:

C = { local = "../C", version = "1.0.0" }

or even this way

C = { local = "../C" }

We'd have a diamond problem if some root package depended both on A and B.

Since we rely on versions mentioned in the dependency specification, it did not seem necessary to create different subdirectories for different versions of local dependencies.

I can certainly do that, though, if the current setup is not clear enough (though hopefully not wrong...)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recording a conversation we had offline: I think the test set-up here is fine, but it's highlighted a peculiarity with dependency specification (that we treat dependencies as different based on the version field, even though we don't use the version field during dependency resolution), which we should change in future (probably by removing the version field).

3 changes: 3 additions & 0 deletions ...tools/move-package/tests/test_sources/diamond_problem_dep_external_conflict/Move.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Failed to resolve dependencies for package 'Root': Adding dependencies from ../resolvers/successful.sh for dependency 'A' in 'Root': Conflicting dependencies found:
ADep = { local = "deps_only/ADep", version = "1.0.0" }
ADep = { local = "deps_only/ADep" } # Resolved by ../resolvers/successful.sh
Loading