Add export-config feature to pyo3-build-config

[aganders3]

This is intended to address the needs outlined in #2033.

Basically how this works is that a dependent crate would add pyo3-build-config as a build dep and enable the export-config feature. I made this a feature (as opposed to e.g. an environment variable) such that it could be configured via Cargo.toml in the dependent crate (though I think newer versions of cargo may allow you to set environment variables there as well...).

Enabling this feature causes the resolved pyo3 config to be written out during compilation. It is written in the standard pyo3 config file format because the functions for reading/writing this config already existed. This file is written to OUT_DIR (I think it has to be).

The only means I could find to pass information from one build script to another was the links manifest key:

The purpose of this manifest key is to give Cargo an understanding about the set of native dependencies that a package has, as well as providing a principled system of passing metadata between package build scripts.

Fortunately this was already enabled in the pyo3 crate for "python" which at least somewhat makes sense for this purpose. So in addition to writing out the config I added a key/value such that the config can be read back in by a dependent crate build script using the path in DEP_PYTHON_PYO3_EXPORT_CONFIG. Fortunately however this is all transparent to a dependent user, as the crate that actually reads the file back in is pyo3-build-config itself!

In addition I exposed two functions implemented on InterpreterConfig that allow running a python script using the resolved interpreter with and without additional environment variables (e.g. PYTHONPATH).

Feedback on this design is very welcome! It took me a while to figure out how to pass info from one build script to another.

TODO

• add an entry in CHANGELOG.md
• add docs for all new functions and / or detail in the guide
• add tests for all new or changed functions

[davidhewitt]

Thank you for this! I will do my best to find a chance to review within the next few days.

[aganders3]

This is failing patch coverage slightly, but I'm not sure it makes sense to test the un-covered lines highlighted as they involve environment variables and unlikely error cases. If it's a blocker I can refactor or come up with something.

[davidhewitt]

Sorry that I'm still failing to get to this. I currently have a clear agenda on Sunday (OMG) so I have every intention to sit down and write a review then.

This is failing patch coverage slightly, but I'm not sure it makes sense to test the un-covered lines highlighted as they involve environment variables and unlikely error cases. If it's a blocker I can refactor or come up with something.

I think that's fine; this is an awkward bit of the code base to cover because of the reasons you mention.

[aganders3]

No worries! I appreciate the update but also understand this is not the most pressing feature/issue in the project or your queue. Whenever you have time is fine with me. It's typically only a minor chore to keep this rebased on main and ready to merge, since this crate is not changing too quickly.

[davidhewitt]

Thank you for working on this, and sorry it's taken me so long to get to review this.

Finally starting to get my head around it; I've started by putting a few suggestions / asking some questions...

[aganders3]

Thanks for the input! I'll do another iteration to make it a bit simpler based on your suggestions.

[aganders3]

I (finally 🙁) found time to pick this back up but it looks like there have been a few changes even quite recently.

If this is still wanted my plan would be to emit to a dependency/link env var (as before) but serialize the whole InterpreterConfig as suggested above. Since the Python "link" has now changed to pyo3-ffi it would have to be set in the build script there.

[davidhewitt]

Sounds good to me 👍

[davidhewitt]

Looking great, thanks! Just a couple of final tweaks I think.

[davidhewitt]

Looks great to me, thanks for finishing this off! As before, do you want to test this against cryptography using a git dependency before we merge, or should I just go ahead and merge it?

[aganders3]

After all this time I might as well run the test either with cryptography or at least an example package. It would be annoying to find out there's some issue after merging. I'll try to test that in the next day or so!

[aganders3]

Okay I ran this on that cryptography PR and it seems to work but is basically stuck in the same spot it was before.

I also created a small test crate and it works as expected there: https://github.com/aganders3/hello-pyo3-build-config
You can't see the output in the CI run but running locally I can see it in the output file:

❯ cat ./target/debug/build/hello-pyo3-build-config-b6062149f81c7f0e/output
cargo:rustc-cdylib-link-arg=-undefined
cargo:rustc-cdylib-link-arg=dynamic_lookup
Hello from a Python build script run using `/Users/aanderson/src/oss/test-crates/hello-pyo3-build-config/.venv/bin/python`!

One quirk I noticed when building that is that it panics with abi3-py37, but I guess this makes sense as it does not require an interpreter for that.

Let me know if there's anything else you'd like tested or demonstrated!

[davidhewitt]

Great!

Okay I ran this on that cryptography PR and it seems to work but is basically stuck in the same spot it was before.

To clarify, you mean that this is working but that PR needs further implementation on the cryptography side once this lands?

[aganders3]

To clarify, you mean that this is working but that PR needs further implementation on the cryptography side once this lands?

Oh sorry my wording was indeed confusing. Yes you are correct - the remaining work would be on the cryptography side.

In my opinion this PR is good to go!

[davidhewitt]

Excellent! Many thanks again 👍