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

Force cython when building sdist (see 196 instead) #188

Closed
wants to merge 3 commits into from

Conversation

perlpunk
Copy link
Member

Fixes #182

thanks @asottile

Alternatively there's #33 which will only issue a warning

Copy link
Contributor

@asottile asottile left a comment

Choose a reason for hiding this comment

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

lg2m!

@perlpunk perlpunk requested a review from ingydotnet June 28, 2018 23:06
Copy link
Member

@ingydotnet ingydotnet left a comment

Choose a reason for hiding this comment

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

Looks good. Would have helped me not create the bad 4.1 dist.

@nitzmahone
Copy link
Member

The warning probably shouldn't be in setup.py for default usage though- otherwise anyone who installs from sdist will get it. Better to have a test to make sure the dist looks like we want.

@asottile
Copy link
Contributor

@nitzmahone this error will only happen for packagers and not for normal users during install. setup.py sdist isn't part of the install pathway (setup.py install / setup.py bdist_wheel)

If you want to convince yourself, check out this branch, build the sdist with python setup.py sdist and then install it in a fresh virtualenv via pip install path/to/pyyaml.tar.gz

@perlpunk
Copy link
Member Author

perlpunk commented Jun 29, 2018

I think @nitzmahone meant the warning, not the error.
I included it because the comment in #33 suggested that it would be a good idea.
We can remove it if it doesn't make sense.

@asottile
Copy link
Contributor

ah! I should learn to read more carefully :)

yeah the warning probably isn't necessary given we'll have the .c file now 🤷‍♂️

@perlpunk
Copy link
Member Author

I created #196

@perlpunk perlpunk closed this Jun 30, 2018
@perlpunk perlpunk changed the title Force cython when building sdist Force cython when building sdist (see 196 instead) Jun 30, 2018
@perlpunk perlpunk deleted the force-cython branch July 1, 2018 12:24
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.

None yet

4 participants