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.

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

Conversation

messense
Copy link
Member

@messense messense commented Sep 6, 2022

Fixes #1083 by also bundling optional path dependencies.

@netlify
Copy link

netlify bot commented Sep 6, 2022

Deploy Preview for maturin-guide canceled.

Name Link
🔨 Latest commit eb909d5
🔍 Latest deploy log https://app.netlify.com/sites/maturin-guide/deploys/631702aa317037000818f361

@messense
Copy link
Member Author

messense commented Sep 6, 2022

This does not work actually, build from sdist fails

$ cargo build
error: failed to load manifest for dependency `polars`

Caused by:
  failed to load manifest for dependency `polars-algo`

Caused by:
  failed to read `/Users/messense/Projects/maturin/dist/polars-0.14.9/local_dependencies/polars/polars-algo/Cargo.toml`

Caused by:
  No such file or directory (os error 2)

// 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

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

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

@messense messense marked this pull request as ready for review September 6, 2022 08:29
@messense messense merged commit 8a3d060 into PyO3:main Sep 6, 2022
@messense messense deleted the fix-optional-path-deps branch September 6, 2022 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

maturin fails if a local dependency is not released on crates.io
1 participant