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

[move-package] Support dependency overrides #964

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

[move-package] Support dependency overrides #964

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
98570d0
[move-package] Support dependency overrides
awelc Mar 4, 2023
eb54a5c
Updated test outputs
awelc Mar 4, 2023
bd139e9
Brought back graph entries
awelc Mar 7, 2023
8f938e1
Dependency override does not have to be an option
awelc Mar 7, 2023
2bcfc51
Cosmetic Rust cleanup
awelc Mar 7, 2023
2f0f791
Updated test output
awelc Mar 7, 2023
e1b18b5
Remove dependency overrides from lock files
awelc Mar 7, 2023
8c71fe4
A solution for explicitly defined overrides
awelc Mar 8, 2023
b5f59d6
Normalized local paths when reading manifest
awelc Mar 8, 2023
4a80f06
Cosmetic comment change
awelc Mar 8, 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
369 changes: 308 additions & 61 deletions language/tools/move-package/src/resolution/dependency_graph.rs
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions language/tools/move-package/src/resolution/lock_file/schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ pub struct Dependency {

/// Expected hash for the source and manifest of the package being depended upon.
pub digest: Option<String>,

/// Optional dependency override to handle dependency conflicts.
pub dep_override: Option<bool>,

@amnn amnn Mar 6, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think dep overrides should be necessary in the lock file -- i.e. the lock file should only contain one package for each package name.

@awelc awelc Mar 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The thought here (though not fully implemented yet) was that there may be an override encoded in the dependency graph coming from an external resolver. In particular, for the following manifest for a package involving a diamond problem down the line (e.g., https://github.com/move-language/move/pull/964/files#diff-8c1d4a846826c7a3c78ecd708834e6d958c922a948298f9279e402ab3b30ea37) , instead of overriding B an external resolver could conceivably contain an override for A:

[package]
name = "Root"
version = "0.0.0"

[dependencies]
B = { local = "./deps_only/B", override = true }

[dependencies.A]
resolver = "../resolvers/successful.sh"

[dependencies.A.packages]
Contains = "Anything"
Has = { No = "Schema" }

If we don't worry about this case (I think the external override would only make sense at the level of external root package) then we indeed don't need overrides in Move.lock files.

@awelc awelc Mar 7, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Let's not worry about external resolvers specifying overrides (for now).

}

#[derive(Serialize, Deserialize)]
Expand Down
12 changes: 12 additions & 0 deletions 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,10 @@ 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()?;

let kind = match (
table.remove("local"),
Expand Down Expand Up @@ -433,6 +437,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 +507,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
19 changes: 18 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,15 +56,31 @@ pub struct InternalDependency {
pub subst: Option<Substitution>,
pub version: Option<Version>,
pub digest: Option<PackageDigest>,
pub dep_override: Option<DepOverride>,
Comment thread
awelc marked this conversation as resolved.
Outdated
}

#[derive(Debug, Clone, Eq, PartialEq)]
#[derive(Debug, Clone, Eq)]
pub enum DependencyKind {
Local(PathBuf),
Git(GitInfo),
Custom(CustomDepInfo),
}

/// Custom implementation to normalize local paths
impl PartialEq for DependencyKind {
fn eq(&self, other: &Self) -> bool {
match (self, other) {
(&DependencyKind::Local(ref p), &DependencyKind::Local(ref op)) => {
normalize_path(p, true).unwrap() == normalize_path(op, true).unwrap()

@amnn amnn Mar 6, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it possible to ensure local paths are stored normalized? No worries if not, but I remember some other occasions where I ran into this, and that's how I solved it (by calling normalize_path).

@awelc awelc Mar 6, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Is it possible to ensure local paths are stored normalized?

That was my first thought, but this I believe would require changing the code parsing the manifest file and I wasn't sure we wanted to go there. The comparison change seemed less invasive, but I am happy to go the other route if it looks better.

@awelc awelc Mar 7, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Let's settle on normalizing local paths.

}

(&DependencyKind::Git(ref i), &DependencyKind::Git(ref iv)) => i == iv,
(&DependencyKind::Custom(ref i), &DependencyKind::Custom(ref iv)) => i == iv,
_ => false,
}
}
}

#[derive(Debug, Clone, Eq, PartialEq)]
pub struct GitInfo {
/// The git clone url to download from
Expand Down
36 changes: 21 additions & 15 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 @@ -17,38 +17,38 @@ ResolvedGraph {
Outgoing,
),
],
"B": [
"A": [
(
"C",
Outgoing,
"Root",
Incoming,
),
(
"A",
Incoming,
"B",
Outgoing,
),
(
"Root",
Incoming,
"D",
Outgoing,
),
],
"C": [
"B": [
(
"B",
"A",
Incoming,
),
(
"C",
Outgoing,
),
(
"Root",
Incoming,
),
],
"A": [
"C": [
(
"B",
Outgoing,
),
(
"D",
Outgoing,
Incoming,
),
(
"Root",
Expand Down Expand Up @@ -142,6 +142,7 @@ ResolvedGraph {
subst: None,
version: None,
digest: None,
dep_override: None,
},
),
},
Expand All @@ -154,6 +155,7 @@ ResolvedGraph {
subst: None,
version: None,
digest: None,
dep_override: None,
},
),
},
Expand Down Expand Up @@ -189,6 +191,7 @@ ResolvedGraph {
subst: None,
version: None,
digest: None,
dep_override: None,
},
),
},
Expand Down Expand Up @@ -271,6 +274,7 @@ ResolvedGraph {
subst: None,
version: None,
digest: None,
dep_override: None,
},
),
"C": Internal(
Expand All @@ -281,6 +285,7 @@ ResolvedGraph {
subst: None,
version: None,
digest: None,
dep_override: None,
},
),
},
Expand All @@ -293,6 +298,7 @@ ResolvedGraph {
subst: None,
version: None,
digest: None,
dep_override: None,
},
),
},
Expand Down
1 change: 1 addition & 0 deletions 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 @@ -117,6 +117,7 @@ ResolvedGraph {
digest: Some(
"6A88B7888D6049EB0121900E22B6FA2C0E702F042C8C8D4FD62AD5C990B9F9A8",
),
dep_override: None,
},
),
},
Expand Down
20 changes: 12 additions & 8 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 @@ -14,14 +14,14 @@ ResolvedGraph {
),
],
"A": [
(
"C",
Outgoing,
),
(
"Root",
Incoming,
),
(
"C",
Outgoing,
),
],
"C": [
(
Expand All @@ -34,14 +34,14 @@ ResolvedGraph {
),
],
"B": [
(
"C",
Outgoing,
),
(
"Root",
Incoming,
),
(
"C",
Outgoing,
),
],
},
package_table: {
Expand Down Expand Up @@ -123,6 +123,7 @@ ResolvedGraph {
),
version: None,
digest: None,
dep_override: None,
},
),
},
Expand Down Expand Up @@ -171,6 +172,7 @@ ResolvedGraph {
),
version: None,
digest: None,
dep_override: None,
},
),
},
Expand Down Expand Up @@ -243,6 +245,7 @@ ResolvedGraph {
subst: None,
version: None,
digest: None,
dep_override: None,
},
),
"B": Internal(
Expand All @@ -259,6 +262,7 @@ ResolvedGraph {
),
version: None,
digest: None,
dep_override: None,
},
),
},
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"
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
13 changes: 13 additions & 0 deletions ...age/tools/move-package/tests/test_sources/diamond_problem_dep_external_conflict/Move.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
[package]
name = "Root"
version = "0.0.0"

