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: Add python3-dll-a crate support #2282

Merged
merged 9 commits into from
Apr 11, 2022

Conversation

ravenexp
Copy link
Contributor

@ravenexp ravenexp commented Apr 7, 2022

Automatically generate python3.dll import libraries
for Windows compile targets in the build script.

Adds a new PyO3 crate feature auto-abi3-import-lib enabling
automatic import library generation.

Closes #2231

WIP: The documentation is still a TODO

@ravenexp ravenexp marked this pull request as draft April 7, 2022 12:56
Cargo.toml Outdated Show resolved Hide resolved
@ravenexp ravenexp marked this pull request as ready for review April 8, 2022 09:30
@ravenexp ravenexp requested a review from adamreichold April 8, 2022 09:30
@@ -230,6 +234,14 @@ When cross-compiling, PyO3's build script cannot execute the target Python inter
* `PYO3_CROSS_PYTHON_VERSION`: Major and minor version (e.g. 3.9) of the target Python installation. This variable is only needed if PyO3 cannot determine the version to target from `abi3-py3*` features, or if `PYO3_CROSS_LIB_DIR` is not set, or if there are multiple versions of Python present in `PYO3_CROSS_LIB_DIR`.
* `PYO3_CROSS_PYTHON_IMPLEMENTATION`: Python implementation name ("CPython" or "PyPy") of the target Python installation. CPython is assumed by default when this variable is not set, unless `PYO3_CROSS_LIB_DIR` is set for a Unix-like target and PyO3 can get the interpreter configuration from `_sysconfigdata*.py`.

An experimental `pyo3` crate feature `generate-abi3-import-lib` enables the user to cross compile
"abi3" extension modules for Windows targets without setting `PYO3_CROSS_LIB_DIR` environment
variable or providing any Windows Python library files. It uses external `python3-dll-a` crate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A link to the crate homepage might be a nice courtesy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a link to docs.rs page. As for the GitHub project page, I actually hoped to transfer that project to the PyO3 organization after this feature is done. Unfortunately, there's a chance I might lose access to my GitHub or Gmail accounts because of the circumstances, and I don't want PyO3 to end up with an unmaintainable dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added links to the docs.rs page to both the user guide and Architecture.md.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look fine to me. Leaving this unresolved due to the question of long-term maintenance though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to be misunderstood here. I do intend to maintain it and to contribute to PyO3 in the future, but I also want to safeguard against the situation where I'd be unable to do so. GitHub has blocked accounts of users from sanctioned countries before, and if that happens I don't want it to become a problem for the projects that depend on mine. That's why I want to transfer the GitHub project and the crates.io package ownership to an organization as a backup measure.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay on my part, thanks @adamreichold for reviewing.

Last thing that would be nice to see IMO would be a check for this in CI. I think it could probably be done similarly to the other cross compilation check -

- name: Test cross compilation

@ravenexp
Copy link
Contributor Author

Last thing that would be nice to see IMO would be a check for this in CI. I think it could probably be done similarly to the other cross compilation check -

I'll try to figure out how to do it, but it'll probably take some time. I've only used GitLab CI before, and GitHub Actions are a complete mystery to me. Everybody just calls somebody else's actions, and those actions call somebody else's actions, and it's difficult for me to figure out where the resulting shell commands even come from...

@davidhewitt
Copy link
Member

davidhewitt commented Apr 10, 2022

I think it'll be something like this:

      - name: Test windows python3-dll-a cross compilation
        if: ${{ matrix.platform.os == 'ubuntu-latest' && matrix.python-version == '3.9' }}
        uses: messense/maturin-action@v1
        with:
          target: x86_64-pc-windows-msvc
          args: --release -i python3.9 --no-sdist -m examples/maturin-starter/Cargo.toml --cargo-extra-args="--features=pyo3/generate-abi3-import-lib --features=pyo3/abi3"

(might need env: PYO3_NO_PYTHON too? Not sure if maturin will set it.)

@ravenexp
Copy link
Contributor Author

(might need env: PYO3_NO_PYTHON too? Not sure if maturin will set it.)

