Conversation
|
@davidhewitt do you mind giving a little feedback if this makes sense |
luxedo
force-pushed
the
4761/build-config-cleanups
branch
from
ae5453c to
0726e20
Compare
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
I like this approach. I recently touched a lot of the same code (see #5924 which will conflict with this) and noticed the parameterless functions that query environment variables. I think observing the environment state once and then referring to a state struct thereafter is definitely cleaner.
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Agreed, very nice cleanup 👍
| BuildScriptContext, CrossCompileConfig, InterpreterConfig, PythonImplementation, PythonVersion, | ||
| Triple, BUILD_CTX, |
There was a problem hiding this comment.
I think we should make BUILD_CTX not a public export but instead part of the pyo3_build_script_impl exports below (i.e. private implementation detail).
| pub struct ExtEnv { | ||
| pub is_print_config: LazyLock<bool>, | ||
| pub use_abi13_forward_compatibility: LazyLock<bool>, | ||
| pub pyo3_config_file: LazyLock<Option<PathBuf>>, | ||
| pub pyo3_python: LazyLock<Option<OsString>>, | ||
| pub pyo3_no_python: LazyLock<bool>, | ||
| pub pyo3_build_extension_module: LazyLock<bool>, | ||
| pub pyo3_cross: LazyLock<Option<OsString>>, | ||
| pub pyo3_cross_lib_dir: LazyLock<Option<OsString>>, | ||
| pub pyo3_cross_python_version: LazyLock<Option<OsString>>, | ||
| pub pyo3_cross_python_implementation: LazyLock<Option<OsString>>, | ||
| pub python_sysconfigdata_name: LazyLock<Option<OsString>>, | ||
| pub virtual_env: LazyLock<Option<OsString>>, | ||
| pub conda_prefix: LazyLock<Option<OsString>>, | ||
| } | ||
|
|
||
| pub struct CargoEnv { | ||
| pub dep_python_pyo3_config: LazyLock<Option<String>>, | ||
| pub cargo_feature_abi3: LazyLock<bool>, | ||
| pub cargo_feature_extension_module: LazyLock<bool>, | ||
|
|
||
| /// The minimum supported Python version from PyO3 `abi3-py*` features. | ||
| /// Must be called from a PyO3 crate build script. | ||
| pub abi3_version: LazyLock<Option<PythonVersion>>, | ||
| pub cargo_cfg_target_pointer_width: | ||
| LazyLock<Result<u32, Box<dyn std::error::Error + Send + Sync>>>, | ||
| pub cargo_cfg_target_os: LazyLock<String>, | ||
| pub cargo_feature_auto_initialize: LazyLock<bool>, | ||
| } |
There was a problem hiding this comment.
I have an intuition that it'd be helpful if these were named after their environment variables, e.g. is_print_config -> pyo3_print_config etc.
That'd make it really easy to see all the environment accesses at a glance.
|
|
||
| /// Gets an external environment variable, and registers the build script to rerun if | ||
| /// the variable changes. | ||
| pub fn env_var(var: &str) -> Option<OsString> { |
There was a problem hiding this comment.
Can probably make this private, and similar for cargo_env_var below.
| pub fn env_var(var: &str) -> Option<OsString> { | |
| fn env_var(var: &str) -> Option<OsString> { |
| pub fn env_var(var: &str) -> Option<OsString> { | ||
| if cfg!(feature = "resolve-config") { | ||
| println!("cargo:rerun-if-env-changed={var}"); | ||
| pub struct ExtEnv { |
There was a problem hiding this comment.
| pub struct ExtEnv { | |
| /// External environment variables which can be used to configure PyO3. | |
| /// | |
| /// Modifying these will generally cause PyO3 to rebuild (subject to some | |
| /// ordering between which variables take priority for certain cases). | |
| pub struct ExtEnv { |
| pub conda_prefix: LazyLock<Option<OsString>>, | ||
| } | ||
|
|
||
| pub struct CargoEnv { |
There was a problem hiding this comment.
| pub struct CargoEnv { | |
| /// Environment variables set by cargo, which PyO3's build scripts consume. | |
| pub struct CargoEnv { |
3 participants
Closes #4761
Creates a new struct
BuildScriptContextfor storing environment variables.