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

Use "environment markers" for conditional install_requires #2111

Merged
merged 1 commit into from
Jul 9, 2021

Conversation

tianon
Copy link
Member

@tianon tianon commented Jul 7, 2021

These are defined in PEP 508: https://www.python.org/dev/peps/pep-0508/#environment-markers

The current implementation means that installing 1.0a2 from the published wheel on Python 3.8 does not include astor (and thus does not work) because "the Python version that is building the wheel is different from the Python version that is installing it" (from https://hynek.me/articles/conditional-python-dependencies/).

I don't think Windows and Linux can share wheels, so pyreadline doesn't strictly have to be included here, but it seems more correct to use this method for both.

(Refs 06f34b0 / #1999)

@tianon
Copy link
Member Author

tianon commented Jul 7, 2021

I guess at this point it might even make sense to embed this list directly in the setup() invocation again? (Happy to adjust, as always 👍)

@Kodiologist
Copy link
Member

Whoops. It's convenient that there happens to be a straightforward solution to this problem. Yes, install_requires can be inlined now.

These are defined in PEP 508: https://www.python.org/dev/peps/pep-0508/#environment-markers

The current implementation means that installing 1.0a2 from the published wheel on Python 3.8 does not include `astor` (and thus does not work) because "the Python version that is building the wheel is different from the Python version that is installing it" (from https://hynek.me/articles/conditional-python-dependencies/).

I don't think Windows and Linux can share wheels, so `pyreadline` doesn't strictly have to be included here, but it seems more correct to use this method for both.
@tianon
Copy link
Member Author

tianon commented Jul 7, 2021

Oh, I suppose I should add that I've tested this successfully on both Python 3.8 and 3.9 😅

Copy link
Member

@Kodiologist Kodiologist 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 catching this so fast. I'll release 1.0a3 with just this fix once it's in.

@tianon
Copy link
Member Author

tianon commented Jul 7, 2021

I was worried it would be harder but was pleased to find such a simple fix that I could contribute it myself. 😄

@Kodiologist Kodiologist merged commit 8347a98 into hylang:master Jul 9, 2021
@Kodiologist
Copy link
Member

1.0a3 has been released.

@tianon tianon deleted the environment-markers branch July 9, 2021 17:54
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