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: Make PYO3_CROSS_LIB_DIR optional #2241

Merged
merged 4 commits into from
Apr 2, 2022

Conversation

ravenexp
Copy link
Contributor

Try to support most common extension module cross compilation targets
without requiring any of the PYO3_CROSS_* environment variables.

This is a work in progress tangentially related to #2231
I'm creating this mostly to get some initial feedback from the pyo3-build-config maintainers.

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.

Thanks; to check I understand the idea, it's that when PYO3_CROSS_LIB_DIR is not set then (for unix extension modules at least) we can infer all we need from the abi3 and version settings?

Also, a fair number of functions have changed to be pub fn. Was that necessary?

.gitignore Outdated Show resolved Hide resolved
pyo3-build-config/src/impl_.rs Outdated Show resolved Hide resolved
pyo3-build-config/src/impl_.rs Show resolved Hide resolved
pyo3-build-config/src/impl_.rs Show resolved Hide resolved
pyo3-build-config/src/impl_.rs Outdated Show resolved Hide resolved
pyo3-build-config/src/impl_.rs Outdated Show resolved Hide resolved
@ravenexp
Copy link
Contributor Author

Thanks; to check I understand the idea, it's that when PYO3_CROSS_LIB_DIR is not set then (for unix extension modules at least) we can infer all we need from the abi3 and version settings?

Yes, that's the general idea. I needed to make PYO3_CROSS_LIB_DIR optional to allow python3-dll-a to substitute its own library directory located somewhere inside OUT_DIR. In the process of doing that, I've realized it can also work for Unix-to-Unix cross compilation in many cases.

It's not entirely viable yet though, because of Android as you've mentioned above and also projects embedding a Python interpreter on Unix need to link to a real libpython3*.so somehow. This can be solved for Windows and abi3, but not in the general case.

This is a work in progress, and I need to add a clear error message for every currently accepted but broken configuration.

Also, a fair number of functions have changed to be pub fn. Was that necessary?

I've noticed that several PyO3 build scripts contained literal re-implementations of these functions, so I intended to unify them in later commits. This commit alone is getting way too big, perhaps I should further split it into the refactoring and the breaking change parts for easier review... Also the bug fix part needs to be split out.

@davidhewitt
Copy link
Member

👍 makes sense, yes I think several commits (or even PRs) will be needed to get to the final goal for this one. These build scripts are quite fiddly, there's so many ways to distribute Python leading to lots of edge cases to handle!

@ravenexp
Copy link
Contributor Author

My current plan is to submit a new PR with ABI3_MAX_MINOR bug fix only, and convert the current PR into 2+ commits on top of it. I've added a lot of new code that doesn't have test coverage, so it needs to be addressed too...

It it OK to rebase and force push pull requests for this project, or do you prefer that I use merges and fixup commits, so that you can squash them yourself before the final merge?

@davidhewitt
Copy link
Member

Rebase & force push is fine! Thanks 👍

@ravenexp
Copy link
Contributor Author

I'm resubmitting PR this as a set of smaller patches to get continuous feedback from the CI.

It's still a work in progress, but feel free to review the commits I've added so far. I'm highly confident in these, so if changes are requested the sooner I learn about them the better.

self.os == "windows"
}