[dependencies]
B = { local = "./deps_only/B" }

[dependencies.A]
resolver = "../resolvers/successful.sh"

[dependencies.A.packages]
Contains = "Anything"
Has = { No = "Schema" }
6 changes: 6 additions & 0 deletions ...ve-package/tests/test_sources/diamond_problem_dep_external_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]
ADep = { local = "../ADep" }
3 changes: 3 additions & 0 deletions ...package/tests/test_sources/diamond_problem_dep_external_conflict/deps_only/ADep/Move.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[package]
name = "ADep"
version = "0.0.0"
6 changes: 6 additions & 0 deletions ...ve-package/tests/test_sources/diamond_problem_dep_external_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]
ADep = { local = "../ADep", version = "1.0.0" }
29 changes: 29 additions & 0 deletions ...ools/move-package/tests/test_sources/diamond_problem_dep_external_no_conflict/Move.locked
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# @generated by Move, please check-in and do not edit manually.

[move]
version = 0

dependencies = [
{ name = "A" },
{ name = "B" },
]

[[move.package]]
name = "A"
source = { local = "./deps_only/A" }

dependencies = [
{ name = "ADep" },
]

[[move.package]]
name = "ADep"
source = { local = "deps_only/ADep" }

[[move.package]]
name = "B"
source = { local = "deps_only/B" }

dependencies = [
{ name = "ADep" },
]
Loading