Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move: build writes toolchain version to Move.lock #15166

Merged
merged 1 commit into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ members = [
]

[workspace.package]
# This version string will be inherited by sui-core, sui-faucet, sui-node, sui-tools, sui-sdk, and sui crates.
# This version string will be inherited by sui-core, sui-faucet, sui-node, sui-tools, sui-sdk, sui-move-build, and sui crates.
version = "1.16.0"

[profile.release]
Expand Down
13 changes: 12 additions & 1 deletion crates/sui-core/src/unit_tests/move_package_publish_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,14 @@ async fn test_generate_lock_file() {
let mut build_config = BuildConfig::new_for_testing();
build_config.config.lock_file = Some(lock_file_path.clone());
build_config
.build(path)
.clone()
.build(path.clone())
.expect("Move package did not build");
// Update the lock file with placeholder compiler version so this isn't bumped every release.
build_config
.config
.update_lock_file_toolchain_version(&path, "0.0.1".into())
.expect("Could not update lock file");

let mut lock_file_contents = String::new();
File::open(lock_file_path)
Expand Down Expand Up @@ -240,6 +246,11 @@ async fn test_generate_lock_file() {
dependencies = [
{ name = "MoveStdlib" },
]

[move.toolchain-version]
compiler-version = "0.0.1"
edition = "legacy"
flavor = "sui"
"#]];
expected.assert_eq(lock_file_contents.as_str());
}
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-move-build/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "sui-move-build"
version = "0.0.0"
version.workspace = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exposes the binary / root crate version in env!("CARGO_PKG_VERSION")

edition = "2021"
authors = ["Mysten Labs <eng@mystenlabs.com>"]
description = "Logic for building Sui Move Packages"
Expand Down
17 changes: 14 additions & 3 deletions crates/sui-move-build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,23 @@ impl BuildConfig {
let print_diags_to_stderr = self.print_diags_to_stderr;
let run_bytecode_verifier = self.run_bytecode_verifier;
let resolution_graph = self.resolution_graph(&path)?;
build_from_resolution_graph(
path,
let result = build_from_resolution_graph(
path.clone(),
resolution_graph,
run_bytecode_verifier,
print_diags_to_stderr,
)
);
if let Ok(ref compiled) = result {
compiled
.package
.compiled_package_info
.build_flags
.update_lock_file_toolchain_version(&path, env!("CARGO_PKG_VERSION").into())
.map_err(|e| SuiError::ModuleBuildFailure {
error: format!("Failed to update Move.lock toolchain version: {e}"),
})?;
}
Comment on lines +154 to +163
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main change.

This is the best place to write lock file logic right now. It is the top level function for source verification and should have all the context we care about before building.

I say "right now" because I'd love to flatten things and have these build steps happen in the external-crates move-package, in one place, which we can do at a later stage.

result
}

pub fn resolution_graph(mut self, path: &Path) -> SuiResult<ResolvedGraph> {
Expand Down
16 changes: 10 additions & 6 deletions external-crates/move/crates/move-package/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ impl BuildConfig {
let lock_string = std::fs::read_to_string(path.join(SourcePackageLayout::Lock.path())).ok();
let _mutx = PackageLock::lock(); // held until function returns

let install_dir_set = self.install_dir.is_some();
let install_dir = self.install_dir.as_ref().unwrap_or(&path).to_owned();

let mut dep_graph_builder = DependencyGraphBuilder::new(
Expand All @@ -190,7 +191,9 @@ impl BuildConfig {
lock_string,
)?;

if modified {
if modified || install_dir_set {
// (1) Write the Move.lock file if the existing one is `modified`, or
// (2) `install_dir` is set explicitly, which may be a different directory, and where a Move.lock does not exist yet.
Comment on lines +194 to +196
Copy link
Contributor Author

@rvantonder rvantonder Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addresses what I consider a bug: this block writes Move.lock to install_dir when it is modified. If install_dir is set to something other than the current directory (let's say /tmp/foo) and run build, then Move.lock would only be written to install_dir if Move.lock changed. So /tmp/foo would be empty if we build again and a Move.lock exists and is the same as before, but if Move.lock does not exist or changed, then it would be written to /tmp/foo.

We should be writing to /tmp/foo if it install_dir is set (i.e., ensure Move.lock is written when we're not on the default package path). Tests helpfully rely on checking the Move.lock file in install_dir (1) so if the Move.lock doesn't exist there, things break.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasonable! We should arguably be also looking in the install dir for the lock file that DependencyGraph reads as well, to completely fix this bug, but the difference in behaviour in what you have here vs that is likely an un-exercised edge case, so all good.

let lock = dependency_graph.write_to_lock(install_dir)?;
if let Some(lock_path) = &self.lock_file {
lock.commit(lock_path)?;
Expand Down Expand Up @@ -222,14 +225,15 @@ impl BuildConfig {
.set_silence_warnings(self.silence_warnings)
}

pub fn update_lock_file_toolchain_version(&self, compiler_version: String) -> Result<()> {
pub fn update_lock_file_toolchain_version(
&self,
path: &PathBuf,
compiler_version: String,
) -> Result<()> {
let Some(lock_file) = self.lock_file.as_ref() else {
return Ok(());
};
let install_dir = self
.install_dir
.clone()
.unwrap_or_else(|| PathBuf::from("."));
let install_dir = self.install_dir.as_ref().unwrap_or(path).to_owned();
let mut lock = LockFile::from(install_dir, lock_file)?;
lock.seek(SeekFrom::Start(0))?;
update_compiler_toolchain(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
//! [move] table). This module does not support serialization because of limitations in the `toml`
//! crate related to serializing types as inline tables.

use std::io::{Read, Seek, SeekFrom, Write};
use std::io::{Read, Seek, Write};

use anyhow::{bail, Context, Result};
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -182,7 +182,6 @@ pub fn update_compiler_toolchain(
) -> Result<()> {
let mut toml_string = String::new();
file.read_to_string(&mut toml_string)?;
file.seek(SeekFrom::Start(0))?;
let mut toml = toml_string.parse::<toml_edit::Document>()?;
let move_table = toml["move"].as_table_mut().ok_or(std::fmt::Error)?;
let toolchain_version = toml::Value::try_from(ToolchainVersion {
Expand All @@ -191,6 +190,8 @@ pub fn update_compiler_toolchain(
flavor,
})?;
move_table["toolchain-version"] = to_toml_edit_value(&toolchain_version);
file.set_len(0)?;
file.rewind()?;
Comment on lines +193 to +194
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small fix: we need to truncate the file to erase all previous content when updating these values, now covered by test.

write!(file, "{}", toml)?;
file.flush()?;
Ok(())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ fn update_lock_file_toolchain_version() {
build_config.default_flavor = Some(Flavor::Sui);
build_config.default_edition = Some(Edition::E2024_ALPHA);
build_config.lock_file = Some(lock_path.clone());
let _ = build_config.update_lock_file_toolchain_version("0.0.1".into());
let _ =
build_config.update_lock_file_toolchain_version(&pkg.path().to_path_buf(), "0.0.1".into());

let mut lock_file = File::open(lock_path).unwrap();
let toolchain_version =
Expand Down
Loading