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

mdgeorge4153 · 2025-02-13T15:17:24Z

Description

This PR enables you to use system packages (e.g. MoveStdlib, Sui, DeepBook) without explicitly adding them to your Move.toml. It works by querying the network during publication / upgrade for the protocol version and using that information to determine the correct version of the system dependencies. During build, the latest known version is used.

Test plan

TODO

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

vercel · 2025-02-13T15:17:29Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 21, 2025 10:32pm

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview		Feb 21, 2025 10:32pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview		Feb 21, 2025 10:32pm

…mework-dep-2

stefan-mysten · 2025-02-21T20:35:24Z

external-crates/move/crates/move-package/tests/test_dependency_graph.rs

@@ -43,6 +43,7 @@ fn no_dep_graph() {
        /* skip_fetch_latest_git_deps */ true,
        std::io::sink(),
        tempfile::tempdir().unwrap().path().to_path_buf(),
+        /* implicit deps */ Dependencies::default(),


Suggested change

/* implicit deps */ Dependencies::default(),

Dependencies::default() /* implicit deps */ ,

stefan-mysten · 2025-02-21T20:37:40Z

crates/sui/src/client_commands.rs

+    let protocol_version = protocol_config.protocol_version;
+
+    build_config.implicit_dependencies = implicit_deps_for_protocol_version(protocol_version)?;


Suggested change

let protocol_version = protocol_config.protocol_version;

build_config.implicit_dependencies = implicit_deps_for_protocol_version(protocol_version)?;

build_config.implicit_dependencies = implicit_deps_for_protocol_version(protocol_config.protocol_version)?;

stefan-mysten · 2025-02-21T20:40:18Z

crates/sui/src/client_commands.rs

+            "[{}]: The network is using protocol version {:?}, which is newer than this binary; \
+            the system packages used for compilation (e.g. MoveStdlib) may be out of date.",
+            "warning".bold().yellow(),


Do we want to include a bit more details here along the lines:

protocol version in the binary, protocol version in the network, including which network is being active

may be out of date is a bit vague, but I think it's fine if we include some suggestion on what could go wrong or what to do.

while this is a warning, I am suggesting to add information on what to do if a user would want to get rid of this warning.

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

stefan-mysten · 2025-02-21T20:47:49Z

From a high level looks good to me, I left one comment about previous tests that use local dependency. I assume that means that this is an explicit dependency, and therefore it is ignored by the implicit deps feature, so nothing needs to happen there.

A nit: not sure if it's too late to try to split this PR into smaller ones, would make it easier to review + rebasing + managing the change with all the tests going on:

one with the changes to remove the default of BuildConfig. This would make it easier to fix the tests and make sure everything runs correctly there.
another PR for the core logic
(optional) another PR for fixing the stuff in the sui-source-validation that we discussed earlier.

stefan-mysten · 2025-02-21T20:50:38Z

crates/sui-move/src/build.rs

        with_unpublished_deps: bool,
        dump_bytecode_as_base64: bool,
        generate_struct_layouts: bool,
        chain_id: Option<String>,
    ) -> anyhow::Result<()> {
+        config.implicit_dependencies = implicit_deps(latest_system_packages());


I am not sure I follow why we call the latest system packages here. Is it because the sui move build is chain-agnostic and does not connect to a network, so we need to get the last known system packages that are available in the binary?

Yes, exactly

Perhaps I'm not following the logic here fully (or maybe this is handled elsewhere?), but I'm somewhat worried about this and the behavior it introduces.

My understanding of what this is doing (and please correct me if I'm wrong) is that this is setting the implicit dependencies to be the system packages at the time the Sui CLI (/binary) is built. However this then fixes the system packages that will be used implicitly to be frozen at build time of the CLI binary, so if someone is using an old(er) Sui CLI to test or publish they will be getting possibly stale dependencies unless they specify the dep explicitly or upgrade their CLI?

stefan-mysten · 2025-02-21T20:52:16Z

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

+            install_dir: Some(install_dir),
+            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:


Not sure I understand this TODO comment.

stefan-mysten · 2025-02-21T20:53:48Z

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

+    })
+}
+
+/// Returns the deps from `resolution_graph` that have no source code


Suggested change

/// Returns the deps from `resolution_graph` that have no source code

/// Returns the bytecode deps from `resolution_graph` that have no source code

stefan-mysten · 2025-02-21T20:57:25Z

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

