-
Notifications
You must be signed in to change notification settings - Fork 135
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
Support link mode control #193
Conversation
FWIW #191, #192, and this issue represent all the changes I've made to Merging these PRs isn't super urgent for me, as PyOxidizer has been getting by with a fork of |
@indygreg thanks for the bunch of PRs. I agree with the general goal, and, needless to say, I wish I've assigned a few to myself (those I felt confident with). I'll take a look at that one once I've reviewed those easier ones, though. The fact that it's not super urgent aligns well with my present workload :-) As you know, I don't have that much bandwith I will surely take a look at that one, but that should be after the easy ones. Don't hesitate to ping me after 2 weeks |
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'm generally favorable to these changes, having maybe just minor suggestions. I'm not bothered that it's a bit exotic use-case, as long as it has transparent backwards compatibility, which I think it does.
As for supporting Python 3 only, that's fine by me, though it would be better to abort the build with an explicit error message if mixed with Python 2 features.
But the thing is I don't have any Windows system to try this on, nor significative development experience with Windows. So it's best if someone else does the conclusion and probable merge.
Would it be hard to add Windows CI for the new unresolved static linking feature? Or should we consider it's covered enough by PyOxidizer tests?
Cargo.toml
Outdated
# These these features to explicitly control linking for Python 3. | ||
# (See the documentation in python3-sys/Cargo.toml for more info.) | ||
link-mode3-default = [ "python3-sys/link-mode-default" ] | ||
link-mode3-unresolved-static = [ "python3-sys/link-mode-unresolved-static" ] |
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.
Just nitpicking, but I don't feel the need to include this 3
in the new feature names. This would be consistent with extension-module
, that does not.
Maybe it'd be better to prefix with python
or py
, to make explicit it's about linking Python: py-link-mode-default
, py-link-mode-unresolved-static
?
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 will incorporate this suggestion.
# type will be static, shared, or framework depending on the discovered Python. | ||
# | ||
# The path to pythonXY from the discovered Python install may also be | ||
# added to the linker search path. |
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.
Maybe here add to the comments that it's automatically activated (I can see that in build.rs
) if other link-mode
features aren't specified?
I suppose you didn't put it in default
in order for the change to be transparently compatible for downstream crates that use --no-default-features
, e.g, those that are for Python 2.7 ?
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.
Yes, I didn't put in in default
because of implications for backwards compatibility or --no-default-features
.
e640eb7
to
1ef3e2c
Compare
Regarding automated tests, I'm not sure how. We would have to inspect low-level In the latest push, I also added an abort in the main crate's |
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'm not too worried about the typo, but will merge this later in case you want to fix it. Looks fine to me.
I'll try to spin a release soon, too.
Cargo.toml
Outdated
@@ -65,6 +65,10 @@ extension-module = [ "python3-sys/extension-module" ] | |||
# or python3-sys. (honestly, we should probably merge both crates into 'python-sys') | |||
extension-module-2-7 = [ "python27-sys/extension-module" ] | |||
|
|||
# These these features to explicitly control linking for Python 3. |
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 assume this is a typo - Use these features...
?
let link_mode_unresolved_static = env::var_os("CARGO_FEATURE_LINK_MODE_UNRESOLVED_STATIC").is_some(); | ||
|
||
if link_mode_default && link_mode_unresolved_static { | ||
return Err("link-mode-default and link-mode-unresolved-static are mutually exclusive".to_owned()); |
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 think you have to be a little bit careful with mutually exclusive features, that's not really the way they were designed, as Cargo expects to be able to union them together when multiple crates depend on the same crate with different features.
That said, for this particular link mode case, it doesn't seem likely that would happen.
As part of developing PyOxidizer, I needed to force python3-sys to statically link against a Python library on Windows in a downstream crate of python3-sys. This requires the unstable `static-nobundle` link type so Cargo leaves symbols as unresolved when python3-sys is built. (Currently, the `static` linkage type verifies referenced symbols are present at crate build time.) See rust-lang/rust#37403 for more. Look for comments by me (@indygreg) to describe the issue in more detail. This commit teaches python3-sys a pair of new build features which enable more explicit control over the linker directives emitted by its build script. If no directive is specified, `link-mode-default` is used and the existing logic for linker directive emission is used. If `link-mode-unresolved-static` is used and we're on Windows, we emit a `static-nobundle=pythonXY` linker directive and omit the location of the library. This effectively says "I depend on a static `pythonXY` library but don't resolve the symbols when you build me and require someone else to specify the location to that library." What PyOxidizer does is emit its own linker directive that defines the location of a static `pythonXY` library, satisfying the linker constraint and enabling the build to work. If a downstream crate doesn't do this, the build should fail due to a missing library or symbols. I have purposefully designed the crate features to be extensible. If we want to add additional, mutually exclusive features in the future, we could do that. e.g. we could add a `link-mode-static` that force emits a `rustc-link-lib=static=pythonXY` directive to force static linking, even if a shared library is detected. But I have no need for this today and don't want to complicate the code, so I haven't added it. To round out the new feature, features have been added to the cpython crate to toggle the new features. Because Python 2.7 is end of life, I have not implemented the new feature for Python 2.7. I suspect very few people will use this feature anyway and I'm pretty confident that nobody will request this feature on Python 2.7. I concede that adding this feature to the crate to support PyOxidizer's esoteric use case is a bit unfortunate. I really wish Cargo allowed a crate to wholesale replace the build script output of a dependency, as PyOxidizer could statically resolve the Python settings for python3-sys since it brings its own Python library. But Cargo doesn't have this feature. So I'm stuck having to land this feature in the upstream crate to avoid having to maintain a permanent fork of `rust-cpython`. Sorry :/
1ef3e2c
to
10d667f
Compare
Thank you for accepting this! I was able to move PyOxidizer off a fork of |
As part of developing PyOxidizer, I needed to force python3-sys to
statically link against a Python library on Windows in a downstream
crate of python3-sys. This requires the unstable
static-nobundle
link type so Cargo leaves symbols as unresolved when python3-sys
is built. (Currently, the
static
linkage type verifies referencedsymbols are present at crate build time.) See
rust-lang/rust#37403 for more. Look for
comments by me (@indygreg) to describe the issue in more detail.
This commit teaches python3-sys a pair of new build features which
enable more explicit control over the linker directives emitted by
its build script. If no directive is specified,
link-mode-default
is used and the existing logic for linker directive emission is
used. If
link-mode-unresolved-static
is used and we're on Windows,we emit a
static-nobundle=pythonXY
linker directive andomit the location of the library. This effectively says "I depend
on a static
pythonXY
library but don't resolve the symbolswhen you build me and require someone else to specify the location
to that library." What PyOxidizer does is emit its own linker
directive that defines the location of a static
pythonXY
library,satisfying the linker constraint and enabling the build to work.
If a downstream create doesn't do this, the build should fail due
to a missing library or symbols.
I have purposefully designed the crate features to be extensible.
If we want to add additional, mutually exclusive features in the
future, we could do that. e.g. we could add a
link-mode-static
that force emits a
rustc-link-lib=static=pythonXY
directiveto force static linking, even if a shared library is detected.
But I have no need for this today and don't want to complicate
the code, so I haven't added it.
To round out the new feature, features have been added to the
cpython crate to toggle the new features.
Because Python 2.7 is end of life in a few weeks, I have not
implemented the new feature for Python 2.7. I suspect very
few people will use this feature anyway and I'm pretty confident
that nobody will request this feature on Python 2.7.
I concede that adding this feature to the crate to support
PyOxidizer's esoteric use case is a bit unfortunate. I really wish
Cargo allowed a crate to wholesale replace the build script output
of a dependency, as PyOxidizer could statically resolve the
Python settings for python3-sys since it brings its own Python
library. But Cargo doesn't have this feature. So I'm stuck
having to land this feature in the upstream crate to avoid having
to maintain a permanent fork of
rust-cpython
. Sorry :/