Don't package non-path-dep crates in sdist for workspaces

[messense]

Fixes #462

For the included workspace setup, before:

❯ tar -ztf dist/workspace_with_path_dep-0.1.0.tar.gz
workspace_with_path_dep-0.1.0/local_dependencies/generic_lib/Cargo.toml
workspace_with_path_dep-0.1.0/local_dependencies/generic_lib/src/lib.rs
workspace_with_path_dep-0.1.0/local_dependencies/dont_include_in_sdist/Cargo.toml
workspace_with_path_dep-0.1.0/local_dependencies/dont_include_in_sdist/src/main.rs
workspace_with_path_dep-0.1.0/Cargo.toml
workspace_with_path_dep-0.1.0/pyproject.toml
workspace_with_path_dep-0.1.0/src/lib.rs
workspace_with_path_dep-0.1.0/PKG-INFO

after:

❯ tar -ztf dist/workspace_with_path_dep-0.1.0.tar.gz
workspace_with_path_dep-0.1.0/local_dependencies/generic_lib/Cargo.toml
workspace_with_path_dep-0.1.0/local_dependencies/generic_lib/src/lib.rs
workspace_with_path_dep-0.1.0/Cargo.toml
workspace_with_path_dep-0.1.0/pyproject.toml
workspace_with_path_dep-0.1.0/src/lib.rs
workspace_with_path_dep-0.1.0/PKG-INFO

TODO:

• Fix transitive path dependency.

[netlify]

✔️ Deploy Preview for maturin-guide canceled.

🔨 Explore the source changes: b7f18b4

🔍 Inspect the deploy log: https://app.netlify.com/sites/maturin-guide/deploys/61a77bce2781ff000741a31a

[messense]

This one is a little tricky, I'd love to have more eyes on it.

cc @koehlma

[davidhewitt]

👍 happy to review however I don't think I'll manage to get to this before the weekend FYI. My FIFO queue is backed up a bit this week 😅

[koehlma]

Hey, thanks for addressing this.

It seems to me that this approach misses transitive path dependencies. Imagine I have the dependency graph A → B → C and these are all path dependencies. Now, A.deps does contain B but not C because the dependency graph provided by cargo_metadata is transitivity reduced as far as I know. So, after discovering that B is a dependency of A the dependencies of B should be scanned and so on. If I may, I would also suggest factoring out the scanning for path dependencies into its own function.

I once implemented this as follows: https://github.com/koehlma/maturin/blob/master/src/source_distribution.rs#L146 This solution also does not depend on the repr of a package identifier.

[messense]

@koehlma Thanks for the review!

I'm a little worried that this top.dependencies
https://github.com/koehlma/maturin/blob/dee86e60b1771055210d04fff5d8509d80a0fe04/src/source_distribution.rs#L155 doesn’t support renamed dependencies. Not sure if it breaks anything, needs to give it a test.

[koehlma]

@messense My pleasure!

The part of the documentation you quoted is for Node but my code actually uses Package. In case a dependency is renamed, one can access the new name via the rename field of the dependency. This raises an interesting question: What happens if you use a local variant of a crate via renaming and another variant from crates.io? I am not quite sure which names should end up in the HashMap, in particular, this may be inconsistent accross transitive dependencies.

[messense]

I've just added a transitive path dep test crate. The dependency graph is workspace_with_path_dep -> generic_lib -> transitive_lib and these are all path dependencies. The end result looks fine to me?

❯ tar -ztf dist/workspace_with_path_dep-0.1.0.tar.gz
workspace_with_path_dep-0.1.0/local_dependencies/generic_lib/Cargo.toml
workspace_with_path_dep-0.1.0/local_dependencies/generic_lib/src/lib.rs
workspace_with_path_dep-0.1.0/local_dependencies/transitive_lib/Cargo.toml
workspace_with_path_dep-0.1.0/local_dependencies/transitive_lib/src/lib.rs
workspace_with_path_dep-0.1.0/Cargo.toml
workspace_with_path_dep-0.1.0/pyproject.toml
workspace_with_path_dep-0.1.0/src/lib.rs
workspace_with_path_dep-0.1.0/PKG-INFO

[konstin]

LGTM

A test that compares the archive file list would be useful