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

Building sdist fails if a crate in workspace isn't used #2117

Closed
1 of 2 tasks
ritchie46 opened this issue Jun 23, 2024 · 9 comments · Fixed by #2215
Closed
1 of 2 tasks

Building sdist fails if a crate in workspace isn't used #2117

ritchie46 opened this issue Jun 23, 2024 · 9 comments · Fixed by #2215
Labels
bug Something isn't working sdist Source distribution

Comments

@ritchie46
Copy link

Bug Description

We have a crate polars-stream, which is in the workspace but not yet used in the python package of polars. However, having it in the workspace seems to break building the dist.

Your maturin version (maturin --version)

1.6

Your Python version (python -V)

unrelated

Your pip version (pip -V)

unrelated

What bindings you're using

pyo3

Does cargo build work?

  • Yes, it works

If on windows, have you checked that you aren't accidentally using unix path (those with the forward slash /)?

  • Yes

Steps to Reproduce

See the failing CI log here: https://github.com/pola-rs/polars/actions/runs/9633112245/job/26567116574

@ritchie46 ritchie46 added the bug Something isn't working label Jun 23, 2024
@ritchie46
Copy link
Author

Note that adding the crate to dependencies (even not used) makes it work: pola-rs/polars@5cad69e

@messense messense added the sdist Source distribution label Jun 24, 2024
@danking
Copy link

danking commented Sep 4, 2024

I think we're encountering a similar issue in vortex (https://github.com/spiraldb/vortex).

We have a Rust workspace with several crates. Suppose, for example, that we have three crates: a, b, and python. The python crate has some Rust code and some Python code. python/Cargo.toml depends explicitly on a but not on b. a/Cargo.toml does depend on b.

If I run

maturin pep517 write-sdist  --sdist-directory sdist --manifest-path python/Cargo.toml

This writes an sdist/vortex-0.1.0.tar.gz which contains all of python and all of a but, critically, does not contain b. When I try to install this source distribution it fails.

pip3 install --force-reinstall --verbose sdist/vortex-0.1.0.tar.gz
...
  Running command Preparing metadata (pyproject.toml)
  error: failed to load manifest for workspace member `/private/var/folders/2l/yzgrtp5s1kqgxk5nsw8dc4hw0000gn/T/pip-req-build-n6a6wijx/encodings/alp`
  referenced by workspace at `/private/var/folders/2l/yzgrtp5s1kqgxk5nsw8dc4hw0000gn/T/pip-req-build-n6a6wijx/Cargo.toml`

  Caused by:
    failed to load manifest for dependency `a`

  Caused by:
    failed to load manifest for dependency `b`

  Caused by:
    failed to read `/private/var/folders/2l/yzgrtp5s1kqgxk5nsw8dc4hw0000gn/T/pip-req-build-n6a6wijx/b/Cargo.toml`

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

It seems to me that maturin needs to crawl the dependency tree and include in the sdist every local/workspace crate. If I add b to python/Cargo.toml's dependencies, the build succeeds (because b is included in the sdist).

# maturin --version
maturin 1.5.1
# cargo --version
cargo 1.81.0-nightly (a1f47ec3f 2024-06-15)

@messense
Copy link
Member

messense commented Sep 5, 2024

@danking do you have a fixed branch/commmit that we can use for repro?

@danking
Copy link

danking commented Sep 5, 2024

Hey @messense !

Thanks for the rapid reply! Yes, this branch fails:

git clone git@github.com:spiraldb/vortex.git --branch dk/python-publish

I failed to produce a simple example, so it might be that we have something subtly misconfigured. Here's an example of what goes wrong for me:

