[move] Update syntax and usage for external resolvers in Move packages (

#19561) ## Description This updates the way external package resolvers are declared and used inside Move packages. In particular, external resolvers can be prefixed by `r.` so to use an external resolver binary `foo` you would declare a dependency like `A = { r.foo = "bar", <other args if you want here> }`. This additionally adds support for resolver-specific configs in the `Move.toml` however these are treated opaquely -- any top-level item prefixed with `r.` will be allowed by the parser. Bottom commit are the changes + new tests, and top commit are the test updates. ## Test plan Added new tests, as well as updated existing tests and made sure they continued to work as expected.
MystenLabs · Sep 26, 2024 · be7160a · be7160a
1 parent 3fa3ec9
commit be7160a
Show file tree

Hide file tree

Showing 45 changed files with 208 additions and 124 deletions.
diff --git a/external-crates/move/crates/move-package/src/source_package/manifest_parser.rs b/external-crates/move/crates/move-package/src/source_package/manifest_parser.rs
@@ -25,13 +25,16 @@ const DEV_ADDRESSES_NAME: &str = "dev-addresses";
 const DEPENDENCY_NAME: &str = "dependencies";
 const DEV_DEPENDENCY_NAME: &str = "dev-dependencies";
 
+const EXTERNAL_RESOLVER_PREFIX: &str = "r";
+
 const KNOWN_NAMES: &[&str] = &[
     PACKAGE_NAME,
     BUILD_NAME,
     ADDRESSES_NAME,
     DEV_ADDRESSES_NAME,
     DEPENDENCY_NAME,
     DEV_DEPENDENCY_NAME,
+    EXTERNAL_RESOLVER_PREFIX,
 ];
 
 const REQUIRED_FIELDS: &[&str] = &[PACKAGE_NAME];
@@ -329,24 +332,42 @@ fn parse_address_literal(address_str: &str) -> Result<AccountAddress, AccountAdd
     AccountAddress::from_hex_literal(address_str)
 }
 
