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

Maturin sdist Generation Fails When Using --no-isolation #2245

Closed
1 of 2 tasks
paddonizio opened this issue Oct 2, 2024 · 5 comments · Fixed by #2292
Closed
1 of 2 tasks

Maturin sdist Generation Fails When Using --no-isolation #2245

paddonizio opened this issue Oct 2, 2024 · 5 comments · Fixed by #2292
Labels
bug Something isn't working sdist Source distribution

Comments

@paddonizio
Copy link

paddonizio commented Oct 2, 2024

Bug Description

Hello,

I have been working on getting Rust builds with maturin in our company's build system. This system isolates builds in an environment already and then runs sys.executable -m build --sdist --outdir=<staging dir> --no-isolation. Maturin then fails with Found pyo3 bindings 💥 maturin failed Caused by: Invalid python interpreter version.

I traced this issue to https://github.com/PyO3/maturin/blob/main/src/build_options.rs#L1206. Here, maturin calls python_ver.split_once('.').... However, in a no-isolation situation, for sdist generation, maturin gets the python interpreter from target.get_python. This will return python3 for non-venv linux systems. If python3 is not available on the system, either because the symlink doesn't exist or because it isn't in the system PATH, maturin calls find_interpreter_in_sysconfig, which has the aforementioned python_ver.split_once('.'). When we try to split_once on that, it fails.

I'm happy to put up a fix for this if I can get some guidance on what the proper fix should be. We could first check for just a major version for example and go from there if that would work downstream.

Our build system is quite complicated, but I can reproduce this locally as well and with multiple python versions.

Your maturin version (maturin --version)

1.7.4

Your Python version (python -V)

3.12

Your pip version (pip -V)

23.2.1

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

  1. Create a minimal python/rust library with just PyO3 as a dependency.
  2. In a linux system, (no venv), install build and maturin.
  3. Run python3.12 -m build --sdist --no-isolation .
  4. Maturin should crash with the same error.
@paddonizio paddonizio added the bug Something isn't working label Oct 2, 2024
@paddonizio paddonizio changed the title Maturin sdist Generation Fails When Default Python Command Doesn't Exist Maturin sdist Generation Fails When Using --no-isolation Oct 2, 2024
@messense messense added the sdist Source distribution label Oct 3, 2024
@lkollar
Copy link

lkollar commented Oct 28, 2024

I just tried reproducing this with the python:3.12 Docker image and I can successfully build the sample extension generated by maturin init:

🔗 Found pyo3 bindings
🐍 Found CPython 3.12 at /usr/local/bin/python3
📡 Using build options features from pyproject.toml
From `cargo package --list --allow-dirty --manifest-path /tmp/foobar/Cargo.toml`:
warning: manifest has no description, license, license-file, documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
📦 Built source distribution to /tmp/foobar/dist/foobar-0.1.0.tar.gz
foobar-0.1.0.tar.gz
Successfully built foobar-0.1.0.tar.gz

As you can see in the output above, the Python executable was resolved to /usr/local/bin/python3, which should trigger the issue, but it doesn't in this case. So I suspect that there is something else at play.

@paddonizio is there something else I'm missing here to reproduce it?

@paddonizio
Copy link
Author

I was just testing out my fix and realized the same thing. Upon further investigation, I think the one additional piece is that python3 cannot be in the PATH. In our build environment's case, we don't symlink python3.x to python3, and coincidentally, the environment I was testing in outside of our build system also does not have python3 as a symlink. When this happens, we end up in the find_interpreter_in_sysconfig code that has the version split issue described above.

@lkollar
Copy link

lkollar commented Oct 28, 2024

Thanks, I was able to reproduce it now by removing the python3 symlink in the container. From what I can tell, the issue is that unless the interpreter is explicitly specified on the command line, maturin falls back to a default python3 executable here.

When building an sdist, the pep517 write-sdist subcommand is invoked, which unlike build-wheel, does not take an interpreter argument. And because maturin falls back to the hard coded python3 value outside of a venv, it just expects to find a python3 executable on the PATH.

I think write-sdist could also take a --interpreter argument, which can be set as sys.executable in __init__.py and also allows users of the maturin CLI to set the interpreter they want to use to build the sdist.

@paddonizio
Copy link
Author

@lkollar,

Thanks for your input. I put up a PR with your thoughts as it made sense to me. Upon looking though, I don't actually see where interpreter is even used for sdist generation, though I could have missed something. Either way, I tested this candidate on my local system and it worked. Let me know your thoughts.

@lkollar
Copy link

lkollar commented Oct 29, 2024

Actually, that's a very good point, maybe there is no real need for the interpreter at all on this code path and the right thing to do would be to simply side step the interpreter detection when building an sdist? That would probably require making the interpreter an optional field of BuildContext. It would be great if a maintainer could chime in.

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
3 participants