Skip to content

Commit

Permalink
Merge pull request #1084 from messense/fix-optional-path-deps
Browse files Browse the repository at this point in the history 
Fix sdist build for optional path dependencies
  • Loading branch information
@messense
messense authored Sep 6, 2022
2 parents 7d92871 + eb909d5 commit 8a3d060
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 76 deletions.
1 change: 1 addition & 0 deletions Changelog.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* Update MSRV to 1.59.0 in [#1071](https://github.com/PyO3/maturin/pull/1071)
* Fix abi3 wheel build when no Python interpreters found in [#1072](https://github.com/PyO3/maturin/pull/1072)
* Add `zig ar` support in [#1073](https://github.com/PyO3/maturin/pull/1073)
* Fix sdist build for optional path dependencies in [#1084](https://github.com/PyO3/maturin/pull/1084)

## [0.13.2] - 2022-08-14

Expand Down
124 changes: 48 additions & 76 deletions src/source_distribution.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::module_writer::{add_data, ModuleWriter};
use crate::{Metadata21, SDistWriter};
use anyhow::{bail, Context, Result};
use cargo_metadata::{Metadata, PackageId};
use cargo_metadata::Metadata;
use fs_err as fs;
use std::collections::HashMap;
use std::path::{Path, PathBuf};
Expand All @@ -10,12 +10,6 @@ use std::str;

const LOCAL_DEPENDENCIES_FOLDER: &str = "local_dependencies";

#[derive(Debug, Clone)]
struct PathDependency {
id: PackageId,
path: PathBuf,
}

/// We need cargo to load the local dependencies from the location where we put them in the source
/// distribution. Since there is no cargo-backed way to replace dependencies
/// (see https://github.com/rust-lang/cargo/issues/9170), we do a simple
Expand All @@ -25,7 +19,7 @@ struct PathDependency {
/// This method is rather frail, but unfortunately I don't know a better solution.
fn rewrite_cargo_toml(
manifest_path: impl AsRef<Path>,
known_path_deps: &HashMap<String, PathDependency>,
known_path_deps: &HashMap<String, PathBuf>,
root_crate: bool,
) -> Result<String> {
let text = fs::read_to_string(&manifest_path).context(format!(
Expand Down Expand Up @@ -58,14 +52,6 @@ fn rewrite_cargo_toml(
dep_name
)
}
// This is the location of the targeted crate in the source distribution
table[&dep_name]["path"] = if root_crate {
toml_edit::value(format!("{}/{}", LOCAL_DEPENDENCIES_FOLDER, dep_name))
} else {
// Cargo.toml contains relative paths, and we're already in LOCAL_DEPENDENCIES_FOLDER
toml_edit::value(format!("../{}", dep_name))
};
rewritten = true;
if !known_path_deps.contains_key(&dep_name) {
bail!(
"cargo metadata does not know about the path for {}.{} present in {}, \
Expand All @@ -75,6 +61,14 @@ fn rewrite_cargo_toml(
manifest_path.as_ref().display()
);
}
// This is the location of the targeted crate in the source distribution
table[&dep_name]["path"] = if root_crate {
toml_edit::value(format!("{}/{}", LOCAL_DEPENDENCIES_FOLDER, dep_name))
} else {
// Cargo.toml contains relative paths, and we're already in LOCAL_DEPENDENCIES_FOLDER
toml_edit::value(format!("../{}", dep_name))
};
rewritten = true;
}
}
}
Expand Down Expand Up @@ -129,19 +123,25 @@ fn add_crate_to_source_distribution(
writer: &mut SDistWriter,
manifest_path: impl AsRef<Path>,
prefix: impl AsRef<Path>,
known_path_deps: &HashMap<String, PathDependency>,
known_path_deps: &HashMap<String, PathBuf>,
root_crate: bool,
) -> Result<()> {
let manifest_path = manifest_path.as_ref();
let output = Command::new("cargo")
.args(&["package", "--list", "--allow-dirty", "--manifest-path"])
.arg(manifest_path)
.output()
.context("Failed to run `cargo package --list --allow-dirty`")?;
.with_context(|| {
format!(
"Failed to run `cargo package --list --allow-dirty --manifest-path {}`",
manifest_path.display()
)
})?;
if !output.status.success() {
bail!(
"Failed to query file list from cargo: {}\n--- Stdout:\n{}\n--- Stderr:\n{}",
"Failed to query file list from cargo: {}\n--- Manifest path: {}\n--- Stdout:\n{}\n--- Stderr:\n{}",
output.status,
manifest_path.display(),
String::from_utf8_lossy(&output.stdout),
String::from_utf8_lossy(&output.stderr),
);
Expand Down Expand Up @@ -199,64 +199,36 @@ fn add_crate_to_source_distribution(
Ok(())
}

/// Get path dependencies for a cargo package
fn get_path_deps(
cargo_metadata: &Metadata,
resolve: &cargo_metadata::Resolve,
pkg_id: &cargo_metadata::PackageId,
visited: &HashMap<String, PathDependency>,
) -> Result<HashMap<String, PathDependency>> {
// Parse ids in the format:
// on unix: some_path_dep 0.1.0 (path+file:///home/konsti/maturin/test-crates/some_path_dep)
// on windows: some_path_dep 0.1.0 (path+file:///C:/konsti/maturin/test-crates/some_path_dep)
// This is not a good way to identify path dependencies, but I don't know a better one
let node = resolve
.nodes
.iter()
.find(|node| &node.id == pkg_id)
.context("Expected to get a node of dependency graph from cargo")?;
let path_deps = node
.deps
.iter()
.filter(|node| node.pkg.repr.contains("path+file://"))
.filter_map(|node| {
cargo_metadata.packages.iter().find_map(|pkg| {
if pkg.id.repr == node.pkg.repr && !visited.contains_key(&pkg.name) {
let path_dep = PathDependency {
id: pkg.id.clone(),
path: PathBuf::from(&pkg.manifest_path),
};
Some((pkg.name.clone(), path_dep))
} else {
None
}
})
})
.collect();
Ok(path_deps)
}

/// Finds all path dependencies of the crate
fn find_path_deps(cargo_metadata: &Metadata) -> Result<HashMap<String, PathDependency>> {
let resolve = cargo_metadata
.resolve
.as_ref()
.context("Expected to get a dependency graph from cargo")?;
let root = resolve
.root
.clone()
.context("Expected to get a root package id of dependency graph from cargo")?;
let mut known_path_deps = HashMap::new();
let mut stack = vec![root];
while let Some(pkg_id) = stack.pop() {
let path_deps = get_path_deps(cargo_metadata, resolve, &pkg_id, &known_path_deps)?;
if path_deps.is_empty() {
continue;
fn find_path_deps(cargo_metadata: &Metadata) -> Result<HashMap<String, PathBuf>> {
let root = cargo_metadata
.root_package()
.context("Expected the dependency graph to have a root package")?;
// scan the dependency graph for path dependencies
let mut path_deps = HashMap::new();
let mut stack: Vec<&cargo_metadata::Package> = vec![root];
while let Some(top) = stack.pop() {
for dependency in &top.dependencies {
if let Some(path) = &dependency.path {
// we search for the respective package by `manifest_path`, there seems
// to be no way to query the dependency graph given `dependency`
let dep_manifest_path = path.join("Cargo.toml");
path_deps.insert(
dependency.name.clone(),
PathBuf::from(dep_manifest_path.clone()),
);
if let Some(dep_package) = cargo_metadata
.packages
.iter()
.find(|package| package.manifest_path == dep_manifest_path)
{
// scan the dependencies of the path dependency
stack.push(dep_package)
}
}
}
stack.extend(path_deps.values().map(|dep| dep.id.clone()));
known_path_deps.extend(path_deps);
}
Ok(known_path_deps)
Ok(path_deps)
}

/// Creates a source distribution, packing the root crate and all local dependencies
Expand Down Expand Up @@ -287,15 +259,15 @@ pub fn source_distribution(
for (name, path_dep) in known_path_deps.iter() {
add_crate_to_source_distribution(
&mut writer,
&path_dep.path,
&path_dep,
&root_dir.join(LOCAL_DEPENDENCIES_FOLDER).join(name),
&known_path_deps,
false,
)
.context(format!(
"Failed to add local dependency {} at {} to the source distribution",
name,
path_dep.path.display()
path_dep.display()
))?;
}

Expand Down

0 comments on commit 8a3d060

Please sign in to comment.