[DRAFT FOR COMMENTS] move: compile deps with prior compiler binary versions #15245

rvantonder · 2023-12-07T05:25:50Z

Description

We want to enable compiling packages with dependencies that specify prior compiler versions in their Move.lock file (with respective edition and flavor flags). The main thing this does is allow to check byte-for-byte equality of a package build / its dependents against published on-chain code.

Example of the info we store in Move.lock (implemented in #15166):

        [move.toolchain-version]
        compiler-version = "1.16.0"
        edition = "legacy"
        flavor = "sui"

This PR implements an end-to-end package build that will pick up prior compiler-versions for dependencies, download a prior Sui binary, compile them, and "add" them to the build. The (most viable?) approach that seems to enable that is adding deps to the logic in external-crates/move/crates/move-package/src/compilation/compiled_package.rs after compiling with prior versions.

This is the main question for reviewers: does this approach check out? Does this approach violate internal assumptions or impact other build / applications that I may not be aware of?

See inline comments for more.

Test Plan

How did you test the new or updated feature?

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

vercel · 2023-12-07T05:25:59Z

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

Name	Status	Preview	Comments	Updated (UTC)
mysten-ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 7, 2023 5:42am
sui-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 7, 2023 5:42am
sui-typescript-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 7, 2023 5:42am

3 Ignored Deployments

Name	Status	Preview	Updated (UTC)
explorer	⬜️ Ignored (Inspect)	Visit Preview	Dec 7, 2023 5:42am
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Dec 7, 2023 5:42am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Dec 7, 2023 5:42am

…rsions

rvantonder · 2023-12-07T05:34:39Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+        let (deps_for_current_compiler, deps_for_prior_compiler) =
+            partition_deps_by_toolchain(deps_package_paths.clone())?;
+
        let flags = resolution_graph.build_options.compiler_flags();
        // invoke the compiler
-        let mut paths = deps_package_paths.clone();
+        let mut paths = deps_for_current_compiler;
        paths.push(sources_package_paths.clone());

-        let compiler = Compiler::from_package_paths(paths, vec![])
+        let compiler = Compiler::from_package_paths(paths, deps_for_prior_compiler)


Request for input: this is the "main" change. In effect, we compile deps whose Move.lock specify a prior compiler version other than the one we're on. For those deps, we pass their package paths in the second arg to from_package_paths, which is one route I found that appears to work well and completes the build.

One change in the build directory output I observe though, is that, prior to this change, the build directory would include dependency .mv files in build/dependencies. With this change, .mv files of dependencies compiled with a prior compiler do not end up in build/dependencies (and I'm not sure they need to?).

Is it possible to test this change? That is, add a new Move project that uses Move 2024, but relies on a Move Legacy version of code that is already published? This would allow use to ensure the change supports on-chain verification while building correctly.

Also, I think moving the .mv files to build/dependencies/ is a good and worthwhile goal if we can. it will allow us to cache dependencies by-version, etc., in the future.

Is it possible to test this change? That is, add a new Move project that uses Move 2024, but relies on a Move Legacy version of code that is already published? This would allow use to ensure the change supports on-chain verification while building correctly.

I tested locally to get a successful build, and will check on-chain verification with a proper PR. This PR was meant to get a sense of how to do the build, which I think is ~mostly clear to me now.

Also, I think moving the .mv files to build/dependencies/ is a good and worthwhile goal if we can. it will allow us to cache dependencies by-version, etc., in the future.

Right, so this is the kind of thing that I was a bit unsure about re how the internals work when build/dependencies exist. I.e., if we copy / move .mv to build/dependencies would caching "just work"?

rvantonder · 2023-12-07T05:38:00Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+    let dest_binary_os = OsStr::new(dest_binary.as_path());
+
+    if !dest_binary.exists() {
+        // Download if binary does not exist.


I'm just using curl/tar to test this end-to-end, it can and will probably be replaced with native Rust reqwest, etc.

rvantonder · 2023-12-07T05:39:30Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+            OsStr::new(root.as_path()),
+        ])
+        .output()
+        .expect("failed to build package");


.expect just a shortcut for end-to-end testing, will format proper errors after comments

rvantonder · 2023-12-07T05:41:42Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+        if !lock_file.exists() {
+            // Q: Behavior to pick when package has no lock file. Right now we choose the current compiler (we could use the "legacy" one instead).
+            deps_for_current_compiler.push(dep);
+            continue;
+        }


Implementation detail: which compiler version makes sense to pick when there is no lock file (which may be the case for existing packages). My intuition is we go with the latest / current one.

I think that makes sense? Though I don't fully understand under what situations we don't have a lock file

I think any time we don't have a lock file, we should generate one? And if that is the case, yes, we should use the latest.

Yep, we generate lock files if they don't exist.

situations we don't have a lock file

People might not check it in on a repo

rvantonder · 2023-12-07T05:42:05Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+            None => {
+                // Q: Behavior to pick when package has no [move.toolchain-version] info. Right now we choose the current compiler (we could use the "legacy" one instead).
+                deps_for_current_compiler.push(dep)
+            }


Implementation detail: ditto for when there's no toolchain version info.

Can we fix the version for what version of the lock file added this feature? (which I assume is this one?)

tnowacki · 2023-12-14T22:32:20Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+        if !lock_file.exists() {
+            // Q: Behavior to pick when package has no lock file. Right now we choose the current compiler (we could use the "legacy" one instead).
+            deps_for_current_compiler.push(dep);
+            continue;
+        }


I think that makes sense? Though I don't fully understand under what situations we don't have a lock file

tnowacki · 2023-12-14T22:33:04Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+            None => {
+                // Q: Behavior to pick when package has no [move.toolchain-version] info. Right now we choose the current compiler (we could use the "legacy" one instead).
+                deps_for_current_compiler.push(dep)
+            }


Can we fix the version for what version of the lock file added this feature? (which I assume is this one?)

tnowacki · 2023-12-14T22:34:05Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+            }
+            Some(ToolchainVersion {
+                compiler_version, ..
+            }) if compiler_version == env!("CARGO_PKG_VERSION") => {


Do we want to parse or format these at all?
I suppose it is not critical, but longterm we might need to if the format changes here.

tnowacki · 2023-12-14T22:34:53Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+                // Compile and mark that we compiled this dep with a prior compiler.
+                download_and_compile(root, toolchain_version)?;
+                deps_for_prior_compiler.push(dep)


Should we make this a map? Like collect all deps for version X and compile them all at once.
Or is it not worth it?

Echoing this suggestion: assemble each dep by what version it wants, as Map<Version, Vec<Dep>> or similar. While it likely isn't necessary for this logic to work, it may allow future extensions (blacklisting versions, allowing users to filter versions, etc.)

Noted! I think it makes sense to keep a Map that groups by version, but for V0 of the implementation, in the interest of speed, will probably leave "compile all at once by version" on the table for a second iteration.

tnowacki · 2023-12-14T22:35:07Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+            }
+            Some(ToolchainVersion {
+                compiler_version, ..
+            }) if compiler_version == env!("CARGO_PKG_VERSION") => {


Nit, bind this constant somewhere

tnowacki · 2023-12-14T22:35:16Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+        flavor,
+    }: ToolchainVersion,
+) -> Result<()> {
+    let binaries_path = std::env::var("TOOLCHAIN_BINARIES")?; // E.g., ~/.move/binaries


nit bind this constant somewhere

tnowacki · 2023-12-14T22:37:15Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+
+    if !dest_binary.exists() {
+        // Download if binary does not exist.
+        let url = format!("https://github.com/MystenLabs/sui/releases/download/mainnet-v{}/sui-mainnet-v{}-{}.tgz", compiler_version, compiler_version, platform);


We should probably extract out this and the path information somewhere

tnowacki · 2023-12-14T22:38:16Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

+    }
+
+    println!(
+        "[+] sui move build --default-move-edition {} --default-move-flavor {} -p {}",


I worry about these flags here just in case we accidentally break them. I mean... we shouldn't but we might. Thoughts?

IMO flag shifting is a real possibility. Perhaps a file that defines invocations per-version?

We'll need (a different?) way to build if we can't rely on flags--I'm not aware of another way right now if we're invoking the binaries? I'd love if we can have a high degree of confidence about flags, at least for medium to long term (6+ months?). That way, if they do change, one way to approach this is to have version detection logic added in new releases, that case out and use appropriate flags. Basically, build it into this logic.

Perhaps a file that defines invocations per-version?

Proposal for what this would look like? (Will this mean another file users need to check in, and do we really want that?)

cgswords · 2023-12-16T00:07:01Z

external-crates/move/crates/move-package/src/compilation/compiled_package.rs

@@ -756,3 +765,123 @@ pub(crate) fn make_source_and_deps_for_compiler(
    };
    Ok((source_package_paths, deps_package_paths))
 }
+
+/// partitions `deps` by whether we need to compile dependent packages with a


Can you explain if we handle this case?

A requires B at version 0.1.0

A requires C at version 0.2.0

B requires C at version 0.1.0

If not, I do think we need a map of versions to modules to compile, so any individual module can find its dependencies at the right version.

Based on how we currently resolve dependencies, I'm not sure we can / want to / need to support this case.

If packages in the dependency tree require C at different versions (more plainly, if different C dependencies are specified in Move.tomls based on package name) we detect a conflict. So my understanding based on how we do resolution (which is not entirely complete :P) is that if C exists in the dependency graph it can only be one version.

cgswords

This seems like a good initial draft, modulo some concerns about dependency compilation in the limit and generality.

I'd really like to see an actual test on the diff.

rvantonder

Thanks for the comments @tnowacki @cgswords! I think I have enough of a sense to go ahead with a PR for merging, with proper tests, etc. so will do that. Feel free to tack on more thoughts in the comments though!

rvantonder changed the title ~~[DRAFT FOR COMMENTS] move: compile deps with prior compiler binary ve…~~ [DRAFT FOR COMMENTS] move: compile deps with prior compiler binary versions Dec 7, 2023

vercel bot deployed to Preview – sui-typescript-docs December 7, 2023 05:26 View deployment

vercel bot deployed to Preview – sui-core December 7, 2023 05:26 View deployment

vercel bot deployed to Preview – mysten-ui December 7, 2023 05:26 View deployment

rvantonder force-pushed the rvt/compile-toolchain-xyz branch from 7af1148 to ec4bb12 Compare December 7, 2023 05:33

vercel bot deployed to Preview – sui-typescript-docs December 7, 2023 05:34 View deployment

rvantonder force-pushed the rvt/compile-toolchain-xyz branch from ec4bb12 to aff2ab5 Compare December 7, 2023 05:34

vercel bot deployed to Preview – sui-typescript-docs December 7, 2023 05:34 View deployment

vercel bot deployed to Preview – sui-core December 7, 2023 05:35 View deployment

vercel bot deployed to Preview – mysten-ui December 7, 2023 05:35 View deployment

[DRAFT FOR COMMENTS] move: compile deps with prior compiler binary ve…

699ddbb

…rsions

rvantonder force-pushed the rvt/compile-toolchain-xyz branch from aff2ab5 to 699ddbb Compare December 7, 2023 05:41

vercel bot deployed to Preview – sui-typescript-docs December 7, 2023 05:42 View deployment

vercel bot deployed to Preview – sui-core December 7, 2023 05:42 View deployment

vercel bot deployed to Preview – mysten-ui December 7, 2023 05:42 View deployment

rvantonder commented Dec 7, 2023

View reviewed changes

rvantonder mentioned this pull request Dec 7, 2023

[move-package] Remove version fields from package and dependency manifest sections #15148

Merged

7 tasks

tnowacki reviewed Dec 14, 2023

View reviewed changes

tnowacki requested review from amnn, cgswords, awelc, tnowacki and tzakian December 14, 2023 22:38

cgswords reviewed Dec 16, 2023

View reviewed changes

rvantonder commented Dec 16, 2023

View reviewed changes

rvantonder closed this Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT FOR COMMENTS] move: compile deps with prior compiler binary versions #15245

