-
Notifications
You must be signed in to change notification settings - Fork 763
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
: rebuild when PYO3_ENVIRONMENT_SIGNATURE
value changed
#2727
Conversation
pyo3-build-config/src/impl_.rs
Outdated
@@ -1685,6 +1685,8 @@ fn get_env_interpreter() -> Option<PathBuf> { | |||
/// 4. `python3`, as above | |||
pub fn find_interpreter() -> Result<PathBuf> { | |||
if let Some(exe) = env_var("PYO3_PYTHON") { | |||
println!("cargo:rerun-if-env-changed=PYO3_PYTHON_VERSION"); | |||
println!("cargo:rerun-if-env-changed=PYO3_PYTHON_IMPLEMENTATION"); |
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.
Though we need to decide whether these two env vars should override version and implementation we get from the Python interpreter like what PYO3_CROSS_PYTHON_VERSION
and PYO3_CROSS_PYTHON_IMPLEMENTATION
do. I prefer not to do that, what do you guys think?
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.
Hhhmmm, I think if these are passed explicitly, they should override what the interpreter says. We could issue a warning if their contents does not match what the interpreter says though.
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 prefer not to because they are not intended to be used to override Python version and implementation. Perhaps it's better to be named as _PYO3_XXX
if we agree to not override.
That said, I'm not strongly opposed to make them override what the interpreter says.
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.
If we really want them only to trigger rebuilds, how about using an opaque variable PYO3_ENVIRONMENT_SIGNATURE
and documenting that this has no other effect than to trigger rebuilds if its value changes. It would then be up to the other layers of automation like Maturin what they put in there (like interpreter version used by the venv).
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.
An opaque variable is certainly better, thanks for the idea!
21fa094
to
2645d4b
Compare
pyo3-build-config
: rebuild when PYO3_PYTHON_VERSION
/PYO3_PYTHON_IMPLEMENTATION
changedpyo3-build-config
: rebuild when PYO3_ENVIRONMENT_SIGNATURE
value changed
2645d4b
to
dee791d
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.
Yep I think this makes sense too 👍
1263: Add support for `PYO3_ENVIRONMENT_SIGNATURE` r=messense a=messense xref PyO3/pyo3#2727 Co-authored-by: messense <[email protected]>
1263: Add support for `PYO3_ENVIRONMENT_SIGNATURE` r=messense a=messense xref PyO3/pyo3#2727 Co-authored-by: messense <[email protected]>
1263: Add support for `PYO3_ENVIRONMENT_SIGNATURE` r=messense a=messense xref PyO3/pyo3#2727 Co-authored-by: messense <[email protected]>
Fixes #2724