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

Expose cross compiling configuration from pyo3-build-config #1996

Merged
merged 21 commits into from
Dec 16, 2021

Conversation

aganders3
Copy link
Contributor

@aganders3 aganders3 commented Nov 15, 2021

These modifications were proposed in #1627.

So far this exposes three functions:

  • cross_compiling (altered to take arguments, with current behavior moved to cross_compiling_from_cargo_env)
  • find_sysconfigdata
  • parse_sysconfigdata

and modifies InterpreterConfig with three new fields:

  • abi_flags
  • abi_tag
  • ext_suffix

The refactored cross_compiling takes host (triple), arch, vendor, and os, but vendor is not currently used separately from what I see in maturin. It might be better to change it to take only the complete "target triple" to simplify things, but splitting os and arch from there for CrossCompileConfig may not be straightforward as the overall format seems a bit complicated (i.e. not always only three parts). Maturin uses another crate (target_lexicon) for this, which could be pulled in here so it could perhaps take a target_lexicon::Triple. This might be the best option for supporting the most platforms.

I'm also not sure I have the correct behavior for the fields added to InterpreterConfig - specifically I'm not sure if the defaults are handled correctly. Also, do they need to change depending on the value of abi3?

It seems there's more in pyo3-build-config that could be exposed so I'm happy to bundle that in this PR or work on the different features separately.

TODO:

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.

Looking good! Few comments below.

I try to avoid adding dependencies to pyo3 and sub-crates as much as possible. It looks like target_lexicon is itself a simple dependency with no other dependencies, so it's relatively lightweight. Which platforms do you feel we're lacking support for which target_lexicon could help with?

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

Thanks for the quick feedback! I'll work on these suggestions.

@messense
Copy link
Member

Thank you for helping with this. Just a reminder, this branch has conflicts that must be resolved.

@messense
Copy link
Member

FYI, we recently merged PyO3/maturin#677 , it introduced new dependency on sysconfig.get_platform() of Python.

@aganders3
Copy link
Contributor Author

FYI, we recently merged PyO3/maturin#677 , it introduced new dependency on sysconfig.get_platform() of Python.

Hm, thanks for the heads up. This is already called by InterpreterConfig::from_interpreter but parse_sysconfigdata does not necessarily use the target interpreter.

@aganders3
Copy link
Contributor Author

I know I definitely still have a couple tests to write, but I'm a bit confused by the codecov results. Is there a good way for me to run this locally?

Otherwise I think this refactor definitely provides a better API now than before, so thanks for the suggestion! I'm not sure if the parse_sysconfigdata is what you had in mind, as this will parse the whole file where before there was a list of keys to keep.

@davidhewitt
Copy link
Member

If you go into the pyo3-build-config subdirectory you should be able to run cargo test to exercise the tests in these files? Or is there something more specific you want to run.

This is definitely looking great, let me know when you want a re-review. (I think there will only be minor nits left tbh, nice work.)

this will parse the whole file where before there was a list of keys to keep

Yes indeed I didn't state it but I was assuming that we would parse (and keep) the whole file contents 👍

@davidhewitt
Copy link
Member