(workspace) # cd /tmp
(workspace) # git clone git@github.com:spiraldb/vortex.git --branch dk/python-publish
Cloning into 'vortex'...
remote: Enumerating objects: 17547, done.
remote: Counting objects: 100% (861/861), done.
remote: Compressing objects: 100% (385/385), done.
remote: Total 17547 (delta 472), reused 805 (delta 451), pack-reused 16686 (from 1)
Receiving objects: 100% (17547/17547), 7.88 MiB | 11.79 MiB/s, done.
Resolving deltas: 100% (11160/11160), done.
(workspace) # cd vortex 
(workspace) # maturin pep517 write-sdist  --sdist-directory sdist --manifest-path pyvortex/Cargo.toml
🍹 Building a mixed python/rust project
🔗 Found pyo3 bindings with abi3 support for Python ≥ 3.11
🐍 Not using a specific python interpreter
📡 Using build options features from pyproject.toml
📦 Including files matching "rust-toolchain.toml"
📦 Built source distribution to /private/tmp/vortex/sdist/vortex-0.1.0.tar.gz
vortex-0.1.0.tar.gz
(workspace) # tar tf sdist/vortex-0.1.0.tar.gz | grep vortex-proto
(workspace) # pip3 install sdist/vortex-0.1.0.tar.gz 
Processing ./sdist/vortex-0.1.0.tar.gz
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... error
  error: subprocess-exited-with-error
  
  × Preparing metadata (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> [26 lines of output]
      error: failed to load manifest for workspace member `/private/var/folders/2l/yzgrtp5s1kqgxk5nsw8dc4hw0000gn/T/pip-req-build-ufxs6v7d/encodings/alp`
      referenced by workspace at `/private/var/folders/2l/yzgrtp5s1kqgxk5nsw8dc4hw0000gn/T/pip-req-build-ufxs6v7d/Cargo.toml`
      
      Caused by:
        failed to load manifest for dependency `vortex-array`
      
      Caused by:
        failed to load manifest for dependency `vortex-datetime-dtype`
      
      Caused by:
        failed to load manifest for dependency `vortex-dtype`
      
      Caused by:
        failed to load manifest for dependency `vortex-proto`
      
      Caused by:
        failed to read `/private/var/folders/2l/yzgrtp5s1kqgxk5nsw8dc4hw0000gn/T/pip-req-build-ufxs6v7d/vortex-proto/Cargo.toml`
      
      Caused by:
        No such file or directory (os error 2)
      💥 maturin failed
        Caused by: Cargo metadata failed. Does your crate compile with `cargo build`?
        Caused by: `cargo metadata` exited with an error:
      Error running maturin: Command '['maturin', 'pep517', 'write-dist-info', '--metadata-directory', '/private/var/folders/2l/yzgrtp5s1kqgxk5nsw8dc4hw0000gn/T/pip-modern-metadata-2a5xkla4', '--interpreter', '/Users/danielking/projects/vortex/.venv/bin/python3']' returned non-zero exit status 1.
      Checking for Rust toolchain....
      Running `maturin pep517 write-dist-info --metadata-directory /private/var/folders/2l/yzgrtp5s1kqgxk5nsw8dc4hw0000gn/T/pip-modern-metadata-2a5xkla4 --interpreter /Users/danielking/projects/vortex/.venv/bin/python3`
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
error: metadata-generation-failed

× Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.
(workspace) # python3 --version
Python 3.11.9
(workspace) # python --version
Python 3.11.9
(workspace) # maturin -V 
maturin 1.5.1
(workspace) # cargo --version
cargo 1.81.0-nightly (a1f47ec3f 2024-06-15)

danking added a commit to spiraldb/vortex that referenced this issue Sep 5, 2024
`rye build` failed when it tried to build the sdist to produce a wheel. This failed because some
_transitive_ workspace dependencies (e.g. vortex-proto) were not present in the sdist.

The root cause appears to be a bug in maturin. I saw [a similar issue on their
repo](PyO3/maturin#2117 (comment)). I shared a branch with
them that reproduces the behavior.

Until this issue is resolved, we can work around it by explicitly depending on every workspace
crate.

Unrelated to the maturin issue, we also need to propagate the toolchain to the sdist build
environment.
danking added a commit to spiraldb/vortex that referenced this issue Sep 5, 2024
`rye build` failed when it tried to build the sdist to produce a wheel. This failed because some
_transitive_ workspace dependencies (e.g. vortex-proto) were not present in the sdist.

The root cause appears to be a bug in maturin. I saw [a similar issue on their
repo](PyO3/maturin#2117 (comment)). I shared a branch with
them that reproduces the behavior.

Until this issue is resolved, we can work around it by explicitly depending on every workspace
crate.

Two unrelated issues are also fixed here:

1. We must propagate the Rust toolchain to the sdist build environment.

2. `vortex-bytebool` was misnamed in the root Cargo.toml. I'm not sure why this isn't a problem for
other crates?
danking added a commit to spiraldb/vortex that referenced this issue Sep 5, 2024
`rye build` failed when it tried to build the sdist to produce a wheel. This failed because some
_transitive_ workspace dependencies (e.g. vortex-proto) were not present in the sdist.

The root cause appears to be a bug in maturin. I saw [a similar issue on their
repo](PyO3/maturin#2117 (comment)). I shared a branch with
them that reproduces the behavior.

Until this issue is resolved, we can work around it by explicitly depending on every workspace
crate.

Two unrelated issues are also fixed here:

1. We must propagate the Rust toolchain to the sdist build environment.

2. `vortex-bytebool` was misnamed in the root Cargo.toml. I'm not sure why this isn't a problem for
other crates?
danking added a commit to spiraldb/vortex that referenced this issue Sep 5, 2024
Fixes #695.

`rye build` failed when it tried to build the sdist to produce a wheel.
This failed because some _transitive_ workspace dependencies (e.g.
vortex-proto) were not present in the sdist.

The root cause appears to be a bug in maturin. I saw [a similar issue on
their
repo](PyO3/maturin#2117 (comment)).
I shared a branch with them that reproduces the behavior.

Until this issue is resolved, we can work around it by explicitly
depending on every workspace crate.

Two unrelated issues are also fixed here:

1. We must propagate the Rust toolchain to the sdist build environment.

2. `vortex-bytebool` was misnamed in the root Cargo.toml. I'm not sure
why this isn't a problem for other crates?
@messense
Copy link
Member

messense commented Sep 9, 2024

@danking I think your repro can be fixed by

diff --git a/pyvortex/Cargo.toml b/pyvortex/Cargo.toml
index 59b15ef8..80ee2568 100644
--- a/pyvortex/Cargo.toml
+++ b/pyvortex/Cargo.toml
@@ -58,7 +58,7 @@ tokio = { workspace = true, features = ["fs"] }
 vortex-alp = { workspace = true }
 vortex-array = { workspace = true }
 vortex-dict = { workspace = true }
-vortex-dtype = { workspace = true }
+vortex-dtype = { workspace = true, features = ["proto"] }
 vortex-error = { workspace = true }
 vortex-expr = { workspace = true }
 vortex-fastlanes = { workspace = true }

the problem is that the optional vortex-proto isn't listed in deps of pyvortex when invoking cargo metadata --format-version 1 without activating the proto feature.

@danking
Copy link

danking commented Sep 9, 2024

Thanks! Indeed, this change makes the sdist buildable (indeed, vortex-proto is present in the sdist).

I'm a bit confused though. I still don't see vortex-proto listed in the dependencies (I don't have a --format, I assume you meant --format-version):

cargo metadata --format-version 1 | jq '.packages[] | select(.name == "pyvortex")' | grep vortex-proto

How did this change convince maturin that it needed to grab the vortex-proto directory?

@messense
Copy link
Member

messense commented Sep 9, 2024

It changes the output of cargo metadata, specifically the resolve key.

messense added a commit that referenced this issue Sep 10, 2024
When building sdist, `cargo metadata` command should output
all dependencies including optional ones, otherwise the resulting
sdist file may not compile when building with optional features
activated.

Enabling `--all-features` ensure that all optional path dependencies
to be packaged into sdist, which should help with #2117.
messense added a commit that referenced this issue Sep 10, 2024
When building sdist, `cargo metadata` command should output
all dependencies including optional ones, otherwise the resulting
sdist file may not compile when building with optional features
activated.

Enabling `--all-features` ensure that all optional path dependencies
to be packaged into sdist, which should help with #2117.
messense added a commit that referenced this issue Sep 10, 2024
When building sdist, `cargo metadata` command should output
all dependencies including optional ones, otherwise the resulting
sdist file may not compile when building with optional features
activated.

Enabling `--all-features` ensure that all optional path dependencies
to be packaged into sdist, which should help with #2117.
@messense messense linked a pull request Sep 10, 2024 that will close this issue
@messense
Copy link
Member

Confirmed that both pola-rs/polars@c0871ef and vortex sdist build fine with #2215.

@danking
Copy link

danking commented Sep 10, 2024

Amazing! Thank you kindly @messense!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working sdist Source distribution
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants