-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Script generation code for wheels #1251
Conversation
to be clear, this is "DO NOT MERGE" just due to wanting review, or is their another dependency that needs to be handled? or maybe that you didn't add any tests? : ) |
He's carrying a patch against distlib inside it. |
the distlib issue for the patch: https://bitbucket.org/pypa/distlib/issue/31/scripts-api-does-not-work-when-vendored |
Also, I'll collapse out the "fixing yet another typo" commits before submitting a final PR. |
I think this is now functionally complete. I need to do some manual testing (in addition to the test suite additions I've included) and some refactoring for maintainability/simplification. Then, once Vinay releases a fixed version of distlib, this should be good to go. One point worth making is that with this patch, "pip install XX" does something subtly different to "pip wheel XX; pip install " in that the former will install setuptools style wrappers and the latter will install distlib style wrappers. Personally, I'm happy with this but it may cause comments. |
Do you mean pip install when it installs from a source distribution, or pip install will always install setuptools style wrappers and only wheels made by pip wheel won't? |
Looking at the code it looks like the answer is wheels will always generate, sdist will always setuptools. I think that's OK. We might want to switch to always generating even for sdists for consistency sake, but I think that we don't need to hold up this PR for that. |
Yep, @dstufft got it right. I don't even know how we would alter the behaviour on sdists, as that is done by setuptools and not by pip. I think we have to live with the difference until sdist 2.0 and always installing via wheels becomes the standard. |
We should special case the easy_install wheel as well. Namely easy_install and easy_install-X.Y needs to be created. |
Hmm. OK, I guess. But only because setuptools is a prerequisite of pip, and only as a short-term fix. This needs fixing properly in metadata so that projects can specify versioned executables at source. I know you know this, I just want it on record. Projects other than pip and setuptools will just have to put up with needing separate wheels for each Python version until this is solved properly (even though that makes wheels less attractive for them in the short term). |
Yup agreed. The only reason I think setuptools should be special cased is because it makes it possible to install pip + dependencies entirely from Wheel. Which makes PEP453 easier/better. Somewhat selfish but I think it'll be better that way. |
Just to be clear here: we want the following commands to be available (for Python 3.4, for example): pip, pip3, pip-3.4 Note that the X.Y version has a dash, and the X version does not.That is distlib behaviour that I can't control (we'd need to get distlib changed if we wanted something different). And note that there is no easy_install3, because that's what the setuptools setup.py does. BTW, setuptools' setup.py has the following:
That's not going to be respected when we install from a wheel - I assume that's OK. |
It would be really nice to make that pip, pip3, pip3.4, easy_install, and easy_install-3.4. Instead of variants could we just tell distlib that these are unrelated commands and not use the variants portion of distlib? Alternatively we could seea bout getting Vinay to make it customizable how the interpolation happens. |
Also yes, that's fine. |
I guess I could do that. By the way, as things stand, I'm assuming that pip and setuptools will be updated to only include "pip" and "easy_install" entry points, and not any versioned ones (that could be the wrong versions). If that's not the case, I'd need to deliberately ignore a broader class of entry points (pip, pipN, pipN.M, easy_install, easy_install-N.M) and that gets both messy and fragile. But on the other hand, not including versioned entry points makes installation from sdist omit them. There seem to be a number of options here:
I'm against (3) to be honest, not just because it makes life harder for me but also because it potentially creates transition issues when we do move this stuff to metadata. And my brain isn't up to working out what those consequences would be at the moment... |
Just pushed a version that special-cases easy_install as well, and ignores any entry points matching 'pip(\d(.\d)?)?$' or 'easy_install(-\d.\d)?$'. Well, I would have, but github seems to be playing up. Will try to do so tomorrow. |
OK. IMO, this is ready to go. Things I need to do/happen:
I'll leave it now till we get a new distlib release. If anyone can think of anything I've forgotten, let me know. |
$ git checkout <your branch>
$ git rebase -i develop When your text editor opens you can rearrange the lines, delete lines to delete commits, or change the "pick" line to one of:
So like
Will merge abdf and ggasd into a single commit, and reopen the editor allowing you to edit the combined commit message. You can squash as many commits as you like into one commit as well. |
I'll do a release of |
git push -f origin script-generation Be careful with git push -f, it's a destructive operation, it stands for force and it says "I don't care if this isn't a child of what you already have for the named reference, this is what I want it to be". |
OK, nice. I'm not too worried if I get the github branch wrong, as I was originally planning on deleting & recreating anyway :-) Thanks for the git help. I feel like it's possible to do pretty much anything with git, it's just almost impossible to find out how :-) |
No problem! I've learned the dark magic internals of git, so my brain is sufficiently broken that I understand it! Feel free to ask me anytime. Mostly the thing you want to make sure is you don't actually force push your branch to master or something. |
@dstufft That worked really well, thanks. |
Just wondering what the status of this is? |
Was ready to go pending @vsajip releasing a new version of distlib. Looks like something has since been commited that conflicts, so I'll have to rebase :-( I'm not going to do anything though till distlib 0.1.4 lands. |
OK, distlib 0.1.4 is vendored. Thanks @vsajip! I will work on rebasing these changes and getting them merged tomorrow. |
OK, this is ready to go. I'll let it sit for the rest of the day in case anyone has last minute issues they want to raise, then commit this evening (GMT). |
Looking at this now.. |
@@ -118,6 +133,7 @@ def move_wheel_files(name, req, wheeldir, user=False, home=None, root=None): | |||
source = wheeldir.rstrip(os.path.sep) + os.path.sep | |||
installed = {} | |||
changed = set() | |||
generated = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, a little picky, but maybe a "generated_scripts", or a comment that these are the console scripts we're going to generate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, these could be any generated files, not just scripts. So I'd rather add a comment than change the name.
Thanks @qwcode for the thorough review. I will update the patch accordingly. |
@pfmoore , other than the wording issues I brought up, LGTM. |
Script generation code for wheels
Versioned commands are available since pypa/pip#1053, or pypa/pip#1251 if pip is installed from a wheel. Fixes #353.
No description provided.