(#2011 may be relevant if you're getting weird coverage results.)

@aganders3
Copy link
Contributor Author

aganders3 commented Nov 23, 2021

I wrote a few more tests to improve coverage (not pushed), but the cases where it is cross compiling all rely on environment variables, which I am having a hard time controlling for tests. I can set/unset in each test, but the default for cargo test is to run in parallel which causes intermittent failures. Do you have any suggestions?

Aside from that I am mostly happy with the interface and would appreciate feedback when you have time to review. I don't have a great feeling for whether cross_compiling should take AsRef<str> or Into<String> instead of &str for some of its args, for example. It seems maybe it should be something like this, because the host can always be borrowed but the target values need to become owned by the CrossCompileConfig if cross-compiling:

pub fn cross_compiling(
    host: AsRef<str>,
    target_arch: Into<String>,
    target_vendor: Into<String>,
    target_os: Into<String>,
) -> Result<Option<CrossCompileConfig>>

Edit: regarding tests/coverage

Thanks - I can run the tests locally (though I could get better at actually doing that before I push), but I wasn't sure how to check for a codecov failure before pushing. I guess codecov is its own thing though even if I can measure coverage locally. I was having trouble viewing the full report but that seems working now.

@davidhewitt
Copy link
Member

davidhewitt commented Nov 23, 2021

👍 I'll give this a full review in the morning, thanks!

EDIT: sorry it took me an extra day and a bit 😪

@messense
Copy link
Member

Looks good to me! I'd recommend to actually give it a try on maturin(as a git dependency) to see if it fits.

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.

Thank you, this is looking excellent!

I have a few minor tidy ups commented below, really just nits though.

I agree also that it's probably worth having a whirl with using this as a git dependency with maturin (and maybe even opening that branch as a PR on the maturin repo) to confirm that we're happy with how this works downstream before we merge.

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
pyo3-build-config/src/impl_.rs Outdated Show resolved Hide resolved
pyo3-build-config/src/impl_.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

👍 this is looking great to me from the PyO3 side, thanks!

I see you have opened PyO3/maturin#718, so will wait to merge this until you're happy with that one too. Ping me when ready!

Comment on lines +35 to +37
if cfg!(feature = "resolve-config") {
println!("cargo:rerun-if-env-changed={}", var);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidhewitt let me know if this is problematic or if there's a better way to suppress this output in maturin.

Copy link
Member

Choose a reason for hiding this comment

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

This seems like a sensible way to do it to me!

@aganders3
Copy link
Contributor Author

Okay PyO3/maturin#718 seems good to go and I think this is ready for another look. I will make a few inline comments for areas I have questions but of course appreciate any additional feedback

Merging probably needs to wait for #2040 but otherwise hopefully we can close this out soon!

@@ -826,7 +920,7 @@ fn ends_with(entry: &DirEntry, pat: &str) -> bool {
/// So we must find the file in the following possible locations:
///
/// ```sh
/// # distribution from package manager, lib_dir should include lib/
/// # distribution from package manager, (lib_dir may or may not include lib/)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully this is okay - see implementation below. This was requested in Maturin to support cross-compiling with PyPy, where the _sysconfigdata*.py is not under the lib dir.

@messense
Copy link
Member

I think we have a use case for the new Sysconfigdata in cryptography, how about exposing the current detected Python interpreter and it's sysconfigdata in pyo3-build-config for downstream consumption?

@aganders3
Copy link
Contributor Author

I think we have a use case for the new Sysconfigdata in cryptography, how about exposing the current detected Python interpreter and it's sysconfigdata in pyo3-build-config for downstream consumption?

Just as I merge main and get back to green 😆.

I can look into adding this here if you all think it's best to have in the same PR. It doesn't seem like too much work. In any case I'd be happy to take this on as another project.

@messense
Copy link
Member

I can look into adding this here if you all think it's best to have in the same PR. It doesn't seem like too much work. In any case I'd be happy to take this on as another project.

Thanks! Do it in another PR is fine.

messense referenced this pull request in pyca/cryptography Dec 14, 2021
@aganders3
Copy link
Contributor Author

No rush @davidhewitt and @messense but I'm just waiting for feedback at this point so please let me know if there's anything else needed here! My PR in Maturin is passing tests while pointed to the latest in this branch.

Otherwise I will start looking into the above request to expose more features of pyo3-build-config for use in cryptography.

@messense
Copy link
Member

Looks good to me. Let's give @davidhewitt a chance to take another look before merging.

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.

Looks brilliant! Thank you for all the work put into this, and I'm sorry it took me so long to find time to review.

Comment on lines +35 to +37
if cfg!(feature = "resolve-config") {
println!("cargo:rerun-if-env-changed={}", var);
}
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a sensible way to do it to me!

@davidhewitt davidhewitt merged commit c30ca03 into PyO3:main Dec 16, 2021
aganders3 pushed a commit to aganders3/pyo3 that referenced this pull request Dec 16, 2021
davidhewitt added a commit that referenced this pull request Dec 16, 2021
@aganders3 aganders3 deleted the expose-cross-compiling branch December 16, 2021 21:34
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.

3 participants