ConSol-Lab / Pumpkin

A lazy clause generation constraint solver written in Rust.
Apache License 2.0
17 stars 4 forks source link

fix: make pumpkin-py build without any additional installs #113

Open tiptenbrink opened 2 weeks ago

tiptenbrink commented 2 weeks ago

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 https://github.com/PyO3/pyo3/issues/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.