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.

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

Fix sdist build for optional path dependencies #1084

Merged
merged 3 commits into from
Sep 6, 2022
Merged

Fix sdist build for optional path dependencies #1084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21ab189
Fix sdist build for optional path dependencies
messense Sep 6, 2022
d1773fc
Refactor finding path dependencies logic
messense Sep 6, 2022
eb909d5
Fix sdist build for optional path dependencies
messense Sep 6, 2022
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
1 change: 1 addition & 0 deletions Changelog.md
View file
Open in desktop
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
Open in desktop
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) {
Copy link
Member Author

@messense messense Sep 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of #1084 (comment), I think we need to somehow add the optional path deps to known_path_deps even if they're not activated by Cargo features.

Copy link
Member Author

@messense messense Sep 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately there isn't enough metadata:

"dependencies": [
      {
          "name": "polars-algo",
          "source": null,
          "req": "^0.23.0",
          "kind": null,
          "rename": null,
          "optional": true,
          "uses_default_features": true,
          "features": [],
          "target": null,
          "registry": null,
          "path": "/Users/messense/Projects/polars/polars/polars-algo"
      }
]

and the optional polars-algo dep isn't present at packages, thus we can only get its name and path, but we also need it's id which is unavailable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reminds me of #720 (comment), we might be able to workaround this by using dependencies instead of the structured dep.

https://github.com/koehlma/maturin/blob/dee86e60b1771055210d04fff5d8509d80a0fe04/src/source_distribution.rs#L146-L175

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