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

Add wheels for arm64 macos #1534

Merged
merged 7 commits into from
Mar 12, 2021
Merged

Add wheels for arm64 macos #1534

merged 7 commits into from
Mar 12, 2021

Conversation

glandium
Copy link
Contributor

@glandium glandium commented Mar 3, 2021

This also refactors wheels creation to allow cross-compilation in a smoother manner.

@glandium glandium force-pushed the arm64-macos branch 7 times, most recently from d6d95e0 to ea27194 Compare March 4, 2021 02:28
@glandium glandium marked this pull request as ready for review March 4, 2021 02:29
@auto-assign auto-assign bot requested a review from badboy March 4, 2021 02:29
@glandium
Copy link
Contributor Author

glandium commented Mar 4, 2021

The failure on Android tests don't seem related.

@badboy
Copy link
Member

badboy commented Mar 4, 2021

The failure on Android tests don't seem related.

Yeah, ignore those. Will need to look into them separately.

All versions of python >= 3.2 are compatible with abi3. cp36 indicates
compat with >= 3.6.

The wheels module used to create wheels with py3_none on Windows, but
that was, as far as I know, a bug, and wheels with cp3*_abi3 work just
fine.
This avoids special-casing in the CI configuration, allowing to build
locally with less extra manual steps.
The rust host triple is its default target. Instead of relying on the
python platform, we now rely on that host triple to make decisions,
which is more likely to fit what we really want.

And now that we know the default target for the rust compiler, we can
also always explicitly pass it, such that the shared library path is
consistently under target/$target, which simplifies things.

At the same time, we stop assuming "linux" and "darwin" means x86_64.
This allows a more general way to use a custom target, and use that for
Windows wheels on CI.
The python wheel says 11.0 for arm64 and 10.7 otherwise, but the crate
has dependencies that build C code that could pull symbols that aren't
available in older version of macOS, because of building against a newer
SDK.

Although practically speaking, it was probably not happening.
Technically, though, rust code could be doing the same and that wouldn't
be detected.
@glandium
Copy link
Contributor Author

glandium commented Mar 4, 2021

I just realized there was temporary garbage in the history. Removed them.

@badboy badboy requested a review from mdboom March 4, 2021 10:59
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

I like it! The part for arm64 macos looks good.
Moving around where and how we build the wheel looks fine as well.
As long as from-source-and-build-it continues to work, we can land this.

Defering to @mdboom for a final look, because he's the Python expert here.

Copy link
Contributor

@mdboom mdboom 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. Generally a nice cleanup of the setup.py, especially the standard way to cross compile anything.

Before merging, we should confirm that:

  1. The new Windows wheels produced by this work with the different pattern matching values
  2. Source installation through a non-wheel-provided platform (e.g. OpenBSD) still works.

return super().get_tag()
_, __, plat_name = super().get_tag()

return ("cp36", "abi3", plat_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you confirmed this works with Windows wheels? The pattern matching by pip is slightly different there, hence the weird hard coding of all of the supported options that was here before. (Related, let's update the comment above this function if this new approach does work).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It worked for me locally, but it also obviously works more generally, since Windows python tests on CI use the wheels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note the comment above get_tag is still relevant as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just reminded myself of the details of the issue (which we should probably put in a comment so it isn't lost again).

Windows Python prior to 3.7 does not have a stable ABI, so therefore doesn't support the abi3 tag. Therefore, it won't "match" that wheel and it falls back to installing from source. You can test this locally by putting the wheels in a local directory and forcing pip to look there instead of PyPI with the -f directory option. It won't work with the wheel named as this PR does it.

Of course, we don't actually use the Python3 ABI since we use CFFI, so the best way to make a wheel that will work on Windows Python 3.6 and up is to mark it with py3-none rather than cp36-abi3.

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'm not sure this is the full story. Because while installing a fresh Windows Python 3.6 and running pip install glean_sdk-35.0.0-cp36-abi3-win_amd64.whl doesn't work, pip tells you it's version 18.1 and that you should upgrade to 21.0.1, and after doing that, it works. Trying a few versions, it started working with pip 20.0.1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Installing directly from a file path always works. What doesn't is the pattern matching engine. So you need to do:

pip install -f directory_containing_wheels glean_sdk

to match what would happen in normal situations when installing from pypi.

This doesn't work for me on Python 3.6, even when upgrading pip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Installing directly from a file path always works

That's not my experience:

# ./AppData/Local/Programs/Python/Python36/python -m pip install glean_sdk-35.0.0-cp36-abi3-win_amd64.whl
glean_sdk-35.0.0-cp36-abi3-win_amd64.whl is not a supported wheel on this platform.
You are using pip version 18.1, however version 21.0.1 is available.
You should consider upgrading via the 'python -m pip install --upgrade pip' command.

This doesn't work for me on Python 3.6, even when upgrading pip.

That doesn't look to be true (after upgrading to pip 21.0.1):

# ./AppData/Local/Programs/Python/Python36/python -m pip install -f . glean_sdk
Looking in links: .
Processing c:\users\glandium\glean_sdk-35.0.0-cp36-abi3-win_amd64.whl
(...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for that. I think I wasn't upgrading pip correctly.

So then I think the only thing to confirm is that we aren't held back to a particular version of pip by how things are vendored in m-c. Assuming that checks out, 👍 from me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for making me look. Things are grim, unfortunately, as the common way to install and upgrade moz-phab on Windows uses pip 19.3 :(

cp36-abi3 only works with pip >= 20, but we still have use-cases on pip
19.3.
@mdboom mdboom enabled auto-merge (squash) March 10, 2021 19:15
@mdboom mdboom disabled auto-merge March 10, 2021 19:16
@badboy badboy closed this Mar 12, 2021
@badboy badboy reopened this Mar 12, 2021
@mdboom mdboom merged commit 1dda09f into mozilla:main Mar 12, 2021
@glandium
Copy link
Contributor Author

Any idea yet when the next release is going to be?

@badboy
Copy link
Member

badboy commented Mar 15, 2021

Any idea yet when the next release is going to be?

This week. I'll probably get one done today or tomorrow.

@badboy
Copy link
Member

badboy commented Mar 16, 2021

Upload failed. Tracking here: https://bugzilla.mozilla.org/show_bug.cgi?id=1698860

We can manually upload that wheel when we figure out what platform tag we need.

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