Build configuration settings to facilitate complete link control #1793
Conversation
indygreg
force-pushed
the
build-settings
branch
2 times, most recently
from
5ae8102
to
169e58b
Compare
There was a problem hiding this comment.
👍 Overall I haven't really read the code super hard yet, I wanted to comment and ask questions about the high-level first. Covering each commit in turn:
- Yep this is much appreciated, thank you for writing this. It could be worth mentioning the defaults on deserialization (if the corresponding config key is missing)? In theory only
versionis required, depending on use case. - I'm suprised that cargo/rustc doesn't have what you need already. Can you give an example of the lines you need to emit? It seems like something like using RUSTFLAGS env var to pass link args might do what you need?
- The
links = "python"bit is a good idea, I think it probably does need to happen. A couple of questions about the use case though:
- There's already
pyo3_build_config::get()which can be used from build scripts to get the resolvedInterpreterConfig. If that were stabilised and documented would it meet your needs? - If the above is no good, what about passing config file contents through the
DEP_env var, rather than writing out to a path? Seems like an extra step which may not be needed, however I'm happy to cede on this point.
- Yep I think having this option makes sense as an advanced feature (it's basically the
extension-modulecargo feature on steroids).
One use case is we want to tell We also want to make use of
Maybe? I'm not sure if that will work correctly since it relies on
The commit message explains my reasoning on this topic. |
|
I've got PyOxidizer working with this PR! As hypothesized by my last comment, I was able to get rid of the |
Awesome! I think that if we need to add the So in that spirit, it might be worth trying out (And I was expecting that you'd have the If It's worth noting I had a bit of pain with sccache interacting with the config-file-writing - it wasn't reliably making it available to downwind crates. It was probably related to my (ab)use of getting PyO3's build script to write to the
Looks like you considered having multiple |
PyOxidizer has crates depending on `pyo3` that would like to access the `pyo3` crate configuration. (This use case isn't unique to PyOxidizer.) Cargo has a facility for enabling the build scripts of dependent crates to access _exported_ variables via `DEP_` environment variables. However, this only works if the exporting crate defines a `links` key in its Cargo manifest. See https://doc.rust-lang.org/cargo/reference/build-scripts.html#the-links-manifest-key. While `pyo3`'s build script doesn't yet export the variables that PyOxidizer will need, a prerequisite to making this work is adding the `links` key. Since this change could introduce unintended side-effects, it warrants being made in its own commit, which is why we're making this change outside of PyO3#1793. I _think_ this change should be mostly safe: the `links` key is effectively metadata advertising that a crate links against a named library. The only side-effects setting it has is to enable the aforementioned `DEP_` environment variables in build scripts and enforcing a limitation that only a single crate may link against the same native library. I believe the only potential for this change to cause problems is if there are multiple crates with `links = "python"` entries. I'm not aware of any other crates that advertise `links = "python"`: even `python3-sys` / `cpython` use `links = "python3"` so this change should not prevent dual use of `pyo3` and `cpython` in the same build.
PyOxidizer has crates depending on `pyo3` that would like to access the `pyo3` crate configuration. (This use case isn't unique to PyOxidizer.) Cargo has a facility for enabling the build scripts of dependent crates to access _exported_ variables via `DEP_` environment variables. However, this only works if the exporting crate defines a `links` key in its Cargo manifest. See https://doc.rust-lang.org/cargo/reference/build-scripts.html#the-links-manifest-key. While `pyo3`'s build script doesn't yet export the variables that PyOxidizer will need, a prerequisite to making this work is adding the `links` key. Since this change could introduce unintended side-effects, it warrants being made in its own commit, which is why we're making this change outside of PyO3#1793. I _think_ this change should be mostly safe: the `links` key is effectively metadata advertising that a crate links against a named library. The only side-effects setting it has is to enable the aforementioned `DEP_` environment variables in build scripts and enforcing a limitation that only a single crate may link against the same native library. I believe the only potential for this change to cause problems is if there are multiple crates with `links = "python"` entries. I'm not aware of any other crates that advertise `links = "python"`: even `python3-sys` / `cpython` use `links = "python3"` so this change should not prevent dual use of `pyo3` and `cpython` in the same build.
|
I'm starting to split functionality from this into separate PRs to reduce the scope of this one. I understand why we can't land some of this functionality in 0.14. But I want to make it clear that I'm blocked on adopting PyO3 in PyOxidizer until some aspects of this PR land. I'm on the fence about merging my PyO3 migration branch until there is a published With my discovery that |
|
👍 I've merged the piece on interpreter config docs and also the new
I fully agree that having a git dependency blocking release for you is undesirable. Most of what you need is going into 0.14.3, which I'm about to release shortly. If we can find ways to get the rest of what you need for PyOxidizer in as non-breaking changes I'd be happy to backport / cherry-pick to make a 0.14.4 to get you everything that you need. I guess we could also release 0.15 before long to make breaking changes if necessary. However I do like to try to include new features (or other improvements) relevant to most users alongside breaking changes so that people have a reason to upgrade / don't get burned out by churn. 😄 |
PyOxidizer has crates depending on `pyo3` that would like to access the `pyo3` crate configuration. (This use case isn't unique to PyOxidizer.) Cargo has a facility for enabling the build scripts of dependent crates to access _exported_ variables via `DEP_` environment variables. However, this only works if the exporting crate defines a `links` key in its Cargo manifest. See https://doc.rust-lang.org/cargo/reference/build-scripts.html#the-links-manifest-key. While `pyo3`'s build script doesn't yet export the variables that PyOxidizer will need, a prerequisite to making this work is adding the `links` key. Since this change could introduce unintended side-effects, it warrants being made in its own commit, which is why we're making this change outside of PyO3#1793. I _think_ this change should be mostly safe: the `links` key is effectively metadata advertising that a crate links against a named library. The only side-effects setting it has is to enable the aforementioned `DEP_` environment variables in build scripts and enforcing a limitation that only a single crate may link against the same native library. I believe the only potential for this change to cause problems is if there are multiple crates with `links = "python"` entries. I'm not aware of any other crates that advertise `links = "python"`: even `python3-sys` / `cpython` use `links = "python3"` so this change should not prevent dual use of `pyo3` and `cpython` in the same build.
indygreg
force-pushed
the
build-settings
branch
from
169e58b
to
927860f
Compare
|
Based on indygreg/PyOxidizer#433, it sounds like if we change this PR to just contain just the two commits adding extra options to |
PyOxidizer has crates depending on `pyo3` that would like to access the `pyo3` crate configuration. (This use case isn't unique to PyOxidizer.) Cargo has a facility for enabling the build scripts of dependent crates to access _exported_ variables via `DEP_` environment variables. However, this only works if the exporting crate defines a `links` key in its Cargo manifest. See https://doc.rust-lang.org/cargo/reference/build-scripts.html#the-links-manifest-key. While `pyo3`'s build script doesn't yet export the variables that PyOxidizer will need, a prerequisite to making this work is adding the `links` key. Since this change could introduce unintended side-effects, it warrants being made in its own commit, which is why we're making this change outside of PyO3#1793. I _think_ this change should be mostly safe: the `links` key is effectively metadata advertising that a crate links against a named library. The only side-effects setting it has is to enable the aforementioned `DEP_` environment variables in build scripts and enforcing a limitation that only a single crate may link against the same native library. I believe the only potential for this change to cause problems is if there are multiple crates with `links = "python"` entries. I'm not aware of any other crates that advertise `links = "python"`: even `python3-sys` / `cpython` use `links = "python3"` so this change should not prevent dual use of `pyo3` and `cpython` in the same build.
Yeah, assuming |
PyOxidizer needs to do some... questionable things with regards to configuring how the Python interpreter is linked. The way I solved this problem for the `cpython` / `python3-sys` crates was by adding a bunch of crate features to control what `cargo:` lines were emitted by the build scripts. This added a lot of complexity to the those crates for a target audience of ~1. Now that PyO3 has support for config files to control settings, this provides a richer mechanism than crate features to influence the build script. This commit defines a new field on the `InterpreterConfig` struct to hold an arbitrary list of strings/lines that should be emitted by the build script. This field is only every populated when parsing config files and it is only read by pyo3's build script to `println!()` additional values. My intended use case for this is to have PyOxidizer effectively control the interpreter link settings via the config file (at my own peril) while having minimal impact on the maintainability of PyO3's code base. Given the complexity of the link hacks employed, you probably don't want this polluting pyo3's code base.
indygreg
force-pushed
the
build-settings
branch
from
927860f
to
c8ad680
Compare
PyOxidizer requires advanced control over the settings used to link libpython. We recently implemented support for configuration files defining explicit lines to emit from build scripts to give callers control over what lines to emit from build scripts so use cases like PyOxidizer's are feasible without hacks in PyO3's code base. However, the default logic in `emit_link_config()` may not be appropriate in scenarios where link settings are provided via this "extra lines" mechanism. The default logic may prohibit use of or interfere with desired settings provided externally. This commit defines a new field on the interpreter config that suppresses the emission of the default link control logic from the `pyo3` build script. It effectively gives advanced consumers like PyOxidizer full control over link logic while minimally polluting PyO3's build logic. I thought about implementing this control as a crate feature. But given the expected target audience size of ~1, I thought a crate feature was too visible for a power user feature and decided to implement it via the configuration file.
indygreg
force-pushed
the
build-settings
branch
from
c8ad680
to
c9c606f
Compare
|
I rebased and reduced scope. I think this is ready for review. (Although I haven't tested this exact branch against PyOxidizer yet. I may find time to do that in ~8 hours. But no guarantees.) |
This PR makes a handful of changes to the crate build configuration to support advanced control over linking, which is required by PyOxidizer.
I wrote extensive commit messages detailing each change. Please read the commit messages for more context.
I'm creating this as a draft until I have more confidence that these changes are sufficient to support PyOxidizer on all platforms. I figured I'd publish this as a PR in hopes of getting feedback about the general direction sooner.
The first commit is generally useful and could be cherry picked to
mainif you want.