That's the issue with using maturin: it has its own cross-compilation logic and abi3 bindings handling logic, which get special treatment. It used to set PYO3_NO_PYTHON and alter RUSTFLAGS accordingly, but apparently it does not do that anymore after PyO3/maturin#851 was merged.

I'd rather start with plain cargo build --target ... and proceed from there, if that's possible.

@adamreichold
Copy link
Member

I'd rather start with plain cargo build --target ... and proceed from there, if that's possible.

While I understand aiming for simplicity to be able to follow things and understand errors, I also think that a CI job should be as close as possible to what actual usage will look like. Which I expect to go through Maturin most of the time (definitely, in the case where I would like to use this: building Windows wheels using MinGW in GitLab CI which I currently do by checking in the import libraries into source control). Or putting it differently, if Maturin's abi3 handling logic interferes with using this features, this might become a problem for the intended use cases which a Maturin-based CI job would only surface early (which seems a good thing to me).

@davidhewitt
Copy link
Member

cc @messense - I'd be interested in your opinion on how we might be able to test this (if you get a chance to see this considering the situation in Shanghai, no worries if you are unable to comment atm)

@ravenexp
Copy link
Contributor Author

While I understand aiming for simplicity to be able to follow things and understand errors, I also think that a CI job should be as close as possible to what actual usage will look like. Which I expect to go through Maturin most of the time (definitely, in the case where I would like to use this: building Windows wheels using MinGW in GitLab CI which I currently do by checking in the import libraries into source control).

Well, in my tests both cargo build and maturin build commands work (for the current version of maturin =0.12.12). I wouldn't submit this if they didn't. But, maturin's cross-compilation approach changes from version to version, and it is currently in the process of eliminating its internal PYO3_NO_PYTHON use, which will hopefully simplify things a lot.

Definitely, in the case where I would like to use this: building Windows wheels using MinGW in GitLab CI which I currently do by checking in the import libraries into source control).

This is my main use case as well, and this works with just maturin build --target x86_64-pc-windows-gnu.

My original point was that, preferably, both commands should be tested, because I've encountered the situations where one of them works and the other one doesn't, like PyO3/maturin#824 (comment). Solving issues like that is hard when no additional information is available.

@adamreichold
Copy link
Member

My original point was that, preferably, both commands should be tested

This certainly sounds like a good idea if even more work to implement.

@messense
Copy link
Member

I'll try to figure out how to do it, but it'll probably take some time. I've only used GitLab CI before, and GitHub Actions are a complete mystery to me.

I can work on adding tests to CI by pushing to your branch once this PR is rebased.

Automatically generate `python3.dll` import libraries for Windows
compile targets in the build script.

Adds a new PyO3 crate feature `generate-abi3-import-lib` enabling
automatic import library generation.

Closes PyO3#2231
Update the user guide to describe its applicability to the native
and cross-compilation build scenarios.
@ravenexp
Copy link
Contributor Author

I'll try to figure out how to do it, but it'll probably take some time. I've only used GitLab CI before, and GitHub Actions are a complete mystery to me.

I can work on adding tests to CI by pushing to your branch once this PR is rebased.

I've rebased the PR branch on the current main. This would be a huge help, thanks!

Cross and native compilation to *-pc-windows-gnu targets should work out of the box, and cross-compilation to *-pc-windows-msvc I've only tested with rust-lld as the linker. I don't know how to run link.exe on Linux.

*-msvc targets also need LLVM binutils for llvm-dlltool and the MSVC runtime import libraries to link, as usual.
I don't use the native MSVC toolchain myself, so I can't help much here.

.github/workflows/ci.yml Show resolved Hide resolved
On Unix-like systems this works unconditionally; on Windows you must also set the `RUSTFLAGS` environment variable
to contain `-L native=/path/to/python/libs` so that the linker can find `python3.lib`.

If the `python3.dll` import library is not available, an experimental `generate-abi3-import-lib` crate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed it would also be great to mention this in features.md

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 4347624

Update the feature list section of the user guide
to include `generate-abi3-import-lib` description.
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks all!

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.

Generate python3.dll import libraries for Windows targets in the build script
4 participants