+fn verify_bytecode(package: &MoveCompiledPackage, fn_info: &FnInfoMap) -> SuiResult<()> {
+    let compiled_modules = package.root_modules_map();
+    let verifier_config = ProtocolConfig::get_for_version(ProtocolVersion::MAX, Chain::Unknown)
+        .verifier_config(/* signing_limits */ None);

-    if run_bytecode_verifier {
-        let verifier_config = ProtocolConfig::get_for_version(ProtocolVersion::MAX, Chain::Unknown)
-            .verifier_config(/* signing_limits */ None);
-
-        let compiled_modules = package.root_modules_map();
-        for m in compiled_modules.iter_modules() {
-            move_bytecode_verifier::verify_module_unmetered(m).map_err(|err| {
-                SuiError::ModuleVerificationFailure {
-                    error: err.to_string(),
-                }
-            })?;
-            sui_bytecode_verifier::sui_verify_module_unmetered(m, &fn_info, &verifier_config)?;
-        }
-        // TODO(https://github.com/MystenLabs/sui/issues/69): Run Move linker
+    for m in compiled_modules.iter_modules() {
+        move_bytecode_verifier::verify_module_unmetered(m).map_err(|err| {
+            SuiError::ModuleVerificationFailure {
+                error: err.to_string(),
+            }
+        })?;
+        sui_bytecode_verifier::sui_verify_module_unmetered(m, fn_info, &verifier_config)?;
    }
+    // TODO(https://github.com/MystenLabs/sui/issues/69): Run Move linker


ditto, might want to have a separate PR just for this change (bundled with the one above)

tzakian

Overall looking good to me! Two main questions/comments:

What are your thoughts on including implicit deps even if some of the members are explicitly mentioned (and just not adding those). This feels like the "least surprising" option to me, but maybe there's a cornercase I'm missing with it.
It seems like we're fixing the versions of the implicit system packages on the Sui side to be the greatest version at the time the CLI was built. This seems like it may lead to a frustrating experience for folks as in order to use a more up-to-date version of the system packages that are provided implicitly is to either explicitly depend on them, or to update their CLI (which with our release cycle I could see being a bit annoying). I'm wondering if using a tagged branch for this (e.g., main, or testnet/mainnet depending on chain identifier possibly?) would allow us to insert the most recent and/or current correct version of the system packages when testing/publishing packages to a given network.

tzakian · 2025-02-21T23:13:38Z

external-crates/move/crates/move-package/src/resolution/dependency_graph.rs

+    /// Additional dependencies on [self.implicit_dependencies] are added to all nodes of the
+    /// returned graph, except for nodes that are themselves in [self.implicit_dependencies] or
+    /// that have an explicit dependency on one of the implicit packages (note that having just one
+    /// explicit dep from a node disables all implicit deps for that node!)


2 questions on this here:

I'm guessing that the expectation is that all transitive dependencies will be explicitly mentioned in the implicit_deps? If so, might be helpful to add as a comment.

Should we only filter out the implicit deps that are explicitly mentioned, but insert the others? I'm just wondering if the semantics here could lead to unexpected behavior of e.g., I'm using some Sui Framework code implicitly in my package (e.g., object), I then add a dep on the MoveStdlib for some reason, and now I no longer have access to my object module even though I didn't mention anything explicitly about it.

tzakian · 2025-02-21T23:22:27Z

external-crates/move/crates/move-package/tests/test_runner.rs

+/// Return the dependencies contained in the file at `path`, if any
+fn load_implicits(path: &Path) -> Dependencies {
+    let deps_toml = fs::read_to_string(path).unwrap_or("# no implicit deps".to_string());
+    toml::from_str(&deps_toml)


IMO I think it's fine to derive Serialize/Deserialize on the types as they are saved off in the OnDiskPackage (since the BuildConfig is currently). We could maybe get away with not deriving it if we add a #[serde(skip, default)] on the field, but as long as we are saving the BuildConfig off in the build it makes sense to me to include the implicit deps in there as well.

I do think using the manifest parser here would be nice though (but don't have super strong opinions on it).

tzakian · 2025-02-21T23:28:04Z

external-crates/move/crates/move-package/tests/test_sources/implicits/README.md

+Expected: `a` should not have any implicit deps because any explicits should
+turn off implicits


Phrasing this in terms of my question in dependency_graph.rs; what if this also included i1 but with I2 = i2'?

tzakian · 2025-02-21T23:37:38Z

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

-    }
+/// 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 {


Does this include other system packages? I would have a preference not to include the deepbook package here (and possibly even the bridge package) as that seems maybe a bit too much. "it's a system package, deal with it" will make me sad, but is an acceptable answer here too...

tzakian · 2025-02-21T23:43:33Z

crates/sui-move/src/build.rs

        with_unpublished_deps: bool,
        dump_bytecode_as_base64: bool,
        generate_struct_layouts: bool,
        chain_id: Option<String>,
    ) -> anyhow::Result<()> {
+        config.implicit_dependencies = implicit_deps(latest_system_packages());


Perhaps I'm not following the logic here fully (or maybe this is handled elsewhere?), but I'm somewhat worried about this and the behavior it introduces.

My understanding of what this is doing (and please correct me if I'm wrong) is that this is setting the implicit dependencies to be the system packages at the time the Sui CLI (/binary) is built. However this then fixes the system packages that will be used implicitly to be frozen at build time of the CLI binary, so if someone is using an old(er) Sui CLI to test or publish they will be getting possibly stale dependencies unless they specify the dep explicitly or upgrade their CLI?

refactored build_from_resolution_graph for readability

bb8e3e4

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 13, 2025 15:17 — with GitHub Actions Inactive

mdgeorge4153 changed the base branch from main to mdgeorge/framework-version-move February 13, 2025 15:18

Started plumbing implicit dependencies into the move side.

53d23d4

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 15, 2025 12:17 — with GitHub Actions Inactive

Adding implicit deps to dependency graph

0ffbbda

github-actions bot added the Type: Documentation Improvements or additions to documentation label Feb 15, 2025

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 15, 2025 12:18 — with GitHub Actions Inactive

mdgeorge4153 changed the base branch from mdgeorge/framework-version-move to main February 15, 2025 12:19

vercel bot deployed to Preview – sui-docs February 15, 2025 12:22 View deployment

mdgeorge4153 added 3 commits February 15, 2025 07:56

Merge remote-tracking branch 'origin/main' into mdgeorge/implicit-fra…

6abd9f3

…mework-dep-2

updated test snapshots to include implicit deps field

9065817

clippy

a05faf9

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 15, 2025 13:14 — with GitHub Actions Inactive

github-actions bot removed the Type: Documentation Improvements or additions to documentation label Feb 15, 2025

vercel bot deployed to Preview – sui-docs February 15, 2025 13:15 View deployment

mdgeorge4153 added 2 commits February 15, 2025 08:30

add implicit_deps to snapshots

fda2a9a

snapshot updates for coloring, which is confusing

7653d44

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 15, 2025 13:34 — with GitHub Actions Inactive

fixing color snapshots

117c42b

vercel bot deployed to Preview – sui-docs February 15, 2025 13:35 View deployment

fixing color snapshots

2d08702

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 15, 2025 13:37 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 15, 2025 13:39 View deployment

mdgeorge4153 added 3 commits February 18, 2025 11:46

only add implicits if there aren't any explicits

167bd22

added implicit dependencies to test runner

43aa308

removed BuildConfig::Default

39d8280

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 19, 2025 15:20 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 19, 2025 15:22 View deployment

removed dep from sui move new

07d39c8

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 20, 2025 22:26 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 20, 2025 22:28 View deployment

mdgeorge4153 requested review from tzakian and stefan-mysten February 21, 2025 19:21

Fixed up some snapshot tests

0323cc9

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 21, 2025 19:27 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 21, 2025 19:28 View deployment

Including implicit deps for sui move test

e58b747

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 21, 2025 19:39 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 21, 2025 19:40 View deployment

fixed snapshot tests

64b5701

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 21, 2025 19:48 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 21, 2025 19:49 View deployment

stefan-mysten reviewed Feb 21, 2025

View reviewed changes

crates/sui-move-build/src/lib.rs Show resolved Hide resolved

stefan-mysten reviewed Feb 21, 2025

View reviewed changes

added more test descriptions

da83e1d

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 21, 2025 21:51 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 21, 2025 21:53 View deployment

removed output from tests with explicit system deps

8774366

mdgeorge4153 temporarily deployed to sui-typescript-aws-kms-test-env February 21, 2025 22:31 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 21, 2025 22:32 View deployment

tzakian reviewed Feb 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mdgeorge4153 commented Feb 13, 2025

vercel bot commented Feb 13, 2025 •

edited

Loading

stefan-mysten Feb 21, 2025

stefan-mysten Feb 21, 2025

stefan-mysten Feb 21, 2025

stefan-mysten commented Feb 21, 2025

stefan-mysten Feb 21, 2025

mdgeorge4153 Feb 21, 2025

tzakian Feb 21, 2025

stefan-mysten Feb 21, 2025

stefan-mysten Feb 21, 2025

stefan-mysten Feb 21, 2025

tzakian left a comment •

edited

Loading

tzakian Feb 21, 2025

tzakian Feb 21, 2025

tzakian Feb 21, 2025

tzakian Feb 21, 2025

tzakian Feb 21, 2025

	/* implicit deps */ Dependencies::default(),
	Dependencies::default() /* implicit deps */ ,

		let protocol_version = protocol_config.protocol_version;

		build_config.implicit_dependencies = implicit_deps_for_protocol_version(protocol_version)?;

	/// Returns the deps from `resolution_graph` that have no source code
	/// Returns the bytecode deps from `resolution_graph` that have no source code

		Expected: `a` should not have any implicit deps because any explicits should
		turn off implicits

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

Are you sure you want to change the base?

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

Conversation

mdgeorge4153 commented Feb 13, 2025

Description

Test plan

Release notes

vercel bot commented Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefan-mysten commented Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzakian left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Feb 13, 2025 •

edited

Loading

tzakian left a comment •

edited

Loading