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

remove 'm' and 'd' ABI tags for Python 3.8 wheels #2121

Merged
merged 7 commits into from Apr 21, 2020

Conversation

koehlma
Copy link
Contributor

@koehlma koehlma commented Mar 2, 2020

Hey everyone,

as of Python 3.8 the m and d ABI flags became obsolete (see 1 and 2). In fact, if present, they do prevent the installation of wheels. However, poetry seems to add those flags anyway. As a result, I cannot install wheels build with poetry build on the same system they were built on.

This pull request fixes a RuntimeWarning if the flags are not present in the environment and prevents the flags from being added to the ABI tag if the Python version is >= 3.8.

Cheers,
Maximilian

Resolves: sdispater/pendulum#456

@finswimmer finswimmer requested a review from Mar 2, 2020
@kasteph kasteph added the Bug Something isn't working as expected label Mar 27, 2020
Copy link
Member

@kasteph kasteph left a comment

Thanks for your first contribution! 🚀

It would be great if you could write a test for this -- one doesn't exist yet but you can put it under tests/masonry/utils.

@nirvana-msu
Copy link

@nirvana-msu nirvana-msu commented Apr 15, 2020

I can confirm I faced the exact same issue. Any binary wheel built with Poetry 1.0.5 for Python 3.8 under Windows will have m appended to the tag - which in turn makes the wheel impossible to install as per pypa/pip/issues/6885 & pypa/pip/pull/6874

@sdispater @stephsamson this seems like critical issue, with a very simple fix. Would be great if you find some time to review/merge this PR. Btw even pendulum wheel is broken due to the same issue: sdispater/pendulum/issues/456

@koehlma
Copy link
Contributor Author

@koehlma koehlma commented Apr 15, 2020

It would be great if you could write a test for this -- one doesn't exist yet but you can put it under tests/masonry/utils.

In principle I could do that, however, I find it a bit hard to get started as I do not know where the env object is coming from and what its general structure is. If someone could write a skeleton and construct a mock env, I will provide a test ASAP. Unfortunately, I currently lack the time to dig deeper into this.

poetry/masonry/utils/tags.py Show resolved Hide resolved
poetry/masonry/utils/tags.py Outdated Show resolved Hide resolved
@frostming
Copy link
Contributor

@frostming frostming commented Apr 15, 2020

@koehlma I added some review comment.

For testing, you can construct a mocked env with

from poetry.utils.env import MockEnv

env = MockEnv(version_info=(3, 8, 0))

@koehlma
Copy link
Contributor Author

@koehlma koehlma commented Apr 16, 2020

Here you go. I wrote a test and adapted the MockEnv such that I can overwrite config variables.

Copy link
Member

@abn abn left a comment

Changes look great. One comment, a good to have.

tests/masonry/utils/test_tags.py Show resolved Hide resolved
@koehlma koehlma requested a review from abn Apr 20, 2020
abn
abn approved these changes Apr 21, 2020
@abn abn merged commit dbc1e1b into python-poetry:master Apr 21, 2020
16 checks passed
@sdispater sdispater mentioned this pull request Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants