Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[move-package] Support dependency overrides #11181

Merged
merged 1 commit into from
Apr 21, 2023
Merged

[move-package] Support dependency overrides #11181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
586 changes: 475 additions & 111 deletions external-crates/move/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 external-crates/move/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 external-crates/move/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 external-crates/move/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 external-crates/move/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 external-crates/move/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 ...tes/move/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 ...-crates/move/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
Loading