[Package management] add implicit system package dependencies (dvx-197)

crates/sui-move-build/src/lib.rs

@@ -34,15 +34,21 @@ use move_package::{
     },
     package_hooks::{PackageHooks, PackageIdentifier},
     resolution::{dependency_graph::DependencyGraph, resolution_graph::ResolvedGraph},
-    source_package::parsed_manifest::{Dependencies, PackageName},
-    BuildConfig as MoveBuildConfig, LintFlag,
+    source_package::parsed_manifest::{
+        Dependencies, Dependency, DependencyKind, GitInfo, InternalDependency, PackageName,
+    },
+    BuildConfig as MoveBuildConfig,
 };
 use move_package::{
     source_package::parsed_manifest::OnChainInfo, source_package::parsed_manifest::SourceManifest,
 };
 use move_symbol_pool::Symbol;
 use serde_reflection::Registry;
-use sui_package_management::{resolve_published_id, PublishedAtError};
+use sui_package_management::{
+    resolve_published_id,
+    system_package_versions::{SystemPackagesVersion, SYSTEM_GIT_REPO},
+    PublishedAtError,
+};
 use sui_protocol_config::{Chain, ProtocolConfig, ProtocolVersion};
 use sui_types::{
     base_types::ObjectID,
@@ -110,18 +116,17 @@ pub struct BuildConfig {
 impl BuildConfig {
     pub fn new_for_testing() -> Self {
         move_package::package_hooks::register_package_hooks(Box::new(SuiPackageHooks));
-
-        // Note: in the future, consider changing this to dependencies on the local system
-        // packages:
-        let implicit_dependencies = Dependencies::new();
         let install_dir = tempfile::tempdir().unwrap().into_path();
+
         let config = MoveBuildConfig {
             default_flavor: Some(move_compiler::editions::Flavor::Sui),
-            implicit_dependencies,
+
             lock_file: Some(install_dir.join("Move.lock")),
             install_dir: Some(install_dir),
-            lint_flag: LintFlag::LEVEL_NONE,
             silence_warnings: true,
+            lint_flag: move_package::LintFlag::LEVEL_NONE,
+            // TODO: in the future this should probably be changed to a set of local deps:
+            implicit_dependencies: Dependencies::new(),
             ..MoveBuildConfig::default()
         };
         BuildConfig {
@@ -276,9 +281,9 @@ pub fn build_from_resolution_graph(
 
     // compile!
     let result = if print_diags_to_stderr {
-        BuildConfig::compile_package(resolution_graph, &mut std::io::stderr())
+        BuildConfig::compile_package(&resolution_graph, &mut std::io::stderr())
     } else {
-        BuildConfig::compile_package(resolution_graph, &mut std::io::sink())
+        BuildConfig::compile_package(&resolution_graph, &mut std::io::sink())
     };
 
     let (package, fn_info) = result.map_err(|error| SuiError::ModuleBuildFailure {
@@ -295,6 +300,7 @@ pub fn build_from_resolution_graph(
         published_at,
         dependency_ids,
         bytecode_deps,
+        dependency_graph: resolution_graph.graph,
     })
 }
 
@@ -638,15 +644,12 @@ impl CompiledPackage {
         self.dependency_ids.published.values().cloned().collect()
     }
 
-    /// Tree-shake the package's dependencies to remove any that are not referenced in source code.
-    ///
-    /// This algorithm uses the set of root modules as the starting point to retrieve the
-    /// list of used packages that are immediate dependencies of these modules. Essentially, it
-    /// will remove any package that has no immediate module dependency to it.
-    ///
-    /// Then, it will recursively find all the transitive dependencies of the packages in the list
-    /// above and add them to the list of packages that need to be kept as dependencies.
-    pub fn tree_shake(&mut self, with_unpublished_deps: bool) -> Result<(), anyhow::Error> {
+    /// Find the map of packages that are immediate dependencies of the root modules, joined with
+    /// the set of bytecode dependencies.
+    pub fn find_immediate_deps_pkgs_to_keep(
+        &self,
+        with_unpublished_deps: bool,
+    ) -> Result<BTreeMap<Symbol, ObjectID>, anyhow::Error> {
         // Start from the root modules (or all modules if with_unpublished_deps is true as we
         // need to include modules with 0x0 address)
         let root_modules: Vec<_> = if with_unpublished_deps {
@@ -663,15 +666,14 @@ impl CompiledPackage {
         };
 
         // Find the immediate dependencies for each root module and store the package name
-        // in the used_immediate_packages set. This basically prunes the packages that are not used
+        // in the pkgs_to_keep set. This basically prunes the packages that are not used
         // based on the modules information.
         let mut pkgs_to_keep: BTreeSet<Symbol> = BTreeSet::new();
         let module_to_pkg_name: BTreeMap<_, _> = self
             .package
             .all_modules()
             .map(|m| (m.unit.module.self_id(), m.unit.package_name))
             .collect();
-        let mut used_immediate_packages: BTreeSet<Symbol> = BTreeSet::new();
 
         for module in &root_modules {
             let immediate_deps = module.module.immediate_dependencies();
@@ -680,34 +682,51 @@ impl CompiledPackage {
                     let Some(pkg_name) = pkg_name else {
                         bail!("Expected a package name but it's None")
                     };
-                    used_immediate_packages.insert(*pkg_name);
+                    pkgs_to_keep.insert(*pkg_name);
                 }
             }
         }
 
-        // Next, for each package from used_immediate_packages set, we need to find all the
-        // transitive dependencies. Those trans dependencies need to be included in the final list
-        // of package dependencies (note that the pkg itself will be added by the transitive deps
-        // function)
-        used_immediate_packages.iter().for_each(|pkg| {
-            self.dependency_graph
-                .add_transitive_dependencies(pkg, &mut pkgs_to_keep)
-        });
-
         // If a package depends on another published package that has only bytecode without source
         // code available, we need to include also that package as dep.
         pkgs_to_keep.extend(self.bytecode_deps.iter().map(|(name, _)| *name));
 
-        // Finally, filter out packages that are not in the union above from the
-        // dependency_ids.published field and return the package ids.
-        self.dependency_ids
+        // Finally, filter out packages that are published and exist in the manifest at the
+        // compilation time but are not referenced in the source code.
+        Ok(self
+            .dependency_ids
+            .clone()
             .published
-            .retain(|pkg_name, _| pkgs_to_keep.contains(pkg_name));
-
-        Ok(())
+            .into_iter()
+            .filter(|(pkg_name, _)| pkgs_to_keep.contains(pkg_name))
+            .collect())
     }
 }
 
+/// Create a set of [Dependencies] from a [SystemPackagesVersion]; the dependencies are override git
+/// dependencies to the specific revision given by the [SystemPackagesVersion]
+pub fn implicit_deps(packages: &SystemPackagesVersion) -> Dependencies {
+    packages
+        .packages
+        .iter()
+        .map(|package| {
+            (
+                package.package_name.clone().into(),
+                Dependency::Internal(InternalDependency {
+                    kind: DependencyKind::Git(GitInfo {
+                        git_url: SYSTEM_GIT_REPO.into(),
+                        git_rev: packages.git_revision.clone().into(),
+                        subdir: package.repo_path.clone().into(),
+                    }),
+                    subst: None,
+                    digest: None,
+                    dep_override: true,
+                }),
+            )
+        })
+        .collect()
+}
+
 impl GetModule for CompiledPackage {
     type Error = anyhow::Error;
     // TODO: return ref here for better efficiency? Borrow checker + all_modules_map() make it hard to do this

crates/sui-move/Cargo.toml

@@ -34,6 +34,7 @@ sui-move-natives = { path = "../../sui-execution/latest/sui-move-natives", packa
 sui-move-build.workspace = true
 sui-protocol-config.workspace = true
 sui-types.workspace = true
+sui-package-management.workspace = true
 better_any = "0.1.1"
 
 [target.'cfg(not(target_env = "msvc"))'.dependencies]

crates/sui-move/src/build.rs

@@ -5,9 +5,9 @@ use crate::manage_package::resolve_lock_file_path;
 use clap::Parser;
 use move_cli::base;
 use move_package::BuildConfig as MoveBuildConfig;
-use serde_json::json;
 use std::{fs, path::Path};
-use sui_move_build::{check_invalid_dependencies, check_unpublished_dependencies, BuildConfig};
+use sui_move_build::{implicit_deps, BuildConfig};
+use sui_package_management::system_package_versions::latest_system_packages;
 
 const LAYOUTS_DIR: &str = "layouts";
 const STRUCT_LAYOUTS_FILENAME: &str = "struct_layouts.yaml";
@@ -52,44 +52,25 @@ impl Build {
         Self::execute_internal(
             &rerooted_path,
             build_config,
-            self.with_unpublished_dependencies,
-            self.dump_bytecode_as_base64,
             self.generate_struct_layouts,
             self.chain_id.clone(),
         )
     }
 
     pub fn execute_internal(
         rerooted_path: &Path,
-        config: MoveBuildConfig,
-        with_unpublished_deps: bool,
-        dump_bytecode_as_base64: bool,
+        mut config: MoveBuildConfig,
         generate_struct_layouts: bool,
         chain_id: Option<String>,
     ) -> anyhow::Result<()> {
-        let mut pkg = BuildConfig {
+        config.implicit_dependencies = implicit_deps(latest_system_packages());
+        let pkg = BuildConfig {
             config,
             run_bytecode_verifier: true,
             print_diags_to_stderr: true,
             chain_id,
         }
         .build(rerooted_path)?;
-        if dump_bytecode_as_base64 {
-            check_invalid_dependencies(&pkg.dependency_ids.invalid)?;
-            if !with_unpublished_deps {
-                check_unpublished_dependencies(&pkg.dependency_ids.unpublished)?;
-            }
-
-            pkg.tree_shake(with_unpublished_deps)?;
-            println!(
-                "{}",
-                json!({
-                    "modules": pkg.get_package_base64(with_unpublished_deps),
-                    "dependencies": pkg.get_dependency_storage_package_ids(),
-                    "digest": pkg.get_package_digest(with_unpublished_deps),
-                })
-            )
-        }
 
         if generate_struct_layouts {
             let layout_str = serde_yaml::to_string(&pkg.generate_struct_layouts()).unwrap();

crates/sui-move/src/new.rs

@@ -6,11 +6,6 @@ use move_cli::base::new;
 use move_package::source_package::layout::SourcePackageLayout;
 use std::{fs::create_dir_all, io::Write, path::Path};
 
-const SUI_PKG_NAME: &str = "Sui";
-
-// Use testnet by default. Probably want to add options to make this configurable later
-const SUI_PKG_PATH: &str = "{ git = \"https://github.com/MystenLabs/sui.git\", subdir = \"crates/sui-framework/packages/sui-framework\", rev = \"framework/testnet\", override = true }";
-
 #[derive(Parser)]
 #[group(id = "sui-move-new")]
 pub struct New {
@@ -24,7 +19,7 @@ impl New {
         let provided_name = &self.new.name.to_string();
 
         self.new
-            .execute(path, [(SUI_PKG_NAME, SUI_PKG_PATH)], [(name, "0x0")], "")?;
+            .execute(path, [] as [(&str, &str); 0], [(name, "0x0")], "")?;
         let p = path.unwrap_or_else(|| Path::new(&provided_name));
         let mut w = std::fs::File::create(
             p.join(SourcePackageLayout::Sources.path())

crates/sui-move/src/unit_test.rs

@@ -11,9 +11,10 @@ use move_unit_test::{extensions::set_extension_hook, UnitTestingConfig};
 use move_vm_runtime::native_extensions::NativeContextExtensions;
 use once_cell::sync::Lazy;
 use std::{cell::RefCell, collections::BTreeMap, path::Path, sync::Arc};
-use sui_move_build::decorate_warnings;
+use sui_move_build::{decorate_warnings, implicit_deps};
 use sui_move_natives::test_scenario::InMemoryTestStore;
 use sui_move_natives::{object_runtime::ObjectRuntime, NativesCostTable};
+use sui_package_management::system_package_versions::latest_system_packages;
 use sui_protocol_config::ProtocolConfig;
 use sui_types::{
     gas_model::tables::initial_cost_schedule_for_unit_tests, in_memory_storage::InMemoryStorage,
@@ -71,7 +72,7 @@ static SET_EXTENSION_HOOK: Lazy<()> =
 /// successfully started running the test, and the inner result indicatests whether all tests pass.
 pub fn run_move_unit_tests(
     path: &Path,
-    build_config: BuildConfig,
+    mut build_config: BuildConfig,
     config: Option<UnitTestingConfig>,
     compute_coverage: bool,
     save_disassembly: bool,
@@ -81,6 +82,7 @@ pub fn run_move_unit_tests(
 
     let config = config
         .unwrap_or_else(|| UnitTestingConfig::default_with_bound(Some(MAX_UNIT_TEST_INSTRUCTIONS)));
+    build_config.implicit_dependencies = implicit_deps(latest_system_packages());
 
     let result = move_cli::base::test::run_move_unit_tests(
         path,

crates/sui-package-management/src/system_package_versions.rs

@@ -16,6 +16,8 @@ static VERSION_TABLE: LazyLock<BTreeMap<ProtocolVersion, SystemPackagesVersion>>
         )))
     });
 
+pub const SYSTEM_GIT_REPO: &str = "https://github.com/MystenLabs/sui.git";
+
 #[derive(Debug)]
 pub struct SystemPackagesVersion {
     pub git_revision: String,

crates/sui-source-validation-service/Cargo.toml

@@ -27,6 +27,7 @@ url = "2.3.1"
 
 sui-move.workspace = true
 sui-move-build.workspace = true
+sui-package-management.workspace = true
 sui-sdk.workspace = true
 sui-source-validation.workspace = true
 

crates/sui-source-validation-service/src/lib.rs

@@ -9,6 +9,7 @@ use std::path::PathBuf;
 use std::sync::{Arc, RwLock};
 use std::time::Duration;
 use std::{ffi::OsString, fs, path::Path, process::Command};
+use sui_package_management::system_package_versions::latest_system_packages;
 use tokio::sync::oneshot::Sender;
 
 use anyhow::{anyhow, bail};
@@ -30,7 +31,7 @@ use move_core_types::account_address::AccountAddress;
 use move_package::{BuildConfig as MoveBuildConfig, LintFlag};
 use move_symbol_pool::Symbol;
 use sui_move::manage_package::resolve_lock_file_path;
-use sui_move_build::{BuildConfig, SuiPackageHooks};
+use sui_move_build::{implicit_deps, BuildConfig, SuiPackageHooks};
 use sui_sdk::rpc_types::SuiTransactionBlockEffects;
 use sui_sdk::types::base_types::ObjectID;
 use sui_sdk::SuiClientBuilder;
@@ -163,6 +164,7 @@ pub async fn verify_package(
         resolve_lock_file_path(MoveBuildConfig::default(), Some(package_path.as_ref()))?;
     config.lint_flag = LintFlag::LEVEL_NONE;
     config.silence_warnings = true;
+    config.implicit_dependencies = implicit_deps(latest_system_packages());
     let build_config = BuildConfig {
         config,
         run_bytecode_verifier: false, /* no need to run verifier if code is on-chain */

crates/sui-source-validation/src/lib.rs

@@ -234,6 +234,12 @@ impl ValidationMode {
                     }
                 })?;
 
+            // only keep modules that are actually used
+            let deps_compiled_units: Vec<_> = deps_compiled_units
+                .into_iter()
+                .filter(|pkg| sui_package.dependency_ids.published.contains_key(&pkg.0))
+                .collect();
+
             for (package, local_unit) in deps_compiled_units {
                 let m = &local_unit.unit;
                 let module = m.name;

crates/sui/Cargo.toml

@@ -98,6 +98,8 @@ move-vm-profiler.workspace = true
 move-vm-config.workspace = true
 move-ir-types.workspace = true
 move-command-line-common.workspace = true
+move-cli.workspace = true
+move-symbol-pool.workspace = true
 
 [target.'cfg(not(target_env = "msvc"))'.dependencies]
 jemalloc-ctl.workspace = true