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: make pumpkin-py build without any additional installs #113

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

tiptenbrink
Copy link

Hey!

So while installing and trying to build, I came across the following:

Running cargo build in the project root directory (not necessarily useful, I did it by accident) means you also build pumpkin-py. However, I have a new laptop so I didn't yet have need to install the development headers for Python (python3-dev package on APT). This causes the the build to fail with the below linker error:

  = note: /usr/bin/ld: cannot find -lpython3.13: No such file or directory
          collect2: error: ld returned 1 exit status

This confused me (I've used PyO3 before), it's just an extension module (you're calling Rust from Python so you always have an interpreter) so why would you need the dev headers, which you usually only need if you're calling Python from a Rust executable (https://pyo3.rs/v0.22.6/index.html?highlight=python3-dev#using-python-from-rust).

I then noticed the extension-module feature is not enabled in the Cargo.toml. It is enabled in the pyproject.toml so it works fine if you build from maturin. However I think it's useful (to prevent confusion) to have cargo build when invoked on the whole workspace to still not fail. Plus, I think it's nice you can still just run cargo build even in pumpkin-py without needing a global package that isn't really required under these circumstances. It also means the dependencies in the Cargo.toml don't really match what you are actually building when you use maturin.

There's a tiny wrinkle, though. Maybe it's the reason you removed the feature from the Cargo.toml in the first place. When using workspaces and an IDE that runs cargo check on save, probably the Python interpreter will not match the one in the virtualenv that you use for maturin. This causes PyO3 to recompile on most builds. Thankfully I've had this problem and this can be fixed, see PyO3/pyo3#1708. This does mean that by default someone who will develop the python interface will need to add something to their config manually. The alternative solution however means that cargo build will also fail without some manual work by the user (installing a venv to the right place).

When testing, I found that the current version of the Python script actually fails, so I fixed that and updated the README with the correct path to the file to run. So even if you disagree with the main change that part is definitely useful.

@maartenflippo
Copy link
Contributor

@tiptenbrink if you can update this PR, we will approve and merge it! Thanks for the contribution

@tiptenbrink
Copy link
Author

@maartenflippo it's now updated, so should be ready. I also updated to PyO3 0.23.3 due to PyO3/pyo3#4757 (which was alerted by the precommit)

@maartenflippo maartenflippo changed the base branch from main to develop December 19, 2024 09:37
@tiptenbrink
Copy link
Author

@maartenflippo this one is now ready, rebased on top of develop and made into one commit.

@maartenflippo maartenflippo merged commit e587458 into ConSol-Lab:develop Dec 19, 2024
4 checks passed
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.

2 participants