Skip to content

move: external resolver reads multiple lock contents #19057

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 27, 2024
Merged

move: external resolver reads multiple lock contents #19057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
174 changes: 106 additions & 68 deletions external-crates/move/crates/move-package/src/resolution/dependency_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ use anyhow::{bail, Context, Result};
use colored::Colorize;
use move_symbol_pool::Symbol;
use petgraph::{algo, prelude::DiGraphMap, Direction};

use std::io::BufRead;
use std::{
collections::{btree_map::Entry, BTreeMap, BTreeSet, VecDeque},
fmt,
fs::File,
io::{Read, Write},
io::{BufReader, Read, Write},
path::{Path, PathBuf},
process::Command,
};
Expand Down Expand Up @@ -376,8 +378,9 @@ impl<Progress: Write> DependencyGraphBuilder<Progress> {
let mut resolved_id_deps = BTreeMap::new();
let mut dep_orig_names = BTreeMap::new();
let mut overrides = BTreeMap::new();

for (dep_pkg_name, dep) in dependencies {
let (pkg_graph, is_override, is_external, resolved_pkg_id, resolved_version) = self
let new_deps = self
.new_for_dep(
parent,
&dep,
Expand All @@ -393,34 +396,38 @@ impl<Progress: Write> DependencyGraphBuilder<Progress> {
parent_pkg_name
)
})?;
dep_graphs.insert(
resolved_pkg_id,
DependencyGraphInfo::new(
pkg_graph,
mode,
is_override,
is_external,
resolved_version,
),
);
resolved_id_deps.insert(resolved_pkg_id, dep.clone());
dep_orig_names.insert(resolved_pkg_id, dep_pkg_name);

if is_override {
let kind = match dep {
PM::Dependency::Internal(d) => d.kind,
PM::Dependency::External(_) => {
// external dependencies cannot be overrides
panic!("Unexpected external dependency override")
}
};
let mut dep_pkg = Package {
kind,
resolver: None,
version: resolved_version,
};
dep_pkg.kind.reroot(parent)?;
overrides.insert(resolved_pkg_id, dep_pkg);

for (pkg_graph, is_override, is_external, resolved_pkg_id, resolved_version) in new_deps
{
dep_graphs.insert(
resolved_pkg_id,
DependencyGraphInfo::new(
pkg_graph,
mode,
is_override,
is_external,
resolved_version,
),
);
resolved_id_deps.insert(resolved_pkg_id, dep.clone());
dep_orig_names.insert(resolved_pkg_id, dep_pkg_name);

if is_override {
let kind = match dep {
PM::Dependency::Internal(ref d) => d.kind.clone(),
PM::Dependency::External(_) => {
// external dependencies cannot be overrides
panic!("Unexpected external dependency override")
}
};
let mut dep_pkg = Package {
kind,
resolver: None,
version: resolved_version,
};
dep_pkg.kind.reroot(parent)?;
overrides.insert(resolved_pkg_id, dep_pkg);
}
}
}
Ok((dep_graphs, resolved_id_deps, dep_orig_names, overrides))
Expand All @@ -436,8 +443,8 @@ impl<Progress: Write> DependencyGraphBuilder<Progress> {
parent_pkg_name: PM::PackageName,
dep_pkg_name: PM::PackageName,
dep_pkg_path: PathBuf,
) -> Result<(DependencyGraph, bool, bool, Symbol, Option<Symbol>)> {
let (pkg_graph, is_override, is_external, resolved_pkg_name, resolved_version) = match dep {
) -> Result<Vec<(DependencyGraph, bool, bool, Symbol, Option<Symbol>)>> {
match dep {
PM::Dependency::Internal(d) => {
self.dependency_cache
.download_and_update_if_remote(dep_pkg_name, &d.kind, &mut self.progress_output)
Expand Down Expand Up @@ -482,16 +489,16 @@ impl<Progress: Write> DependencyGraphBuilder<Progress> {
p.kind.reroot(&d.kind)?;
}
}
(
Ok(vec![(
pkg_graph,
d.dep_override,
false,
resolved_pkg_id,
resolved_version,
)
)])
}
PM::Dependency::External(resolver) => {
let pkg_graph = DependencyGraph::get_external(
let external_deps = DependencyGraph::get_external(
mode,
parent_pkg_id,
parent_pkg_name,
Expand All @@ -500,18 +507,15 @@ impl<Progress: Write> DependencyGraphBuilder<Progress> {
&dep_pkg_path,
&mut self.progress_output,
)?;
// TODO: support resolved_pkg_name and resolved_version for
// externally resolved deps.
(pkg_graph, false, true, dep_pkg_name, None)

Ok(external_deps
.into_iter()
.map(|(pkg_graph, _, resolved_pkg_name)| {
(pkg_graph, false, true, resolved_pkg_name, None)
})
.collect())
}
};
Ok((
pkg_graph,
is_override,
is_external,
resolved_pkg_name,
resolved_version,
))
}
}

/// Computes dependency hashes.
Expand Down Expand Up @@ -960,9 +964,8 @@ impl DependencyGraph {
Ok(true)
}
PM::Dependency::External(_) => {
// the way that external graphs are constructed, edges between the (root) package of
// the outer graph and dependencies in the sub-graph are already present in the
// sub-graph
// External dependencies exist in the subgraph of the root package and are added to
// the package_graph as such.
let d = sub_graph
.package_graph
.edge_weight(self.root_package_id, dep_pkg_id)
Expand Down Expand Up @@ -1355,13 +1358,15 @@ impl DependencyGraph {
.map(|(_, dep_name, dep)| (dep_name, dep, &self.package_table[&dep_name]))
}

/// Resolves the packages described at dependency `to` of package `from` with manifest at path
/// `package_path` by running the binary `resolver. `mode` decides whether the resulting
/// packages are added to `self` as dependencies of `package_name` or dev-dependencies.
/// Resolves the packages described at dependency `to` the dependency specified by`from`
/// with manifest at path `package_path` by running the binary `resolver`. `mode`
/// decides whether the resulting packages are added to `self` as dependencies of
/// `package_name` or dev-dependencies.
///
/// Sends progress updates to `progress_output`, including stderr from the resolver, and
/// captures stdout, which is assumed to be a lock file containing the result of package
/// resolution.
/// captures stdout. The output is expected to be one or more null-separated string content.
/// Each string is interpreted as a dependency graph, as represented by the Move.lock TOML
/// schema. It returns each subgraph to be merged into the whole program dependency graph.
fn get_external<Progress: Write>(
mode: DependencyMode,
from_id: PackageIdentifier,
Expand All @@ -1370,7 +1375,7 @@ impl DependencyGraph {
resolver: Symbol,
package_path: &Path,
progress_output: &mut Progress,
) -> Result<DependencyGraph> {
) -> Result<Vec<(DependencyGraph, PM::Dependency, PM::PackageName)>> {
let mode_label = if mode == DependencyMode::DevOnly {
"dev-dependencies"
} else {
Expand All @@ -1389,6 +1394,7 @@ impl DependencyGraph {
)?;

// Call out to the external resolver
// TODO(optimization): this will collect all stdout in memory, but can be streamed instead.
let output = Command::new(resolver.as_str())
.arg(format!("--resolve-move-{mode_label}"))
.arg(to_name.as_str())
Expand Down Expand Up @@ -1416,20 +1422,52 @@ impl DependencyGraph {
}
}

let sub_graph = DependencyGraph::read_from_lock(
package_path.to_path_buf(),
from_id,
from_name,
&mut output.stdout.as_slice(),
Some(resolver),
)
.with_context(|| {
format!(
"Parsing response from '{resolver}' for dependency '{to_name}' of package '{from_id}'"
)
})?;
let mut result = Vec::new();
let mut reader = BufReader::new(output.stdout.as_slice());
let mut buffer = Vec::new();
// Loop over null-separated lock file contents, creating the graph and adding it to the result.
loop {
match reader.read_until(0, &mut buffer) {
Ok(0) => break, // EOF
Ok(_) => {
// Remove the null byte if it's present
if buffer.last() == Some(&0) {
buffer.pop();
}

let sub_graph = DependencyGraph::read_from_lock(
package_path.to_path_buf(),
from_id,
from_name,
&mut buffer.as_slice(),
Some(resolver),
).with_context(|| {
format!("Parsing response from '{resolver}' for dependency '{to_name}' of package '{from_id}'")
})?;

let mut root_sub_package = None;
for (_from_package, to_pkg, _) in sub_graph.package_graph.edges(from_id) {
if root_sub_package.is_some() {
// TODO: We can in fact allow allow multiple root packages / graphs and relax this constraint.
bail!("Multiple root packages in external dependencies found but expected only one");
}
root_sub_package = Some(to_pkg);
}
let Some(root_sub_package) = root_sub_package else {
bail!(
"Expected a root package in external dependencies group but none found"
);
};

let new_dep = PM::Dependency::External(root_sub_package);
result.push((sub_graph, new_dep, root_sub_package));
buffer.clear();
}
Err(e) => return Err(e.into()),
}
}

Ok(sub_graph)
Ok(result)
}

/// Checks that every dependency in the graph, excluding the root package, is present in the
Expand Down
18 changes: 18 additions & 0 deletions ...s/move/crates/move-package/tests/test_sources/external_package_batch_response/Move.locked
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# @generated by Move, please check-in and do not edit manually.

[move]
version = 3
manifest_digest = "C998DF0F473DF6AB3E7B06E0E596FDE9836950D07EDED905464AA1337F176738"
deps_digest = "3C4103934B1E040BB6B23F1D610B4EF9F2F1166A50A104EADCF77467C004C600"
dependencies = [
{ id = "bar", name = "bar" },
{ id = "foo", name = "foo" },
]

[[move.package]]
id = "bar"
source = { local = "deps_only/bar" }

[[move.package]]
id = "foo"
source = { local = "deps_only/foo" }
6 changes: 6 additions & 0 deletions ...move/crates/move-package/tests/test_sources/external_package_batch_response/Move.progress
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
RESOLVING DEPENDENCIES IN Anything FROM Root WITH ../resolvers/successful_package_batch_response.sh
../resolvers/successful_package_batch_response.sh stderr:
Successful External Resolver
PWD: $ROOT/external-crates/move/crates/move-package/tests/test_sources/external_package_batch_response
Type: dependencies
Package: Anything
Loading
Loading