-pub fn parse_dependency(mut tval: TV) -> Result<PM::Dependency> {
-    let Some(table) = tval.as_table_mut() else {
-        bail!("Malformed dependency {}", tval);
+fn parse_external_resolver_name(resolver_val: &TV) -> Result<Option<Symbol>> {
+    let Some(table) = resolver_val.as_table() else {
+        bail!("Malformed dependency {}", resolver_val);
     };
 
-    if let Some(resolver) = table.remove("resolver") {
-        let Some(resolver) = resolver.as_str().map(Symbol::from) else {
-            bail!("Resolver name is not a string")
-        };
+    if table.len() != 1 {
+        bail!("Malformed external resolver declaration for dependency {EXTERNAL_RESOLVER_PREFIX}.{resolver_val}",);
+    }
+
+    let key = table
+        .keys()
+        .next()
+        .expect("Exactly one key by check above")
+        .as_str();
+
+    let key_value = table.get(key).ok_or_else(|| {
+        format_err!("Malformed external resolver declaration for dependency {EXTERNAL_RESOLVER_PREFIX}.{resolver_val}",)
+    })?;
 
-        // Not relevant except for the external resolver, but remove it to mark it as a
-        // recognised part of the manifest.
-        let _ = table.remove("packages");
+    if !key_value.is_str() {
+        bail!("Malformed external resolver declaration for dependency {EXTERNAL_RESOLVER_PREFIX}.{resolver_val}",);
+    }
+
+    Ok(Some(Symbol::from(key)))
+}
 
-        // Any fields that are left are unknown
-        warn_if_unknown_field_names(table, &[]);
+pub fn parse_dependency(mut tval: TV) -> Result<PM::Dependency> {
+    let Some(table) = tval.as_table_mut() else {
+        bail!("Malformed dependency {}", tval);
+    };
 
-        return Ok(PM::Dependency::External(resolver));
+    if let Some(external_resolver_binary_name) = table
+        .get(EXTERNAL_RESOLVER_PREFIX)
+        .and_then(|e| parse_external_resolver_name(e).transpose())
+    {
+        return Ok(PM::Dependency::External(external_resolver_binary_name?));
     }
 
     let subst = table
@@ -418,7 +439,7 @@ pub fn parse_dependency(mut tval: TV) -> Result<PM::Dependency> {
         }
 
         _ => {
-            let keys = ["'local'", "'git'", "'resolver'", "'id'"];
+            let keys = ["'local'", "'git'", "'r.<external_resolver_binary_name>'"];
             bail!(
                 "must provide exactly one of {} for dependency.",
                 keys.join(" or ")
@@ -506,7 +527,7 @@ fn parse_dep_override(tval: TV) -> Result<PM::DepOverride> {
     Ok(tval.as_bool().unwrap())
 }
 
-// check that only recognized names are provided at the top-level
+// 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();
     for key in table.keys() {

diff --git a/...ve/crates/move-package/tests/test_sources/diamond_problem_dep_external_conflict/Move.toml b/...ve/crates/move-package/tests/test_sources/diamond_problem_dep_external_conflict/Move.toml
@@ -3,10 +3,4 @@ name = "Root"
 
 [dependencies]
 B = { local = "./deps_only/B" }
-
-[dependencies.A]
-resolver = "../resolvers/successful.sh"
-
-[dependencies.A.packages]
-Contains = "Anything"
-Has = { No = "Schema" }
+A = { r."../resolvers/successful.sh" = "A" }
diff --git a/...sts/test_sources/diamond_problem_dep_external_incorrect_override_v1/deps_only/C/Move.toml b/...sts/test_sources/diamond_problem_dep_external_incorrect_override_v1/deps_only/C/Move.toml
@@ -1,9 +1,5 @@
 [package]
 name = "C"
 
-[dependencies.A]
-resolver = "../../../resolvers/successful.sh"
-
-[dependencies.A.packages]
-Contains = "Anything"
-Has = { No = "Schema" }
+[dependencies]
+A = { r."../../../resolvers/successful.sh" = "A" }
diff --git a/...e-package/tests/test_sources/diamond_problem_dep_external_incorrect_override_v2/Move.toml b/...e-package/tests/test_sources/diamond_problem_dep_external_incorrect_override_v2/Move.toml
@@ -18,10 +18,4 @@ name = "Root"
 
 [dependencies]
 B = { local = "./deps_only/B" }
-
-[dependencies.A]
-resolver = "../resolvers/successful.sh"
-
-[dependencies.A.packages]
-Contains = "Anything"
-Has = { No = "Schema" }
+A = {r."../resolvers/successful.sh" = "A" }
diff --git a/...ates/move-package/tests/test_sources/diamond_problem_dep_external_no_conflict/Move.locked b/...ates/move-package/tests/test_sources/diamond_problem_dep_external_no_conflict/Move.locked
@@ -2,7 +2,7 @@
 
 [move]
 version = 3
-manifest_digest = "B06FCED8E0EF6B62EC0B572DC233C0D70206B1C10EEDEA0403CA64AFBB3E439B"
+manifest_digest = "1A19D99A36EA0D72B36FC64B2108B42452C99FF87E9DF6E0E54A38AD8679D431"
 deps_digest = "3C4103934B1E040BB6B23F1D610B4EF9F2F1166A50A104EADCF77467C004C600"
 dependencies = [
   { id = "A", name = "A" },

diff --git a/...es/move-package/tests/test_sources/diamond_problem_dep_external_no_conflict/Move.resolved b/...es/move-package/tests/test_sources/diamond_problem_dep_external_no_conflict/Move.resolved
@@ -78,7 +78,7 @@ ResolvedGraph {
             "B",
             "Root",
         },
-        manifest_digest: "B06FCED8E0EF6B62EC0B572DC233C0D70206B1C10EEDEA0403CA64AFBB3E439B",
+        manifest_digest: "1A19D99A36EA0D72B36FC64B2108B42452C99FF87E9DF6E0E54A38AD8679D431",
         deps_digest: "3C4103934B1E040BB6B23F1D610B4EF9F2F1166A50A104EADCF77467C004C600",
     },
     build_options: BuildConfig {

diff --git a/...crates/move-package/tests/test_sources/diamond_problem_dep_external_no_conflict/Move.toml b/...crates/move-package/tests/test_sources/diamond_problem_dep_external_no_conflict/Move.toml
@@ -3,10 +3,4 @@ name = "Root"
 
 [dependencies]
 B = { local = "./deps_only/B" }
-
-[dependencies.A]
-resolver = "../resolvers/successful.sh"
-
-[dependencies.A.packages]
-Contains = "Anything"
-Has = { No = "Schema" }
+A = { r."../resolvers/successful.sh" = "A" }
diff --git a/.../crates/move-package/tests/test_sources/diamond_problem_dep_external_override/Move.locked b/.../crates/move-package/tests/test_sources/diamond_problem_dep_external_override/Move.locked
@@ -2,7 +2,7 @@
 
 [move]
 version = 3
-manifest_digest = "82E01E336DD3374BDC42CA355AB6ACA69E44DB65E36DC0A831F36B06F6832574"
+manifest_digest = "962A6DC626FF53896265D6D3CECA6A95E461EA4C97B32A8527807F7225BFF183"
 deps_digest = "060AD7E57DFB13104F21BE5F5C3759D03F0553FC3229247D9A7A6B45F50D03A3"
 dependencies = [
   { id = "A", name = "A" },

diff --git a/...rates/move-package/tests/test_sources/diamond_problem_dep_external_override/Move.resolved b/...rates/move-package/tests/test_sources/diamond_problem_dep_external_override/Move.resolved
@@ -84,7 +84,7 @@ ResolvedGraph {
             "B",
             "Root",
         },
-        manifest_digest: "82E01E336DD3374BDC42CA355AB6ACA69E44DB65E36DC0A831F36B06F6832574",
+        manifest_digest: "962A6DC626FF53896265D6D3CECA6A95E461EA4C97B32A8527807F7225BFF183",
         deps_digest: "060AD7E57DFB13104F21BE5F5C3759D03F0553FC3229247D9A7A6B45F50D03A3",
     },
     build_options: BuildConfig {

diff --git a/...ve/crates/move-package/tests/test_sources/diamond_problem_dep_external_override/Move.toml b/...ve/crates/move-package/tests/test_sources/diamond_problem_dep_external_override/Move.toml
@@ -4,10 +4,4 @@ name = "Root"
 [dependencies]
 B = { local = "./deps_only/B" }
 ADep = { local = "./deps_only/ADep-v1", override = true }
-
-[dependencies.A]
-resolver = "../resolvers/successful.sh"
-
-[dependencies.A.packages]
-Contains = "Anything"
-Has = { No = "Schema" }
+A = { r."../resolvers/successful.sh" = "A" }
diff --git a/...ckage/tests/test_sources/diamond_problem_dep_external_override_root/deps_only/C/Move.toml b/...ckage/tests/test_sources/diamond_problem_dep_external_override_root/deps_only/C/Move.toml
@@ -1,9 +1,5 @@
 [package]
 name = "C"
 
-[dependencies.A]
-resolver = "../../../resolvers/successful.sh"
-
-[dependencies.A.packages]
-Contains = "Anything"
-Has = { No = "Schema" }
+[dependencies]
+A = { r."../../../resolvers/successful.sh" = "A" }
diff --git a/...-package/tests/test_sources/diamond_problem_dep_nested_dep_conflict/deps_only/C/Move.toml b/...-package/tests/test_sources/diamond_problem_dep_nested_dep_conflict/deps_only/C/Move.toml
@@ -1,9 +1,5 @@
 [package]
 name = "C"
 
-[dependencies.A]
-resolver = "../../../resolvers/successful_subst.sh"
-
-[dependencies.A.packages]
-Contains = "Anything"
-Has = { No = "Schema" }
+[dependencies]
+A = { r."../../../resolvers/successful_subst.sh" = "A" }
diff --git a/external-crates/move/crates/move-package/tests/test_sources/external/Move.locked b/external-crates/move/crates/move-package/tests/test_sources/external/Move.locked
@@ -2,7 +2,7 @@
 
 [move]
 version = 3
-manifest_digest = "8D09D19521F36950C0698F14ED09FE4F6175C022796F1E401F2F5A1BCA6FCE98"
+manifest_digest = "0B6E0EF231AAF334D40D1FE28680D7A9B8939FCEF885743368FAF858533CD0E2"
 deps_digest = "F8BBB0CCB2491CA29A3DF03D6F92277A4F3574266507ACD77214D37ECA3F3082"
 dependencies = [
   { id = "A", name = "A" },

diff --git a/external-crates/move/crates/move-package/tests/test_sources/external/Move.resolved b/external-crates/move/crates/move-package/tests/test_sources/external/Move.resolved
@@ -52,7 +52,7 @@ ResolvedGraph {
             "ADep",
             "Root",
         },
-        manifest_digest: "8D09D19521F36950C0698F14ED09FE4F6175C022796F1E401F2F5A1BCA6FCE98",
+        manifest_digest: "0B6E0EF231AAF334D40D1FE28680D7A9B8939FCEF885743368FAF858533CD0E2",
         deps_digest: "F8BBB0CCB2491CA29A3DF03D6F92277A4F3574266507ACD77214D37ECA3F3082",
     },
     build_options: BuildConfig {

diff --git a/external-crates/move/crates/move-package/tests/test_sources/external/Move.toml b/external-crates/move/crates/move-package/tests/test_sources/external/Move.toml
@@ -1,9 +1,5 @@
 [package]
 name = "Root"
 
-[dependencies.A]
-resolver = "../resolvers/successful.sh"
-
-[dependencies.A.packages]
-Contains = "Anything"
-Has = { No = "Schema" }
+[dependencies]
+A = { r."../resolvers/successful.sh" = "A" }
diff --git a/external-crates/move/crates/move-package/tests/test_sources/external_bad_dep/Move.locked b/external-crates/move/crates/move-package/tests/test_sources/external_bad_dep/Move.locked
@@ -2,7 +2,7 @@
 
 [move]
 version = 3
-manifest_digest = "725168ABE1A1677C13C020BCA80D7C059B53A9C7E2E4D15FD0BE671E0D2A87B0"
+manifest_digest = "6CC399FE60217A66068CDC5CCDBB3518CC81C965A4B093EEA0C8FFC55B77D8DA"
 deps_digest = "F8BBB0CCB2491CA29A3DF03D6F92277A4F3574266507ACD77214D37ECA3F3082"
 dependencies = [
   { id = "A", name = "A" },

diff --git a/external-crates/move/crates/move-package/tests/test_sources/external_bad_dep/Move.toml b/external-crates/move/crates/move-package/tests/test_sources/external_bad_dep/Move.toml
@@ -1,7 +1,7 @@
 [package]
 name = "Root"
 
-[dependencies.A]
+[dependencies]
 # The resolver will introduce Root -> A -> ADep into the dependency
 # graph, but ADep doesn't exist, so it will fail.
-resolver = "../resolvers/successful.sh"
+A = { r."../resolvers/successful.sh" = "A" }
diff --git a/external-crates/move/crates/move-package/tests/test_sources/external_broken/Move.toml b/external-crates/move/crates/move-package/tests/test_sources/external_broken/Move.toml
@@ -1,7 +1,7 @@
 [package]
 name = "Root"
 
-[dependencies.A]
 # This resolver exits with success but returns bad output (not a lock
 # file), so resolution fails.
-resolver = "../resolvers/broken.sh"
+[dependencies]
+A = {r."../resolvers/broken.sh" = "a" } 
diff --git a/external-crates/move/crates/move-package/tests/test_sources/external_dev_dep/Move.locked b/external-crates/move/crates/move-package/tests/test_sources/external_dev_dep/Move.locked
@@ -2,7 +2,7 @@
 
 [move]
 version = 3
-manifest_digest = "84A0B503BE9F9B341AC66860713D12704876E5C2425891E3B94626B12E4313E5"
+manifest_digest = "A0EB64199385F1312BF72CA00B135B4C6CB0531F538FFCF9156C6D96FBE10F39"
 deps_digest = "3C4103934B1E040BB6B23F1D610B4EF9F2F1166A50A104EADCF77467C004C600"
 dependencies = [
   { id = "A", name = "A" },

diff --git a/external-crates/move/crates/move-package/tests/test_sources/external_dev_dep/Move.resolved b/external-crates/move/crates/move-package/tests/test_sources/external_dev_dep/Move.resolved
@@ -90,7 +90,7 @@ ResolvedGraph {
             "ADep",
             "Root",
         },
-        manifest_digest: "84A0B503BE9F9B341AC66860713D12704876E5C2425891E3B94626B12E4313E5",
+        manifest_digest: "A0EB64199385F1312BF72CA00B135B4C6CB0531F538FFCF9156C6D96FBE10F39",
         deps_digest: "3C4103934B1E040BB6B23F1D610B4EF9F2F1166A50A104EADCF77467C004C600",
     },
     build_options: BuildConfig {

diff --git a/external-crates/move/crates/move-package/tests/test_sources/external_dev_dep/Move.toml b/external-crates/move/crates/move-package/tests/test_sources/external_dev_dep/Move.toml
@@ -1,10 +1,10 @@
 [package]
 name = "Root"
 
-[dependencies.A]
-resolver = "../resolvers/successful.sh"
+[dependencies]
+A = { r."../resolvers/successful.sh" = "A" }
 
-[dev-dependencies.B]
+[dev-dependencies]
 # External resolvers can be used for dev dependencies as well as
 # regular dependencies
-resolver = "../resolvers/successful.sh"
+B = { r."../resolvers/successful.sh" = "B" }
diff --git a/external-crates/move/crates/move-package/tests/test_sources/external_failing/Move.toml b/external-crates/move/crates/move-package/tests/test_sources/external_failing/Move.toml
@@ -1,7 +1,7 @@
 [package]
 name = "Root"
 
-[dependencies.A]
+[dependencies]
 # This resolver returns a non-zero exit code, which should cause
 # resolution to fail.
-resolver = "../resolvers/failing.sh"
+A = { r."../resolvers/failing.sh" = "A" }
diff --git a/external-crates/move/crates/move-package/tests/test_sources/external_no_resolver/Move.toml b/external-crates/move/crates/move-package/tests/test_sources/external_no_resolver/Move.toml
@@ -1,6 +1,6 @@
 [package]
 name = "Root"
 
-[dependencies.A]
 # This resolver doesn't exist, so calling out to it will fail.
-resolver = "../resolvers/doesnt_exist.sh"
+[dependencies]
+A = { r."../resolvers/doesnt_exist.sh" = "A" }
diff --git a/external-crates/move/crates/move-package/tests/test_sources/external_overlap/Move.locked b/external-crates/move/crates/move-package/tests/test_sources/external_overlap/Move.locked
@@ -2,7 +2,7 @@
 
 [move]
 version = 3
-manifest_digest = "2401A97D2FE979752E0726963BFC3F677A4FA8041F3C6FAE5A4302401B165463"
+manifest_digest = "F8724912814B06DCBDAFDA91E456B456D299C39AB59519BA190B6C7523884CFD"
 deps_digest = "3C4103934B1E040BB6B23F1D610B4EF9F2F1166A50A104EADCF77467C004C600"
 dependencies = [
   { id = "A", name = "A" },

diff --git a/external-crates/move/crates/move-package/tests/test_sources/external_overlap/Move.resolved b/external-crates/move/crates/move-package/tests/test_sources/external_overlap/Move.resolved
@@ -60,7 +60,7 @@ ResolvedGraph {
             "ADep",
             "Root",
         },
-        manifest_digest: "2401A97D2FE979752E0726963BFC3F677A4FA8041F3C6FAE5A4302401B165463",
+        manifest_digest: "F8724912814B06DCBDAFDA91E456B456D299C39AB59519BA190B6C7523884CFD",
         deps_digest: "3C4103934B1E040BB6B23F1D610B4EF9F2F1166A50A104EADCF77467C004C600",
     },
     build_options: BuildConfig {

diff --git a/external-crates/move/crates/move-package/tests/test_sources/external_overlap/Move.toml b/external-crates/move/crates/move-package/tests/test_sources/external_overlap/Move.toml
@@ -5,10 +5,4 @@ name = "Root"
 # This should succeed even though the external resolver will also return
 # `ADep` as a transitive dependency as `ADep` has the same dependencies in both cases.
 ADep = { local = "./deps_only/ADep"}
-
-[dependencies.A]
-resolver = "../resolvers/successful.sh"
-
-[dependencies.A.packages]
-Contains = "Anything"
-Has = { No = "Schema" }
+A = { r."../resolvers/successful.sh" = "A" }
diff --git a/external-crates/move/crates/move-package/tests/test_sources/external_overlap_fail/Move.toml b/external-crates/move/crates/move-package/tests/test_sources/external_overlap_fail/Move.toml
@@ -6,10 +6,4 @@ name = "Root"
 # and the set of `ADep`s own dependencies is empty in the "external" case and non-empty in the
 # "internal" one.
 ADep = { local = "./deps_only/ADep"}
-
-[dependencies.A]
-resolver = "../resolvers/successful.sh"
-
-[dependencies.A.packages]
-Contains = "Anything"
-Has = { No = "Schema" }
+A = { r."../resolvers/successful.sh" = "A" }
diff --git a/...tes/move/crates/move-package/tests/test_sources/external_overlap_fail_symmetric/Move.toml b/...tes/move/crates/move-package/tests/test_sources/external_overlap_fail_symmetric/Move.toml
@@ -6,10 +6,4 @@ name = "Root"
 # `ADep` as a transitive dependency and the set of `ADep`s own
 # dependencies is different in both cases.
 ADep = { local = "./deps_only/ADep"}
-
-[dependencies.A]
-resolver = "../resolvers/successful_dep.sh"
-
-[dependencies.A.packages]
-Contains = "Anything"
-Has = { No = "Schema" }
+A = { r."../resolvers/successful_dep.sh" = "A" }
diff --git a/...l-crates/move/crates/move-package/tests/test_sources/external_resolver_config/Move.locked b/...l-crates/move/crates/move-package/tests/test_sources/external_resolver_config/Move.locked
@@ -0,0 +1,6 @@
+# @generated by Move, please check-in and do not edit manually.
+
+[move]
+version = 3
+manifest_digest = "D0A99B6D19946036C7A76C46B5B13EC9E47ED5FB3B8940036A0B93D627225A8A"
+deps_digest = "E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855"