fn is_windows_mingw(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Why not is_windows_gnu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the convention the existing code uses:

print("calcsize_pointer", struct.calcsize("P"))
print("mingw", get_platform().startswith("mingw"))
"#;

version: PythonVersion,
implementation: PythonImplementation,
abi3: bool,
mingw: bool,
) -> String {
if abi3 && !implementation.is_pypy() {
WINDOWS_ABI3_LIB_NAME.to_owned()
} else if mingw {
// https://packages.msys2.org/base/mingw-w64-python
format!("python{}.{}", version.major, version.minor)
} else {

@ravenexp
Copy link
Contributor Author

I've just pushed version 2 of this PR, split into 5 smaller commits this time.
User documentation and the ChangeLog entry are still missing,
but I'll add them once all remaining code issues are solved.

Unresolved questions from the previous rounds of review (shortlist):

  • Replacing ad-hoc TargetInfo with target_lexicon::Triple. If the answer is yes, seems easier to address it in this PR.
  • Adding PYO3_CROSS_PYTHON_IMPLEMENTATION environment variable. This can be addressed in a different PR because it involves writing new user documentation and adding a ChangeLog entry.
  • Cross-compilation mode detection. See pyo3-build-config: Make PYO3_CROSS_LIB_DIR optional #2241 (comment) and the later discussion. I think I've solved it in the current revision, but it's better reviewed once again.

@adamreichold
Copy link
Member

Replacing ad-hoc TargetInfo with target_lexicon::Triple. If the answer is yes, seems easier to address it in this PR.

In my experience, build scripts tend to become choke points in parallel builds which is why I would be wary of adding more dependencies to build scripts. That said, target-lexicon seems lightweight enough and seems to have good provenance with the Bytecode Alliance.

@adamreichold
Copy link
Member

Adding PYO3_CROSS_PYTHON_IMPLEMENTATION environment variable. This can be addressed in a different PR because it involves writing new user documentation and adding a ChangeLog entry.

More smaller PR are preferable IMHO.

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.

Yes, I think this revision looks ok to me.

I had been wondering whether we would need to use target_lexicon, it would definitely be worth having a go in a separate commit so we can see how much it helps 👍

@ravenexp
Copy link
Contributor Author

Thanks!

I had been wondering whether we would need to use target_lexicon, it would definitely be worth having a go in a separate commit so we can see how much it helps +1

I'll add this commit on top to evaluate the extent of the required changes, but I'd like to fold it in the first commit for the final revision to reduce useless code churn within the same PR.

@ravenexp
Copy link
Contributor Author

In my experience, build scripts tend to become choke points in parallel builds which is why I would be wary of adding more dependencies to build scripts. That said, target-lexicon seems lightweight enough and seems to have good provenance with the Bytecode Alliance.

The issue with target-lexicon is that it itself has a quite extensive build script, which among other things tries to run rustc to detect its version. Unfortunately, it can't be disabled via crate features.

@davidhewitt
Copy link
Member

Right, yes I think that would make it a lot less lightweight than we were hoping. I guess you decided not to try using it as a result?

Anything else to consider before merging this?

@ravenexp
Copy link
Contributor Author

Right, yes I think that would make it a lot less lightweight than we were hoping. I guess you decided not to try using it as a result?

I can still try to add it as an experiment. I haven't actually checked if it has any measurable impact on the build times.

Anything else to consider before merging this?

Yes, updating user documentation and adding the changelog entry are still a TODO.

@ravenexp
Copy link
Contributor Author

ravenexp commented Mar 25, 2022

Right, yes I think that would make it a lot less lightweight than we were hoping. I guess you decided not to try using it as a result?

I can still try to add it as an experiment. I haven't actually checked if it has any measurable impact on the build times.

I've added d787d2a on top of the current branch to evaluate the target_lexicon::Triple integration impact.

I think there's a demonstrable benefit from the code quality standpoint.

The downsides:

  1. Build times: A simple PyO3 extension module build time increased (on average) from 15.05s to 15.25s (+0.2s) because of the new target-lexicon dependency and its build script.
  2. I had to break the public crate API in pyo3_build_config::cross_compile() function because there's just not enough information to make sound decisions based on the existing function arguments.

In my opinion the build time cost is acceptable, and as for cross_compile() I'm not sure if it has any external users.
PyO3 and maturin themselves do not use it. Maybe it's better to deprecate it altogether.

@messense
Copy link
Member

and as for cross_compile() I'm not sure if it has any external users.
PyO3 and maturin themselves do not use it. Maybe it's better to deprecate it altogether.

It was supposed to be used to replace the duplicated code in maturin, see PyO3/maturin#718

@ravenexp
Copy link
Contributor Author

ravenexp commented Mar 25, 2022

and as for cross_compile() I'm not sure if it has any external users.
PyO3 and maturin themselves do not use it. Maybe it's better to deprecate it altogether.

It was supposed to be used to replace the duplicated code in maturin, see PyO3/maturin#718

Can this code be adapted to the new cross_compile() signature?
As I can see maturin already uses Triple internally.
This would delay the feature merge until the next major PyO3 release, though...

@messense
Copy link
Member

Can this code be adapted to the new cross_compile() signature?

I think it's ok.

This would delay the feature merge until the next major PyO3 release, though...

I think it's fine to break that api since it was tailored for maturin anyway.

@davidhewitt
Copy link
Member

I think it's fine to break that api since it was tailored for maturin anyway.

Even if someone else using it is unlikely, I would still prefer not change cross_compiling signature until the next breaking release. I think anyone who is using it would have good reason to be upset if we broke them in a semver-noncompliant way.

I'm thinking to put out a 0.16.3 release in the next few days (just one last thing I'd like to add to it), and then I'd be happy to start merging breaking changes because I've got ideas anyway 😄

@ravenexp
Copy link
Contributor Author

Even if someone else using it is unlikely, I would still prefer not change cross_compiling signature until the next breaking release. I think anyone who is using it would have good reason to be upset if we broke them in a semver-noncompliant way.

I can try to keep this function as is and try to create a somewhat correct implementation for it until it's deprecated/removed in v0.17.
As a substitute, I'll add a new public API function that uses Triple arguments.

@ravenexp
Copy link
Contributor Author

I've pushed the target-lexicon patch version 2 in fe9c160

Here I'm trying to keep the existing cross_compile() implementation afloat and introduce a new public function cross_compile_from_to() that uses Triple arguments directly.

If everyone is OK with moving on with target_lexicon::Triple, I'd rather split it off in a separate PR with its own ChangeLog entry and then rebase the current PR on top of it.

@davidhewitt
Copy link
Member

Sounds good to me 👍

@ravenexp ravenexp marked this pull request as draft March 25, 2022 15:34
@ravenexp
Copy link
Contributor Author

Rebased on top of #2253

Change the `CrossCompileConfig` structure definition and make
the public `lib_dir` field optional to support more flexible
cross-compilation configuration in the future.

FIXME: This change breaks the public `pyo3-build-config` crate API.

Update the sysconfigdata extraction functions to fall through
when `lib_dir` field is not set.

WIP: Add `unwrap()` stubs to the main cross compile switch.
Try to generalize `windows_hardcoded_cross_compile()`
to all supported target platforms (when possible).
Rename it to `default_cross_compile()` and add some unit tests.

Rewrite `load_cross_compile_config()` to fall back to
the default interpreter configuration when no other config
information sources are available.
Rename `$OUT_DIR/pyo3-cross-compile-config.txt` to
`$OUT_DIR/<triple>/pyo3-build-config.txt` to exclude the possibility
of using stale build configuration data when the build target changes.

Use the presence of the corresponding build configuration file
in the `pyo3-build-config` build script output directory
to detect whether we are cross compiling or not.

This patch enables cross compilation without using
any of `PYO3_CROSS_*` env variables in many cases.
Update Architecture.md to reflect the current cross compilation
support state.
@ravenexp
Copy link
Contributor Author

ravenexp commented Apr 1, 2022

Rebased on main and reopened.

@ravenexp ravenexp marked this pull request as ready for review April 1, 2022 08:43
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.

Thanks, looks good to me. Thanks all for reviewing on this; I didn't manage to find any time to look for a number of days.

@davidhewitt davidhewitt merged commit 040ce86 into PyO3:main Apr 2, 2022
@ravenexp ravenexp deleted the cross-compile branch April 2, 2022 20:49
@ravenexp ravenexp mentioned this pull request Apr 4, 2022
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.

4 participants