Skip to content

Commit

Permalink
move lock: preserve existing info when updating dep graph (MystenLabs…
Browse files Browse the repository at this point in the history
…#16793)

## Description 

Ensures `Move.lock` contents persist independent of dependency graph
updates (for `sui move build`, etc.)

## Test Plan 

Added test.

---
If your changes are not user-facing and do not break anything, you can
skip the following section. Otherwise, please briefly describe what has
changed under the Release Notes section.

### 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
  • Loading branch information
rvantonder authored Mar 25, 2024
1 parent 7c10165 commit c02f2f5
Show file tree
Hide file tree
Showing 4 changed files with 122 additions and 16 deletions.
5 changes: 3 additions & 2 deletions external-crates/move/crates/move-package/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,8 @@ impl BuildConfig {
let path = SourcePackageLayout::try_find_root(path)?;
let manifest_string =
std::fs::read_to_string(path.join(SourcePackageLayout::Manifest.path()))?;
let lock_string = std::fs::read_to_string(path.join(SourcePackageLayout::Lock.path())).ok();
let lock_path = path.join(SourcePackageLayout::Lock.path());
let lock_string = std::fs::read_to_string(lock_path.clone()).ok();
let _mutx = PackageLock::lock(); // held until function returns

let install_dir_set = self.install_dir.is_some();
Expand All @@ -292,7 +293,7 @@ impl BuildConfig {
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.
let lock = dependency_graph.write_to_lock(install_dir)?;
let lock = dependency_graph.write_to_lock(install_dir, Some(lock_path))?;
if let Some(lock_path) = &self.lock_file {
lock.commit(lock_path)?;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,8 @@ impl<Progress: Write> DependencyGraphBuilder<Progress> {
)?;
let dep_lock_files = dep_graphs
.values()
.map(|graph_info| graph_info.g.write_to_lock(self.install_dir.clone()))
// write_to_lock should create a fresh lockfile for computing the dependency digest, hence the `None` arg below
.map(|graph_info| graph_info.g.write_to_lock(self.install_dir.clone(), None))
.collect::<Result<Vec<LockFile>>>()?;
let (dev_dep_graphs, dev_resolved_id_deps, dev_dep_names, dev_overrides) = self
.collect_graphs(
Expand All @@ -270,7 +271,8 @@ impl<Progress: Write> DependencyGraphBuilder<Progress> {

let dev_dep_lock_files = dev_dep_graphs
.values()
.map(|graph_info| graph_info.g.write_to_lock(self.install_dir.clone()))
// write_to_lock should create a fresh lockfile for computing the dependency digest, hence the `None` arg below
.map(|graph_info| graph_info.g.write_to_lock(self.install_dir.clone(), None))
.collect::<Result<Vec<LockFile>>>()?;
let new_deps_digest = self.dependency_digest(dep_lock_files, dev_dep_lock_files)?;
let (manifest_digest, deps_digest) = match digest_and_lock_contents {
Expand Down Expand Up @@ -1209,11 +1211,19 @@ impl DependencyGraph {
Ok(graph)
}

/// Serializes this dependency graph into a lock file and return it.
/// Serializes this dependency graph into a lock file and returns it.
///
/// This operation fails, writing nothing, if the graph contains a cycle, and can fail with an
/// undefined output if it cannot be represented in a TOML file.
pub fn write_to_lock(&self, install_dir: PathBuf) -> Result<LockFile> {
///
/// `install_dir` is a working directory to create a lock file with dependency graph info.
/// `lock_path` is an optional parameter: if it is set, and exists, this `Move.lock` will be
/// updated with the dependency graph content. If not, the lock file is created from scratch.
pub fn write_to_lock(
&self,
install_dir: PathBuf,
lock_path: Option<PathBuf>,
) -> Result<LockFile> {
use fmt::Write;
let mut writer = String::new();

Expand Down Expand Up @@ -1249,15 +1259,23 @@ impl DependencyGraph {
.and_then(|v| v.as_array_of_tables().cloned());
}

use std::io::Seek;
let mut lock = LockFile::new(
install_dir,
self.manifest_digest.clone(),
self.deps_digest.clone(),
)?;
lock.flush()?;
lock.rewind()?;

let mut lock = match lock_path {
// Get a handle to update an existing Move.lock. Since dependency graph updates are
// compatible across all Move.lock schema versions, we can rely on the existing version.
Some(lock_path) if lock_path.exists() => LockFile::from(install_dir, &lock_path)?,
// Initialize a lock file if no existing lock_path is set for this operation.
_ => {
use std::io::Seek;
let mut lock = LockFile::new(
install_dir,
self.manifest_digest.clone(),
self.deps_digest.clone(),
)?;
lock.flush()?;
lock.rewind()?;
lock
}
};
schema::update_dependency_graph(
&mut lock,
self.manifest_digest.clone(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ fn lock_file_roundtrip() {
.expect("Reading DependencyGraph");

let lock = graph
.write_to_lock(tmp.path().to_path_buf())
.write_to_lock(tmp.path().to_path_buf(), None)
.expect("Writing DependencyGraph");

lock.commit(&commit).expect("Committing lock file");
Expand Down
87 changes: 87 additions & 0 deletions external-crates/move/crates/move-package/tests/test_lock_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ use move_package::lock_file::schema::{
update_managed_address, ManagedAddressUpdate, ToolchainVersion,
};
use move_package::lock_file::LockFile;
use move_package::resolution::dependency_graph::DependencyGraph;
use move_package::BuildConfig;
use move_symbol_pool::Symbol;

#[test]
fn commit() {
Expand Down Expand Up @@ -67,6 +69,91 @@ fn discard() {
assert!(!pkg.path().join("Move.lock").is_file());
}

#[test]
fn update_lock_file_dependency_graph() {
let pkg = create_test_package().unwrap();

// Create a lock file that will be updated with dependency graph contents.
// The [move.toolchain-version] contents should be retained.
let lock_path = pkg.path().join("Move.lock");
let lock_contents = r#"# @generated by Move, please check-in and do not edit manually.
[move]
version = 1
manifest_digest = "0"
deps_digest = "0"
[move.toolchain-version]
compiler-version = "0.0.0"
edition = "legacy"
flavor = "sui"
"#;
fs::write(lock_path.clone(), lock_contents).unwrap();

// For convenience, create a graph by deserializing from lock contents
let lock_path_with_graph = pkg.path().join("Move.lock.graph");
let lock_graph_contents = r#"
# @generated by Move, please check-in and do not edit manually.
[move]
version = 1
manifest_digest = "0"
deps_digest = "0"
dependencies = [
{ name = "Dep" }
]
[[move.package]]
name = "Dep"
source = { local = "some/path" }
"#;
fs::write(lock_path_with_graph.clone(), lock_graph_contents).unwrap();

let graph = DependencyGraph::read_from_lock(
pkg.path().to_path_buf(),
Symbol::from("Root"),
Symbol::from("Root"),
&mut File::open(&lock_path_with_graph).expect("Open lock file"),
None,
)
.expect("Read DependencyGraph");

let lock = graph
.write_to_lock(pkg.path().to_path_buf(), Some(lock_path.clone()))
.expect("Write DependencyGraph");
lock.commit(&lock_path).expect("Commit lock file");

let mut lock = File::open(&lock_path).expect("Reading lock file");
let contents = {
let mut buf = String::new();
let _ = lock.read_to_string(&mut buf);
buf
};

let expected = expect![[r#"
# @generated by Move, please check-in and do not edit manually.
[move]
version = 1
manifest_digest = "0"
deps_digest = "0"
dependencies = [
{ name = "Dep" },
]
[[move.package]]
name = "Dep"
source = { local = "some/path" }
[move.toolchain-version]
compiler-version = "0.0.0"
edition = "legacy"
flavor = "sui"
"#]];
expected.assert_eq(&contents);
}

#[test]
fn update_lock_file_toolchain_version() {
let pkg = create_test_package().unwrap();
Expand Down

0 comments on commit c02f2f5

Please sign in to comment.