From 888f6da712185e147421bf0886652f32035dbc85 Mon Sep 17 00:00:00 2001 From: Adam Welc Date: Fri, 22 Sep 2023 14:44:18 +0200 Subject: [PATCH] [packages] Fix dependency override prunning issue (#13921) ## Description Fixes an issue related to pruning overridden dependencies during dependency graph construction. More concretely, when tryin to find dependency paths in a pruned sub-graph, one could encounter no target node if this node was representing a pruned overridden package. The proposed solution is to simply also search for the target node in the overrides map (if a search for it in the sub-graph fails) ## Test Plan Added a test for this specific scenario, plus all existing tests must pass. --- 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) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes --- .../src/resolution/dependency_graph.rs | 56 +++- .../tests/test_dependency_graph.rs | 24 +- .../nested_deps_shared_override/Move.locked | 33 ++ .../nested_deps_shared_override/Move.resolved | 289 ++++++++++++++++++ .../nested_deps_shared_override/Move.toml | 8 + .../deps_only/more/Move.toml | 6 + .../deps_only/nested/Move.toml | 7 + .../deps_only/shared/Move.toml | 3 + 8 files changed, 408 insertions(+), 18 deletions(-) create mode 100644 tools/move-package/tests/test_sources/nested_deps_shared_override/Move.locked create mode 100644 tools/move-package/tests/test_sources/nested_deps_shared_override/Move.resolved create mode 100644 tools/move-package/tests/test_sources/nested_deps_shared_override/Move.toml create mode 100644 tools/move-package/tests/test_sources/nested_deps_shared_override/deps_only/more/Move.toml create mode 100644 tools/move-package/tests/test_sources/nested_deps_shared_override/deps_only/nested/Move.toml create mode 100644 tools/move-package/tests/test_sources/nested_deps_shared_override/deps_only/shared/Move.toml diff --git a/tools/move-package/src/resolution/dependency_graph.rs b/tools/move-package/src/resolution/dependency_graph.rs index 62a6e1659e..db72681caf 100644 --- a/tools/move-package/src/resolution/dependency_graph.rs +++ b/tools/move-package/src/resolution/dependency_graph.rs @@ -270,7 +270,7 @@ impl DependencyGraphBuilder { .add_node(combined_graph.root_package); // get overrides - let overrides = collect_overrides(parent, &root_manifest.dependencies)?; + let mut overrides = collect_overrides(parent, &root_manifest.dependencies)?; let dev_overrides = collect_overrides(parent, &root_manifest.dev_dependencies)?; for ( @@ -296,7 +296,10 @@ impl DependencyGraphBuilder { let mut all_deps = root_manifest.dependencies.clone(); all_deps.extend(root_manifest.dev_dependencies.clone()); - combined_graph.merge(dep_graphs, parent, &all_deps)?; + // we can mash overrides together as the sets cannot overlap (it's asserted during pruning) + overrides.extend(dev_overrides); + + combined_graph.merge(dep_graphs, parent, &all_deps, &overrides)?; combined_graph.check_acyclic()?; combined_graph.discover_always_deps(); @@ -563,6 +566,7 @@ impl DependencyGraph { mut dep_graphs: BTreeMap, parent: &PM::DependencyKind, dependencies: &PM::Dependencies, + overrides: &BTreeMap, ) -> Result<()> { if !self.always_deps.is_empty() { bail!("Merging dependencies into a graph after calculating its 'always' dependencies"); @@ -664,6 +668,7 @@ impl DependencyGraph { &dep_graphs, graph_info.is_external, ), + overrides, ) { Ok(_) => continue, Err((existing_pkg_deps, pkg_deps)) => { @@ -715,7 +720,7 @@ impl DependencyGraph { /// available in a package tables of a respective (dependent) subgraph. This function return the /// right package table, depending on whether conflict was detected between pre-populated /// combined graph and another sub-graph or between two separate sub-graphs. If we tried to use - /// combined graphs's package table "as is" we would get an error in all cases similar to the on + /// combined graphs's package table "as is" we would get an error in all cases similar to the one /// in the direct_and_indirect_dep test where A is a direct dependency of Root (as C would be /// missing from the combined graph's table): /// @@ -1452,6 +1457,7 @@ fn deps_equal<'a>( graph1_pkg_table: &'a BTreeMap, graph2: &'a DependencyGraph, graph2_pkg_table: &'a BTreeMap, + overrides: &'a BTreeMap, ) -> std::result::Result< (), ( @@ -1459,18 +1465,38 @@ fn deps_equal<'a>( Vec<(&'a Dependency, PM::PackageName, &'a Package)>, ), > { - let graph1_edges = BTreeSet::from_iter( - graph1 - .package_graph - .edges(pkg_name) - .map(|(_, pkg, dep)| (dep, pkg, graph1_pkg_table.get(&pkg).unwrap())), - ); - let graph2_edges = BTreeSet::from_iter( - graph2 - .package_graph - .edges(pkg_name) - .map(|(_, pkg, dep)| (dep, pkg, graph2_pkg_table.get(&pkg).unwrap())), - ); + // Unwraps in the code below are safe as these edges (and target nodes) must exist either in the + // sub-graph or in the pre-populated combined graph (see pkg_table_for_deps_compare's doc + // comment for a more detailed explanation). If these were to fail, it would indicate a bug in + // the algorithm so it's OK to panic here. + let graph1_edges: BTreeSet<_> = graph1 + .package_graph + .edges(pkg_name) + .map(|(_, pkg, dep)| { + ( + dep, + pkg, + graph1_pkg_table + .get(&pkg) + .or_else(|| overrides.get(&pkg)) + .unwrap(), + ) + }) + .collect(); + let graph2_edges: BTreeSet<_> = graph2 + .package_graph + .edges(pkg_name) + .map(|(_, pkg, dep)| { + ( + dep, + pkg, + graph2_pkg_table + .get(&pkg) + .or_else(|| overrides.get(&pkg)) + .unwrap(), + ) + }) + .collect(); let (graph1_pkgs, graph2_pkgs): (Vec<_>, Vec<_>) = graph1_edges .symmetric_difference(&graph2_edges) diff --git a/tools/move-package/tests/test_dependency_graph.rs b/tools/move-package/tests/test_dependency_graph.rs index 5846bad3a9..4508e4285d 100644 --- a/tools/move-package/tests/test_dependency_graph.rs +++ b/tools/move-package/tests/test_dependency_graph.rs @@ -234,7 +234,12 @@ fn merge_simple() { }), )]); assert!(outer - .merge(dep_graphs, &DependencyKind::default(), dependencies,) + .merge( + dep_graphs, + &DependencyKind::default(), + dependencies, + &BTreeMap::new() + ) .is_ok(),); assert_eq!( outer.topological_order(), @@ -280,7 +285,12 @@ fn merge_into_root() { }), )]); assert!(outer - .merge(dep_graphs, &DependencyKind::default(), dependencies,) + .merge( + dep_graphs, + &DependencyKind::default(), + dependencies, + &BTreeMap::new() + ) .is_ok()); assert_eq!( @@ -320,6 +330,7 @@ fn merge_detached() { dep_graphs, &DependencyKind::default(), &BTreeMap::new(), + &BTreeMap::new() ) else { panic!("Inner's root is not part of outer's graph, so this should fail"); }; @@ -354,6 +365,7 @@ fn merge_after_calculating_always_deps() { dep_graphs, &DependencyKind::default(), &BTreeMap::new(), + &BTreeMap::new() ) else { panic!("Outer's always deps have already been calculated so this should fail"); }; @@ -426,7 +438,12 @@ fn merge_overlapping() { ), ]); assert!(outer - .merge(dep_graphs, &DependencyKind::default(), dependencies,) + .merge( + dep_graphs, + &DependencyKind::default(), + dependencies, + &BTreeMap::new() + ) .is_ok()); } @@ -498,6 +515,7 @@ fn merge_overlapping_different_deps() { dep_graphs, &DependencyKind::default(), dependencies, + &BTreeMap::new() ) else { panic!("Outer and inner mention package A which has different dependencies in both."); }; diff --git a/tools/move-package/tests/test_sources/nested_deps_shared_override/Move.locked b/tools/move-package/tests/test_sources/nested_deps_shared_override/Move.locked new file mode 100644 index 0000000000..aa2f8c1d5d --- /dev/null +++ b/tools/move-package/tests/test_sources/nested_deps_shared_override/Move.locked @@ -0,0 +1,33 @@ +# @generated by Move, please check-in and do not edit manually. + +[move] +version = 0 +manifest_digest = "D2FFFC63D2AF7BF7028F583317C7ACF0EBB9E6B9F0DFC6D8FB46732AF7B552E4" +deps_digest = "060AD7E57DFB13104F21BE5F5C3759D03F0553FC3229247D9A7A6B45F50D03A3" + +dependencies = [ + { name = "More" }, + { name = "Nested" }, + { name = "Shared" }, +] + +[[move.package]] +name = "More" +source = { local = "deps_only/more" } + +dependencies = [ + { name = "Shared" }, +] + +[[move.package]] +name = "Nested" +source = { local = "deps_only/nested" } + +dependencies = [ + { name = "More" }, + { name = "Shared" }, +] + +[[move.package]] +name = "Shared" +source = { local = "deps_only/shared" } diff --git a/tools/move-package/tests/test_sources/nested_deps_shared_override/Move.resolved b/tools/move-package/tests/test_sources/nested_deps_shared_override/Move.resolved new file mode 100644 index 0000000000..38c71803a8 --- /dev/null +++ b/tools/move-package/tests/test_sources/nested_deps_shared_override/Move.resolved @@ -0,0 +1,289 @@ +ResolvedGraph { + graph: DependencyGraph { + root_path: "tests/test_sources/nested_deps_shared_override", + root_package: "Root", + package_graph: { + "Root": [ + ( + "More", + Outgoing, + ), + ( + "Nested", + Outgoing, + ), + ( + "Shared", + Outgoing, + ), + ], + "More": [ + ( + "Root", + Incoming, + ), + ( + "Shared", + Outgoing, + ), + ( + "Nested", + Incoming, + ), + ], + "Shared": [ + ( + "More", + Incoming, + ), + ( + "Nested", + Incoming, + ), + ( + "Root", + Incoming, + ), + ], + "Nested": [ + ( + "Root", + Incoming, + ), + ( + "More", + Outgoing, + ), + ( + "Shared", + Outgoing, + ), + ], + }, + package_table: { + "More": Package { + kind: Local( + "deps_only/more", + ), + version: None, + resolver: None, + }, + "Nested": Package { + kind: Local( + "deps_only/nested", + ), + version: None, + resolver: None, + }, + "Shared": Package { + kind: Local( + "deps_only/shared", + ), + version: None, + resolver: None, + }, + }, + always_deps: { + "More", + "Nested", + "Root", + "Shared", + }, + manifest_digest: "D2FFFC63D2AF7BF7028F583317C7ACF0EBB9E6B9F0DFC6D8FB46732AF7B552E4", + deps_digest: "060AD7E57DFB13104F21BE5F5C3759D03F0553FC3229247D9A7A6B45F50D03A3", + }, + build_options: BuildConfig { + dev_mode: true, + test_mode: false, + generate_docs: false, + generate_abis: false, + install_dir: Some( + "ELIDED_FOR_TEST", + ), + force_recompilation: false, + lock_file: Some( + "ELIDED_FOR_TEST", + ), + additional_named_addresses: {}, + fetch_deps_only: false, + skip_fetch_latest_git_deps: false, + default_flavor: None, + default_edition: None, + deps_as_root: false, + }, + package_table: { + "More": Package { + source_package: SourceManifest { + package: PackageInfo { + name: "More", + version: ( + 0, + 0, + 0, + ), + authors: [], + license: None, + edition: None, + flavor: None, + custom_properties: {}, + }, + addresses: None, + dev_address_assignments: None, + build: None, + dependencies: { + "Shared": Internal( + InternalDependency { + kind: Local( + "../shared", + ), + subst: None, + version: None, + digest: None, + dep_override: true, + }, + ), + }, + dev_dependencies: {}, + }, + package_path: "ELIDED_FOR_TEST", + renaming: {}, + resolved_table: {}, + source_digest: "ELIDED_FOR_TEST", + }, + "Nested": Package { + source_package: SourceManifest { + package: PackageInfo { + name: "Nested", + version: ( + 0, + 0, + 0, + ), + authors: [], + license: None, + edition: None, + flavor: None, + custom_properties: {}, + }, + addresses: None, + dev_address_assignments: None, + build: None, + dependencies: { + "More": Internal( + InternalDependency { + kind: Local( + "../more", + ), + subst: None, + version: None, + digest: None, + dep_override: false, + }, + ), + "Shared": Internal( + InternalDependency { + kind: Local( + "../shared", + ), + subst: None, + version: None, + digest: None, + dep_override: true, + }, + ), + }, + dev_dependencies: {}, + }, + package_path: "ELIDED_FOR_TEST", + renaming: {}, + resolved_table: {}, + source_digest: "ELIDED_FOR_TEST", + }, + "Root": Package { + source_package: SourceManifest { + package: PackageInfo { + name: "Root", + version: ( + 0, + 0, + 0, + ), + authors: [], + license: None, + edition: None, + flavor: None, + custom_properties: {}, + }, + addresses: None, + dev_address_assignments: None, + build: None, + dependencies: { + "More": Internal( + InternalDependency { + kind: Local( + "deps_only/more", + ), + subst: None, + version: None, + digest: None, + dep_override: false, + }, + ), + "Nested": Internal( + InternalDependency { + kind: Local( + "deps_only/nested", + ), + subst: None, + version: None, + digest: None, + dep_override: false, + }, + ), + "Shared": Internal( + InternalDependency { + kind: Local( + "deps_only/shared", + ), + subst: None, + version: None, + digest: None, + dep_override: true, + }, + ), + }, + dev_dependencies: {}, + }, + package_path: "ELIDED_FOR_TEST", + renaming: {}, + resolved_table: {}, + source_digest: "ELIDED_FOR_TEST", + }, + "Shared": Package { + source_package: SourceManifest { + package: PackageInfo { + name: "Shared", + version: ( + 0, + 0, + 0, + ), + authors: [], + license: None, + edition: None, + flavor: None, + custom_properties: {}, + }, + addresses: None, + dev_address_assignments: None, + build: None, + dependencies: {}, + dev_dependencies: {}, + }, + package_path: "ELIDED_FOR_TEST", + renaming: {}, + resolved_table: {}, + source_digest: "ELIDED_FOR_TEST", + }, + }, +} diff --git a/tools/move-package/tests/test_sources/nested_deps_shared_override/Move.toml b/tools/move-package/tests/test_sources/nested_deps_shared_override/Move.toml new file mode 100644 index 0000000000..c5cd86e8d6 --- /dev/null +++ b/tools/move-package/tests/test_sources/nested_deps_shared_override/Move.toml @@ -0,0 +1,8 @@ +[package] +name = "Root" +version = "0.0.0" + +[dependencies] +Shared = { local = "./deps_only/shared", override = true } +Nested = { local = "./deps_only/nested" } +More = { local = "./deps_only/more" } diff --git a/tools/move-package/tests/test_sources/nested_deps_shared_override/deps_only/more/Move.toml b/tools/move-package/tests/test_sources/nested_deps_shared_override/deps_only/more/Move.toml new file mode 100644 index 0000000000..0bd5f9511a --- /dev/null +++ b/tools/move-package/tests/test_sources/nested_deps_shared_override/deps_only/more/Move.toml @@ -0,0 +1,6 @@ +[package] +name = "More" +version = "0.0.0" + +[dependencies] +Shared = { local = "../shared", override = true } diff --git a/tools/move-package/tests/test_sources/nested_deps_shared_override/deps_only/nested/Move.toml b/tools/move-package/tests/test_sources/nested_deps_shared_override/deps_only/nested/Move.toml new file mode 100644 index 0000000000..172978bf4b --- /dev/null +++ b/tools/move-package/tests/test_sources/nested_deps_shared_override/deps_only/nested/Move.toml @@ -0,0 +1,7 @@ +[package] +name = "Nested" +version = "0.0.0" + +[dependencies] +Shared = { local = "../shared", override = true } +More = { local = "../more" } diff --git a/tools/move-package/tests/test_sources/nested_deps_shared_override/deps_only/shared/Move.toml b/tools/move-package/tests/test_sources/nested_deps_shared_override/deps_only/shared/Move.toml new file mode 100644 index 0000000000..2cb6e69203 --- /dev/null +++ b/tools/move-package/tests/test_sources/nested_deps_shared_override/deps_only/shared/Move.toml @@ -0,0 +1,3 @@ +[package] +name = "Shared" +version = "0.0.0"