From 21ab1890b89284fb05d38a2464fa5e024e34ce83 Mon Sep 17 00:00:00 2001 From: messense Date: Tue, 6 Sep 2022 11:37:15 +0800 Subject: [PATCH 1/3] Fix sdist build for optional path dependencies --- Changelog.md | 1 + src/source_distribution.rs | 25 +++++++++++++++++-------- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/Changelog.md b/Changelog.md index 015fc098f..5219ae5b2 100644 --- a/Changelog.md +++ b/Changelog.md @@ -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 diff --git a/src/source_distribution.rs b/src/source_distribution.rs index 12bffaf6e..b3109b16c 100644 --- a/src/source_distribution.rs +++ b/src/source_distribution.rs @@ -58,15 +58,16 @@ 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) { + // Ignore optional indirect dependencies + if !root_crate + && table[&dep_name] + .get("optional") + .and_then(|x| x.as_bool()) + .unwrap_or_default() + { + continue; + } bail!( "cargo metadata does not know about the path for {}.{} present in {}, \ which should never happen ಠ_ಠ", @@ -75,6 +76,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; } } } From d1773fc79071d0f18bdfbcf24ef0e8b45653e06c Mon Sep 17 00:00:00 2001 From: messense Date: Tue, 6 Sep 2022 15:42:46 +0800 Subject: [PATCH 2/3] Refactor finding path dependencies logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Maximilian Köhl --- src/source_distribution.rs | 97 ++++++++++++-------------------------- 1 file changed, 31 insertions(+), 66 deletions(-) diff --git a/src/source_distribution.rs b/src/source_distribution.rs index b3109b16c..42ca10a35 100644 --- a/src/source_distribution.rs +++ b/src/source_distribution.rs @@ -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}; @@ -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 @@ -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, - known_path_deps: &HashMap, + known_path_deps: &HashMap, root_crate: bool, ) -> Result { let text = fs::read_to_string(&manifest_path).context(format!( @@ -138,7 +132,7 @@ fn add_crate_to_source_distribution( writer: &mut SDistWriter, manifest_path: impl AsRef, prefix: impl AsRef, - known_path_deps: &HashMap, + known_path_deps: &HashMap, root_crate: bool, ) -> Result<()> { let manifest_path = manifest_path.as_ref(); @@ -208,64 +202,35 @@ 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, -) -> Result> { - // 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> { - 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> { + 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 { + path_deps.insert(dependency.name.clone(), PathBuf::from(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"); + let dep_package = cargo_metadata + .packages + .iter() + .find(|package| package.manifest_path == dep_manifest_path) + .context(format!( + "Expected metadata to contain a package for path dependency {:?}", + 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 @@ -296,7 +261,7 @@ 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, @@ -304,7 +269,7 @@ pub fn source_distribution( .context(format!( "Failed to add local dependency {} at {} to the source distribution", name, - path_dep.path.display() + path_dep.display() ))?; } From eb909d518bc0c5f67eea88e40273bb5fdd2eded7 Mon Sep 17 00:00:00 2001 From: messense Date: Tue, 6 Sep 2022 15:51:55 +0800 Subject: [PATCH 3/3] Fix sdist build for optional path dependencies --- src/source_distribution.rs | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/src/source_distribution.rs b/src/source_distribution.rs index 42ca10a35..5fce30830 100644 --- a/src/source_distribution.rs +++ b/src/source_distribution.rs @@ -53,15 +53,6 @@ fn rewrite_cargo_toml( ) } if !known_path_deps.contains_key(&dep_name) { - // Ignore optional indirect dependencies - if !root_crate - && table[&dep_name] - .get("optional") - .and_then(|x| x.as_bool()) - .unwrap_or_default() - { - continue; - } bail!( "cargo metadata does not know about the path for {}.{} present in {}, \ which should never happen ಠ_ಠ", @@ -140,11 +131,17 @@ fn add_crate_to_source_distribution( .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), ); @@ -213,20 +210,21 @@ fn find_path_deps(cargo_metadata: &Metadata) -> Result> while let Some(top) = stack.pop() { for dependency in &top.dependencies { if let Some(path) = &dependency.path { - path_deps.insert(dependency.name.clone(), PathBuf::from(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"); - let dep_package = cargo_metadata + 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) - .context(format!( - "Expected metadata to contain a package for path dependency {:?}", - path - ))?; - // scan the dependencies of the path dependency - stack.push(dep_package) + { + // scan the dependencies of the path dependency + stack.push(dep_package) + } } } }