[DRAFT FOR COMMENTS] move: compile deps with prior compiler binary versions #15245

rvantonder commented Dec 7, 2023 •

edited

Loading

vercel bot commented Dec 7, 2023 •

edited

Loading

rvantonder Dec 7, 2023

cgswords Dec 15, 2023

cgswords Dec 16, 2023

rvantonder Dec 16, 2023

rvantonder Dec 7, 2023

rvantonder Dec 7, 2023

rvantonder Dec 7, 2023

tnowacki Dec 14, 2023

cgswords Dec 15, 2023

rvantonder Dec 16, 2023

rvantonder Dec 7, 2023

tnowacki Dec 14, 2023

tnowacki Dec 14, 2023

tnowacki Dec 14, 2023

tnowacki Dec 14, 2023

tnowacki Dec 14, 2023

cgswords Dec 16, 2023

rvantonder Dec 16, 2023

tnowacki Dec 14, 2023

tnowacki Dec 14, 2023

tnowacki Dec 14, 2023

tnowacki Dec 14, 2023

cgswords Dec 16, 2023

rvantonder Dec 16, 2023

cgswords Dec 16, 2023

rvantonder Dec 22, 2023

cgswords left a comment •

edited

Loading

rvantonder left a comment

[DRAFT FOR COMMENTS] move: compile deps with prior compiler binary versions #15245

[DRAFT FOR COMMENTS] move: compile deps with prior compiler binary versions #15245

Conversation

rvantonder commented Dec 7, 2023 • edited Loading

Description

Test Plan

Type of Change (Check all that apply)

Release notes

vercel bot commented Dec 7, 2023 • 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgswords left a comment • edited Loading

Choose a reason for hiding this comment

rvantonder left a comment

Choose a reason for hiding this comment

rvantonder commented Dec 7, 2023 •

edited

Loading

vercel bot commented Dec 7, 2023 •

edited

Loading

cgswords left a comment •

edited

Loading