Skip to content

Conversation

alex
Copy link
Contributor

@alex alex commented Aug 6, 2020

Still doing testing, but getting this branch up now for visibility.

Fixes #76

@alex alex marked this pull request as ready for review August 6, 2020 22:18
@alex
Copy link
Contributor Author

alex commented Aug 6, 2020

Ok, confirmed that without this patch I get an error about non-matching. And with this patch I get a build .pyd (well, first I got an error that I needed to rustup add the i686 target).

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 for this! If you've got a travis configuration to hand, it would be nice to run the tests with this 32-bit windows configuration.

(Alternatively, if you've only got a Github actions configuration to hand, then I was thinking about migrating to actions anyway?)

@alex
Copy link
Contributor Author

alex commented Aug 6, 2020

Yeah, I don't have a travis windows configuration handy -- I did my testing in a manual EC2 VM.

If you're porting to Github Actions, you should be able to peanutbutter+chocolate the current tests with: https://github.com/pyca/cryptography/blob/master/.github/workflows/ci.yml#L55 to get a 32-bit python on a 64-bit windows.

@alex
Copy link
Contributor Author

alex commented Aug 6, 2020

Sorry, just realized I may have misread. Were you asking that I add the GHA build to this PR?

@davidhewitt
Copy link
Member

Sorry, just realized I may have misread. Were you asking that I add the GHA build to this PR?

I'm working on the GHA build on this branch: master...davidhewitt:github-actions

At the moment I'm encountering an interesting CI failure 😅

@alex
Copy link
Contributor Author

alex commented Aug 6, 2020

@davidhewitt I think your problem is that writing .cargo/config is done with just some string formatting, not a TOML library, therefore strings with \ in them, such as a Windows path, look like escapes.

I bet if you used a toml lib at tomlgen.py L120, that'd fix it.

(I suspect this means tomlgen's .cargo/config support never worked on Windows)

@davidhewitt
Copy link
Member

(I suspect this means tomlgen's .cargo/config support never worked on Windows)

Haha yeah this is exactly the conclusion I've come to! 👍

@alex
Copy link
Contributor Author

alex commented Aug 7, 2020

Hmm, so that changelog is slightly off I think. It's "fixed building for 32-bit Pythons on 64-bit Windows"

@davidhewitt
Copy link
Member

davidhewitt commented Aug 7, 2020

Thanks, I've tweaked that with a couple extra commits (oops); I'll squash it all anyway!

Having gotten most of the way with the Github Actions branch, it looks like pypy3 defaults to 32-bit anyway, so I'm going to merge this first and then sort the testing out myself. (https://github.com/davidhewitt/setuptools-rust/runs/956173939?check_suite_focus=true)

Thanks again for this!

@davidhewitt davidhewitt merged commit 2a23f90 into PyO3:master Aug 7, 2020
@alex
Copy link
Contributor Author

alex commented Aug 7, 2020

Sure thing!

It's going to be a bit of a road ahead, but I'm super excited about the possibility of getting pyca/cryptography using pyo3!

@alex alex deleted the 32-on-64-wow branch August 7, 2020 00:09
@davidhewitt
Copy link
Member

Likewise; looking forward to seeing the road ahead! 🎉

@alex
Copy link
Contributor Author

alex commented Aug 7, 2020

@davidhewitt unfortunately because of pep517, there's no way to take advantage of this until there's a release on pypi. Would you be up for doing a release with this? (Totally understand if you want to wait for #78 to be finished!)

@davidhewitt
Copy link
Member

I think #78 might be blocked until pyo3 0.12 comes out with "better" support for pypy, so I'll go ahead and release now.

@alex
Copy link
Contributor Author

alex commented Aug 7, 2020 via email

@alex
Copy link
Contributor Author

alex commented Sep 12, 2020

Hmm, so this was broken by the 0.12 release, which now errors with:

Error: "Must provide PYO3_CROSS_INCLUDE_DIR environment variable when cross-compiling"

I suppose I should just extend this to auto-set that var.

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.

Specify --target automatically based on which python is in use?
2 participants