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 lockfile] rename dependency name field to id and add a separate name field to store manifest name #19387

Merged
merged 5 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
16 changes: 8 additions & 8 deletions crates/sui-core/src/unit_tests/move_package_publish_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,32 +218,32 @@ async fn test_generate_lock_file() {
# @generated by Move, please check-in and do not edit manually.

[move]
version = 2
version = 3
manifest_digest = "4C5606BF71339416027A58BDB5BA2EF2F5E0929FCE98BAB8AFFCBC447AFE3A23"
deps_digest = "3C4103934B1E040BB6B23F1D610B4EF9F2F1166A50A104EADCF77467C004C600"
dependencies = [
{ name = "Examples" },
{ name = "Sui" },
{ id = "Examples", name = "Examples" },
{ id = "Sui", name = "Sui" },
]

[[move.package]]
name = "Examples"
id = "Examples"
source = { local = "../object_basics" }

dependencies = [
{ name = "Sui" },
{ id = "Sui", name = "Sui" },
]

[[move.package]]
name = "MoveStdlib"
id = "MoveStdlib"
source = { local = "../../../../../sui-framework/packages/move-stdlib" }

[[move.package]]
name = "Sui"
id = "Sui"
source = { local = "../../../../../sui-framework/packages/sui-framework" }

dependencies = [
{ name = "MoveStdlib" },
{ id = "MoveStdlib", name = "MoveStdlib" },
]

[move.toolchain-version]
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-source-validation/src/toolchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ use tracing::{debug, info};

pub(crate) const CURRENT_COMPILER_VERSION: &str = env!("CARGO_PKG_VERSION");
const LEGACY_COMPILER_VERSION: &str = CURRENT_COMPILER_VERSION; // TODO: update this when Move 2024 is released
const PRE_TOOLCHAIN_MOVE_LOCK_VERSION: u64 = 0; // Used to detect lockfiles pre-toolchain versioning support
const PRE_TOOLCHAIN_MOVE_LOCK_VERSION: u16 = 0; // Used to detect lockfiles pre-toolchain versioning support
const CANONICAL_UNIX_BINARY_NAME: &str = "sui";
const CANONICAL_WIN_BINARY_NAME: &str = "sui.exe";

Expand Down
2 changes: 1 addition & 1 deletion external-crates/move/Cargo.lock

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

19 changes: 12 additions & 7 deletions external-crates/move/crates/move-package/src/lock_file/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ use super::LockFile;
/// V0: Base version.
/// V1: Adds toolchain versioning support.
/// V2: Adds support for managing addresses on package publish and upgrades.
pub const VERSION: u64 = 2;
/// V3: Renames dependency `name` field to `id` and adds a `name` field to store the name from the manifest.
pub const VERSION: u16 = 3;

/// Table for storing package info under an environment.
const ENV_TABLE_NAME: &str = "env";
Expand All @@ -56,8 +57,8 @@ pub struct Packages {

#[derive(Deserialize)]
pub struct Package {
/// The name of the package (corresponds to the name field from its source manifest).
pub name: String,
/// Package identifier (as resolved by the package hook).
pub id: String,

/// Where to find this dependency. Schema is not described in terms of serde-compatible
/// structs, so it is deserialized into a generic data structure.
Expand All @@ -73,6 +74,9 @@ pub struct Package {

#[derive(Deserialize)]
pub struct Dependency {
/// Package identifier (as resolved by the package hook).
pub id: String,

/// The name of the dependency (corresponds to the key for the dependency in the depending
/// package's source manifest).
pub name: String,
Expand Down Expand Up @@ -110,7 +114,7 @@ pub struct ManagedPackage {

#[derive(Serialize, Deserialize)]
pub struct Header {
pub version: u64,
pub version: u16,
/// A hash of the manifest file content this lock file was generated from computed using SHA-256
/// hashing algorithm.
pub manifest_digest: String,
Expand Down Expand Up @@ -199,9 +203,9 @@ impl Header {
let Schema { move_: header } =
toml::de::from_str::<Schema<Header>>(contents).context("Deserializing lock header")?;

if header.version > VERSION {
if header.version != VERSION {
bail!(
"Lock file format is too new, expected version {} or below, found {}",
"Lock file format mismatch, expected version {}, found {}",
VERSION,
header.version
);
Expand Down Expand Up @@ -252,7 +256,8 @@ pub fn update_dependency_graph(
.as_table_mut()
.ok_or_else(|| anyhow!("Could not find or create move table in Move.lock"))?;

// Update `manifest_digest` and `deps_digest` in `[move]` table section.
// Update `version`, `manifest_digest`, and `deps_digest` in `[move]` table section.
move_table["version"] = value(VERSION as i64);
move_table["manifest_digest"] = value(manifest_digest);
move_table["deps_digest"] = value(deps_digest);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ impl<Progress: Write> DependencyGraphBuilder<Progress> {
let lock_file = File::open(lock_path);
let digest_and_lock_contents = lock_file
.map(|mut lock_file| match schema::Header::read(&mut lock_file) {
Ok(header) if header.version < schema::VERSION => None, // outdated lock file - regenerate
Ok(header) => Some((header.manifest_digest, header.deps_digest, lock_string_opt)),
Err(_) => None, // malformed header - regenerate lock file
})
Expand Down Expand Up @@ -1066,70 +1067,72 @@ impl DependencyGraph {
package_graph.add_node(root_package_id);

for schema::Dependency {
name,
id: dep_id,
name: dep_name,
subst,
digest,
} in packages.root_dependencies.into_iter().flatten()
{
package_graph.add_edge(
root_package_id,
Symbol::from(name.as_str()),
PackageIdentifier::from(dep_id.as_str()),
Dependency {
mode: DependencyMode::Always,
subst: subst.map(parse_substitution).transpose()?,
digest: digest.map(Symbol::from),
dep_override: false,
dep_name: PM::PackageName::from(name),
dep_name: PM::PackageName::from(dep_name),
},
);
}

for schema::Dependency {
name,
id: dep_id,
name: dep_name,
subst,
digest,
} in packages.root_dev_dependencies.into_iter().flatten()
{
package_graph.add_edge(
root_package_id,
Symbol::from(name.as_str()),
PackageIdentifier::from(dep_id.as_str()),
Dependency {
mode: DependencyMode::DevOnly,
subst: subst.map(parse_substitution).transpose()?,
digest: digest.map(Symbol::from),
dep_override: false,
dep_name: PM::PackageName::from(name.as_str()),
dep_name: PM::PackageName::from(dep_name),
},
);
}

// Fill in the remaining dependencies, and the package source information from the lock
// file.
for schema::Package {
name: pkg_name,
id: pkg_id,
source,
version,
dependencies,
dev_dependencies,
} in packages.packages.into_iter().flatten()
{
let pkg_name = PM::PackageName::from(pkg_name.as_str());
let source = parse_dependency(pkg_name.as_str(), source)
.with_context(|| format!("Deserializing dependency '{pkg_name}'"))?;
let pkg_id = PackageIdentifier::from(pkg_id.as_str());
let source = parse_dependency(pkg_id.as_str(), source)
.with_context(|| format!("Deserializing dependency '{pkg_id}'"))?;

let source = match source {
PM::Dependency::Internal(source) => source,
PM::Dependency::External(resolver) => {
bail!("Unexpected dependency '{pkg_name}' resolved externally by '{resolver}'");
bail!("Unexpected dependency '{pkg_id}' resolved externally by '{resolver}'");
}
};

if source.subst.is_some() {
bail!("Unexpected 'addr_subst' in source for '{pkg_name}'")
bail!("Unexpected 'addr_subst' in source for '{pkg_id}'")
amnn marked this conversation as resolved.
Show resolved Hide resolved
}

if source.digest.is_some() {
bail!("Unexpected 'digest' in source for '{pkg_name}'")
bail!("Unexpected 'digest' in source for '{pkg_id}'")
}

let pkg = Package {
Expand All @@ -1138,7 +1141,7 @@ impl DependencyGraph {
version: version.map(Symbol::from),
};

match package_table.entry(pkg_name) {
match package_table.entry(pkg_id) {
Entry::Vacant(entry) => {
entry.insert(pkg);
}
Expand All @@ -1148,22 +1151,23 @@ impl DependencyGraph {
Entry::Occupied(entry) => {
bail!(
"Conflicting dependencies found:\n{0} = {1}\n{0} = {2}",
pkg_name,
pkg_id,
PackageWithResolverTOML(entry.get()),
PackageWithResolverTOML(&pkg),
);
}
};

for schema::Dependency {
id: dep_id,
name: dep_name,
subst,
digest,
} in dependencies.into_iter().flatten()
{
package_graph.add_edge(
pkg_name,
PM::PackageName::from(dep_name.as_str()),
pkg_id,
PackageIdentifier::from(dep_id.as_str()),
Dependency {
mode: DependencyMode::Always,
subst: subst.map(parse_substitution).transpose()?,
Expand All @@ -1175,14 +1179,15 @@ impl DependencyGraph {
}

for schema::Dependency {
id: dep_id,
name: dep_name,
subst,
digest,
} in dev_dependencies.into_iter().flatten()
{
package_graph.add_edge(
pkg_name,
PM::PackageName::from(dep_name.as_str()),
pkg_id,
PackageIdentifier::from(dep_id.as_str()),
Dependency {
mode: DependencyMode::DevOnly,
subst: subst.map(parse_substitution).transpose()?,
Expand Down Expand Up @@ -1232,7 +1237,7 @@ impl DependencyGraph {
for (id, pkg) in &self.package_table {
writeln!(writer, "\n[[move.package]]")?;

writeln!(writer, "name = {}", str_escape(id.as_str())?)?;
writeln!(writer, "id = {}", str_escape(id.as_str())?)?;
writeln!(writer, "source = {}", PackageTOML(pkg))?;
if let Some(version) = &pkg.version {
writeln!(writer, "version = {}", str_escape(version.as_str())?)?;
Expand Down Expand Up @@ -1566,20 +1571,23 @@ impl<'a> fmt::Display for PackageWithResolverTOML<'a> {
impl<'a> fmt::Display for DependencyTOML<'a> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let DependencyTOML(
name,
id,
Dependency {
mode: _,
subst,
digest,
dep_override: _,
dep_name: _,
dep_name,
},
) = self;

f.write_str("{ ")?;

write!(f, "name = ")?;
f.write_str(&str_escape(name.as_str())?)?;
write!(f, "id = ")?;
f.write_str(&str_escape(id.as_str())?)?;

write!(f, ", name = ")?;
f.write_str(&str_escape(dep_name.as_str())?)?;

if let Some(digest) = digest {
write!(f, ", digest = ")?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ fn parse_address_literal(address_str: &str) -> Result<AccountAddress, AccountAdd
AccountAddress::from_hex_literal(address_str)
}

pub fn parse_dependency(dep_name: &str, mut tval: TV) -> Result<PM::Dependency> {
pub fn parse_dependency(dep_id: &str, mut tval: TV) -> Result<PM::Dependency> {
let Some(table) = tval.as_table_mut() else {
bail!("Malformed dependency {}", tval);
};
Expand Down Expand Up @@ -432,7 +432,7 @@ pub fn parse_dependency(dep_name: &str, mut tval: TV) -> Result<PM::Dependency>
.ok_or_else(|| anyhow!("'subdir' not a string"))?,
};

let package_name = Symbol::from(dep_name);
let package_name = Symbol::from(dep_id);

PM::DependencyKind::Custom(PM::CustomDepInfo {
node_url,
Expand Down
Loading
Loading