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

Expose pyo3-build-config APIs for maturin #1627

Closed
davidhewitt opened this issue May 25, 2021 · 6 comments
Closed

Expose pyo3-build-config APIs for maturin #1627

davidhewitt opened this issue May 25, 2021 · 6 comments

Comments

@davidhewitt
Copy link
Member

See #1622 (comment)

@aganders3
Copy link
Contributor

I'm interested in taking this on, but I'm not sure I completely understand. I get that the current method of getting the config using cargo_env_var is not suitable for maturin, but I also don't see maturin depending on pyo3 or pyo3-build-config to call this function directly.

It seems there are a few possible ways to handle this in pyo3:

  • refactor cross_compiling to take the "target triple" as args (and I suspect move the cargo_env_var parts to another function)
  • refactor cross_compiling to optionally take these args (else check the Cargo env vars as it does now)
  • refactor cross_compliling to look for non-Cargo env vars (e.g. PYO3_TARGET_ARCH), falling back to the Cargo env vars

It would be nice to pair this with a PR in maturin for whatever changes would be needed there. I am happy to be the implementer there as well but will definitely need help figuring out where that might go.

@davidhewitt
Copy link
Member Author

Some help with this would be awesome!

refactor cross_compiling to take the "target triple" as args (and I suspect move the cargo_env_var parts to another function)

That is the design that I was thinking. There's also a couple of other functions that would be nice to expose (see below).

I also don't see maturin depending on pyo3 or pyo3-build-config to call this function directly.

It would be nice to pair this with a PR in maturin for whatever changes would be needed there.

Yep, so at the moment maturin doesn't have a dependency, however the idea is that it should be able to depend on pyo3-build-config (with default-features = false).

If you look at the following file, you'll recognise pretty much all of it is logic that also lives in pyo3-build-config/src/impl_.rs.

https://github.com/PyO3/maturin/blob/main/src/cross_compile.rs

That file was basically a copypasta of bits of PyO3's build script some time ago before we made pyo3-build-config into a separate crate. So the goal of this issue would be to add sufficient pub functions to pyo3-build-config so that maturin could re-use as much of that logic as possible.

I'm happy to help feed back on the other functions we'd need if you'd like pointers on those too.

@aganders3
Copy link
Contributor

Thanks - this is super helpful and seems like enough for me to get started. I'll come back here when I have more questions.

@aganders3
Copy link
Contributor

Okay - I'm just documenting my process/progress here, but please let me know if I'm off course. Otherwise I will work on a PR next.

I found these similar items in the two projects. My plan is to change the pyo3 functions/structs to be compatible with the apparent needs of the maturin equivalents and make them public. Then maturin can option in the pyo3-build-config implementations.

pyo3-build-config maturin changes needed
fn cross_compiling -> Result<Option<CrossCompileConfig>> fn is_cross_compiling -> bool pyo3 needs a function that takes args instead of using cargo_env_var
fn find_sysconfigdata -> Result<PathBuf> fn find_sysconfigdata -> Result<PathBuf> n/a
fn parse_sysconfigdata -> InterpreterConfig fn parse_sysconfigdata -> HashMap (then used to make a PythonInterpreter) need to be able to construct PythonInterpreter from InterpreterConfig (or perhaps replace PythonInterpreter)
struct InterpreterConfig struct PythonInterpreter InterpreterConfig is missing fields: abi_flags, abi_tag, ext_suffix (all from sysconfigdata) and target (already available in maturin::build_options::find_interpreter())

@davidhewitt
Copy link
Member Author

👍 that sounds great!

@davidhewitt
Copy link
Member Author

Implemented in #1996. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants