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 1 commit
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
25 changes: 17 additions & 8 deletions src/source_distribution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
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

// 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 ಠ_ಠ",
Expand All @@ -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;
}
}
}
Expand Down