-
Notifications
You must be signed in to change notification settings - Fork 762
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
pyo3-build-config: new crate to re-use build.rs across crates #1622
Conversation
81d4a6f
to
e903196
Compare
e903196
to
79c7149
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
|
||
let target_arch = cargo_env_var("CARGO_CFG_TARGET_ARCH"); | ||
let target_vendor = cargo_env_var("CARGO_CFG_TARGET_VENDOR"); | ||
let target_os = cargo_env_var("CARGO_CFG_TARGET_OS"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately maturin can not use Cargo env var since it's building other crates not itself. See https://github.com/PyO3/maturin/blob/8986455758125328744c8ab9ba3a2371d489860a/src/cross_compile.rs#L12-L13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm good point, I guess we'll need to refactor some of these functions to take target / arch / vendor as arguments? What form would be convenient for maturin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we'll need to refactor some of these functions to take target / arch / vendor as arguments?
That should work for maturin.
1f14efd
to
6def8fe
Compare
#[cfg(not(Py_3_8))] | ||
fn function_only_supported_before_python_3_8() { } | ||
|
||
#[cfg(not(Py_LIMITED_API))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, is it Py_LIMITED_API
or abi3
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[cfg(Py_LIMITED_API)]
is what we've been using internally for some time now. I agree it seems slightly strange at first glance. I think the history of it is that it's designed to be similar to the C preprocessor define:
That does raise a good point though. If any of these cfgs are potentially confusing, we should consider alternatives before we release this to users in 0.14.
Some random ideas:
#[cfg(py_version = 3_7)]
#[cfg(pyo3 = Py_3_7)]
#[cfg(pyo3 = abi3)]
#[cfg(pyo3 = Py_LIMITED_API)]
#[cfg(pyo3 = PyPy)]
The extra verbosity is a little annoying for complex patterns, e.g.
#[cfg(any(not(pyo3 = Py_LIMITED_API), py_version = Py_3_10))]
... but it's not too bad.
Definitely I think that scoping these cfgs would make it much easier for users unfamiliar with PyO3 to figure out where they are coming from. Depends if we think that's worth the extra clutter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thought is that we also have the (very specialist) #[cfg(py_sys_config="Py_TRACE_REFS")]
attributes. It might be nicer to move these to just be #[cfg(Py_TRACE_REFS)]
etc, which then fits nicely with #[cfg(Py_LIMITED_API)]
.
@birkenfeld I've now pushed a lot of documentation for this. I think it should be ready to merge if you're happy with it and want to use it to continue testing the @messense to keep the review focussed I am thinking I will make the changes needed for maturin once we've merged this. (We can iterate on what maturin needs in a new PR, we can create an issue to remember to do this.) (@mejrs I wrote a lot of documentation - feel free to review this if you're interested in what I've written and / or have opinions on the overall design.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Nice work ⭐ We should add an example that shows some useful real world use case where you'd want to use these flags. For example: // we prefer to use `zoneinfo`, which came out with 3.9
// this function is active when built on Python 3.9 or greater
#[cfg(Py_3_9)]
fn your_function() -> PyResult<...>{
Python::with_gil(|py| -> PyResult<...>{
let zoneinfo = PyModule::import(py, "zoneinfo")?;
// do work here
})?;
}
// however, on an earlier version we can fall back
// this function is active when built on lower than Python 3.9
#[cfg(not(Py_3_9))]
fn your_function() -> PyResult<...>{
Python::with_gil(|py| -> PyResult<...>{
let datetime = PyModule::import(py, "datetime")?;
// do work here
})?;
} (I can't think of a great python version specific functionality, also this is probably not something you'd actually use cfg's for- please suggest a better use case :-) ). I'd also copy paste some more of the documentation into Finally, I've been doing some writing for |
I 100% agree. The best example I can currently think of is for the With that in mind, I'm going to merge this PR now, and add an issue to add a better example once |
This adds a new crate
pyo3-build-config
, which most of PyO3'sbuild.rs
is moved into.The new crate exposes a single documented public API,
use_pyo3_cfgs()
, which adds the#[cfg(Py_3_6, Py_LIMITED_API)]
etc. flags which we've been using inside PyO3 for some time.The majority of PyO3's
build.rs
got moved topyo3_build_config::impl
, which we could add tests for in the future.This allows us to re-use the cfgs across our internal crates and examples. Downstream users can support multiple Python versions using the same technique.
If we like this, it could be worth adding a proper guide entry before the 0.14 release.
cc @birkenfeld - this will resolve the need for cfg flags in
pyo3-macros-backend
.cc @messense - this new crate contains PyO3's ways to detect cross-compilation configuration, which you might find useful in maturin.