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

cargo: add links = "python" #1819

Merged
merged 1 commit into from Aug 30, 2021
Merged

cargo: add links = "python" #1819

merged 1 commit into from Aug 30, 2021

Conversation

indygreg
Copy link
Contributor

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 #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.

@davidhewitt
Copy link
Member

FYI I'm sitting on merging this just because I want to understand whether it's really necessary. So I'd like to see the result of indygreg/PyOxidizer#433 (comment)

TBH it's probably a good idea to add this key anyway, however no harm in waiting. I'll make sure this gets merged before 0.15 if deemed relevant.

@davidhewitt davidhewitt added this to the 0.15 milestone Aug 25, 2021
@birkenfeld
Copy link
Member

Wasn't it discussed to add links somewhere else too (to avoid multiple pyo3s in the dep graph)?

@davidhewitt
Copy link
Member

Yes, though I can't remember exactly where. That's what I was implicitly referring to when I said "it's probably a good idea to add this key anyway" - I should have been clearer 🙃

@davidhewitt
Copy link
Member

Ok, shall we go ahead and merge this then? Looks like it needs a rebase; there's a CHANGELOG merge conflict and also some kind of glitch with CI which probably will get fixed with a rebase.

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
Copy link
Contributor Author

I rebased to resolve the merge conflict.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@davidhewitt davidhewitt merged commit 868f668 into PyO3:main Aug 30, 2021
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.

None yet

3 participants