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

pyo3-build-config: support "config files" #1758

Merged
merged 6 commits into from
Aug 7, 2021

Conversation

davidhewitt
Copy link
Member

This is a final change which I'd like to put into PyO3 0.14.2.

When testing out 0.14.1 at work, I ran into problems building in our CI environment where we use sccache. It turns out the strategy in #1648 of generating a config file is problematic with sccache, as it doesn't know about this file and can't cache it.

This convinced me that it's better to go with the original strategy in #1622 which generated the configuration inside the pyo3-build-config build script and inlined it into that crate. Because it then becomes part of the compiled code, this works fine with sccache.

The only situation where inling the configuration doesn't work is in cross-compile scenarios. In this case we still need to let PyO3's build script read the right env vars and generate a cross-compile config file.

To at least alleviate the pain of config files with sccache, I've added a PYO3_CONFIG_FILE env var which users can set to specify a hard-coded config file location. Moving it outside of the build script in this way means that users won't run into issues with caching tools. The output of PYO3_PRINT_CONFIG is valid to be put straight into a config file which users can save.

The PYO3_CONFIG_FILE env var may also prove useful for other users where PyO3's build scripts don't generate the right configuration. I'm thinking in this case that users can use it to override whatever environment we got wrong.

cc @indygreg I'm wondering if this config file might eventually be suitable for use by PyOxidizer.

With this patch, I was able to get PyO3 0.14 into production at Re:infer. 🚀

@davidhewitt
Copy link
Member Author

(added a commit to improve test coverage now that we've got a separate crate rather than it all being in a build script!)

[review] birkenfeld

Co-authored-by: Georg Brandl <[email protected]>
@davidhewitt
Copy link
Member Author

I've cherry-picked #1759 onto this PR to simplify adding testing and coverage for this.

@indygreg
Copy link
Contributor

indygreg commented Aug 7, 2021

My understanding from perusing the code is that by setting an environment variable you can point PyO3's build script at an arbitrary path exposing properties that would otherwise be derived by invoking a Python interpreter. It appears all settings that would be derived from the Python interpreter can now be set via an arbitrary config file, effectively enabling you to define arbitrary Python interpreter / linking configurations with PyO3 none the wiser.

I'm optimistic this functionality will enable PyOxidizer to [finally] be able to cross-compile in certain scenarios and target architectures not currently possible due to cross-compiling issues. But I can't say this with 100% certainty until I actually experiment. At the very least, this is a massive step in the right direction! Thank you for implementing this and thinking of PyOxidizer's use case.

Are there plans to document the config file format or will power users like myself just have to look at the source code to see which named fields are read? Perhaps InterpreterConfig::from_reader() could validate that all known config options are defined so parsing is more strict and additions to the "API" result in hard failures if they aren't present?

@davidhewitt
Copy link
Member Author

🚀 always pleased to help deliver functionality needed by the community! My brain had been turning over the idea of a config file for a while; it was only the other day that I realised I could do this basic key=value format rather than needing to have a JSON or TOML parser embedded in the build script.

My understanding from perusing the code is that by setting an environment variable you can point PyO3's build script at an arbitrary path exposing properties that would otherwise be derived by invoking a Python interpreter. It appears all settings that would be derived from the Python interpreter can now be set via an arbitrary config file, effectively enabling you to define arbitrary Python interpreter / linking configurations with PyO3 none the wiser.

That's pretty much the idea. It's nice that with these build scripts we can get most use-cases working out of the box, however I'm sure that there's many combinations which it gets wrong (cross-compiling in particular).

Are there plans to document the config file format or will power users like myself just have to look at the source code to see which named fields are read?

I was thinking to document it as part of the 0.15 release. I'm going to put 0.14.2 live tomorrow and I didn't want to commit to a stable config format immediately, so leaving it undocumented right now is just to allow time to see that this is definitely the right design. (And for power users like yourself to give initial feedback.)

Perhaps InterpreterConfig::from_reader() could validate that all known config options are defined so parsing is more strict and additions to the "API" result in hard failures if they aren't present?

I was aiming for the reverse - for maximum backwards-compatibility only version would be a required field on the config. That way we could add new fields without breaking anyone. For example this could allow us to add some config settings only relevant for very advanced usage like PyOxidizer without the rest of the userbase having to care about them.

However this is exactly why I wanted to leave it unstable for a release - it'll be helpful to take a moment to learn whether all-required or max-backcompat feels best.

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.

5 participants