-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
Mako wheels have computed install_requires #249
Comments
Michael Bayer (@zzzeek) wrote:
→ 65b1a37 |
Changes by Michael Bayer (@zzzeek):
|
Michael Bayer (@zzzeek) wrote: I have no need for wheels on pypi. Does that fix work for you? I can release tomorrow. |
RobertR (@rbtcollins) wrote: Removing universal won't fix the issue because wheel defaults to building a wheel that can be used on all minor versions of the same python. E.g. removing universal fixes py 2.7 after 3.2, but not py 3.3 after 3.2. Using a strong tag like --python-tag cp26 when building the wheel would isolate everything- but I don't know how to do that automatically from within setup.py yet. |
Michael Merickel (@mmerickel) wrote: I think the correct solution is for you to duplicate the install requires into the
|
RobertR (@rbtcollins) wrote: That would depend on distutils2; the patch I put forward does the same thing without depending on distutils2. |
Michael Bayer (@zzzeek) wrote:
This makes no sense. Removing the |
Donald Stufft (@dstufft) wrote: Wheel correctly handles compiled software because it can detect that it's compiled and thus it makes a platform specific wheel, so psycopg2 works fine (though I recognize that psycopg2 was just an example). The real problem comes not from compiled projects, but pure Python ones that use conditional logic in their Previously to pip 7.0 if a project's So, the question then becomes how do we handle this?
For Mako, I would suggest the first option which can be implemented either by duplicating your There will be discussion over on pip about the third thing, but I think that we should probably force wheels to be more specific than they actually might be since we do not know whether a project is emitting correct wheels or not (and given how many legacy projects there are, there may be more than a few that do). This would solve Mako's problem in the specific case of the implicit pip wheel cache, but not for any other cases where people might be generating a Wheel. I fully understand that the changes in the ecosystem have made some people's lives harder and I wish there was a better way forward than to try and progress us towards a better eventual system with some short term pain. There has been a lot of technical debt added to the entire ecosystem to deal with the shortcoming of the system, and sooner or later all technical debt has to be paid down, and that's what we're doing now. The alternative is to never progress and to simply deal with a subpar system forever. I hope that as things progress, the pain will end up being worth it as we start to crawl out of the technical debt hole and things start working more consistently and sanely for all involved. I do however recognize this can be painful, and I'm sorry for that. |
Michael Bayer (@zzzeek) wrote: OK, thanks for that explaination. As far as Mako's conditionals, there are only two. One is the thing where Armin grumpily didn't want to support MarkupSafe in Python 3.2 and lower. At this point I am fine putting out a new Mako that just doesn't support Python 3.2, we' almost at 3.5 now and nobody uses 3.2. As far as the argparse thing, I have that same conditional in Alembic also. This is because I like to use argparse, which comes included in Python 3 but not in Python 2. In this case, if I just put "argparse" in the requirements, last time I looked into this is downloads and installs argparse even on Py3k where it's already there. But this seems like something that can be conditional based on Py2k/Py3k which from what I'm reading suggests that at that level, there's no issue because non-universal wheels will build separately for py2k/py3k. So can we propose the most cross-compatible patch possible, given that we can drop 3.0->3.2 and start at Python 3.3, and whatever it is I'm supposed to do in order to have argparse available without the double-install on Py3k, we can do that. But obviously Mako is a "long tail" library that's installed in all kinds of crufty environments so I would hope that with an older pip or setuptools it still can install fine. I'd also imagine that even though psycopg2 has the magic "we're compiled!" flag that changes everything, there's still a ton of projects with conditionals in their setup.py. That putting a conditional in a setup.py is now illegal is a big deal, it's fine for Mako to be an early test case for this but I think the repercussions of this need to be carefully worked out. I know about the pip wheel thing and I think it is great as I watch build environments do a better and better job of not double-downloading/installing things these days, but I sort of thought that without "universal", it would build wheels at least for specific Python versions (e.g. 2.6, 2.7, etc). |
Changes by Michael Bayer (@zzzeek):
|
Michael Merickel (@mmerickel) wrote: argparse is in the stdlib from 2.7+ afaik so if you don't want a double install there you still need to do the |
RobertR (@rbtcollins) wrote: [aside] I did not realise that wheel consulted static metadata; that makes the plan for static dependencies more awkward; ca la vie. Anyhow - yes, the churn is hard, but this specific problem has existed since the wheel project was created, late 2012: its not a new thing now. I'm reporting it now because we just noticed. The most compatible version should be the metadata based one - it duplicates your requirements, so will only affect folk using wheels, and that implies opting in or recent versions of tools, either of which should be fine. |
Michael Bayer (@zzzeek) wrote: alright, so all agreed I can do exactly what is in https://bitbucket.org/zzzeek/mako/issues/249/mako-wheels-have-computed-install_requires#comment-20878559, leave setup.py as is, and this will work both for old-school installs on older pips as well as newer pips caching things in wheels, agreed? |
Michael Bayer (@zzzeek) wrote: I'd have to do the same thing in Alembic and probably scan through my other projects for similar things. |
RobertR (@rbtcollins) wrote: Yes, you'll see this: from bdist_wheel. You probably want to put a comment in both setup.cfg and setup.py explaining that they should be kept in sync. |
Donald Stufft (@dstufft) wrote: @zzzeek Yes. The important thing to remember is that section will completely replace the If you have any questions about how to make your projects wheel safe (or really, any packaging question), feel free to @ mention me or ping me on twitter or whatever. |
Michael Bayer (@zzzeek) wrote: Does |
Donald Stufft (@dstufft) wrote:
|
Michael Bayer (@zzzeek) wrote: ouch! is there any plan for improving that? otherwise packages will forever have to maintain both right? |
Donald Stufft (@dstufft) wrote: You don't have to maintain both, setuptools and pip both support specifying the conditionals directly inside of setup.py (this is what the original patch @rbtcollins wrote does). The caveat is that support for that feature requires a modernish pip and setuptools so individual projects have to decide whether they would rather support older setuptools/pip and add duplication or use the newer feature and remove duplication but make older pip/setuptools unsupported. |
Donald Stufft (@dstufft) wrote: By the way, by modernish I mean support was added for sdist based markers in setuptools in 0.7 and I think they were broken in pip until pip 6.0 (but I may be wrong, it may work in older ones). |
Michael Bayer (@zzzeek) wrote: can I do @rbtcollins version and also do a version check on setuptools itself, determining if i do the new version of "Extras_require" or the older thing ? I would assume pip is up to date if setuptools is. |
Donald Stufft (@dstufft) wrote: Um, that would work yes I think. |
Michael Bayer (@zzzeek) wrote: alrighty, taking some notes here. Running an old Python 2.6.6 on a VM, making a virtualenv and doing "./bin/pip install Mako-1.0.2.dev0.tar.gz", which is an sdist of the setup.py that only includes the newer extras_require format, and a blank install_requires in all cases. We want to see that both argparse and MarkupSafe are recognized as dependencies. virtualenv 1.11.3 which has setuptools 2.2 pip 1.5.3 does not honor the directives in extras_require, it seems to just ignore them. same for venv 1.11.4 (pip 1.5.4) same for venv 1.11.5 (pip 1.5.5, setuptools 3.4.4) same for venv 1.11.6 (pip 1.5.6, setuptools 3.6) - so @rbtcollins that contradicts what you were seeing... then in venv 12.0, which has setuptools 7.0 and pip 6.0, now it recognizes the requirements in extras_require. So it looks like setuptools 7.0 is where this feature was added. now of course setuptools has a full changelog at https://pypi.python.org/pypi/setuptools/10.0, however I have read every single word between versions 3.6 and 7.0 and there is not a single thing that even slightly resembles that this new feature is present. This is kind of a problem, on top of all the other ones. E.g. Mako is of course going to get this right because three packaging people are helping me directly. Not sure what the answer is for everyone else... |
Michael Bayer (@zzzeek) wrote: OK, now running it doing a "python setup.py install" to use only setuptools and now I'm seeing something different, setuptools.3.6 is honoring those directives. Will keep trying. |
Michael Bayer (@zzzeek) wrote: OK, so, "python setup.py install", even really ancient versions of setuptools seem to recognize this thing. I'm pretty confused. |
Donald Stufft (@dstufft) wrote: On my phone, but does setup.py install end up installing them (e.g. Cut pip out of the picture) for pre 7.0 setup tools? |
Michael Bayer (@zzzeek) wrote: So, just this patch:
so far, very old setuptools seem to be fine w/ it in the "setup.py install" case, and w/ pip you kind of need the 6.0 level of things but it's not clear if that's the case yet. |
Donald Stufft (@dstufft) wrote: I think support was added in 0.7 setuptools (from the change log ) but pip ignored it until 6.0. |
Michael Bayer (@zzzeek) wrote: also, I can find no mention whatsoever of the ":python_version" syntax in conjunction with extras throughout all of https://pythonhosted.org/setuptools/setuptools.html#developer-s-guide. We've basically got this: https://pythonhosted.org/setuptools/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies shouldn't this whole world of essential features and practices at least be....documented...before posting bugs on downstream projects? I'm starting to think im crazy here. When I make the slightest change in SQLAlchemy's API, you get an enormous paragraph with examples explaining the whole thing. I wouldn't think of expecting my users to know things that aren't even documented ? |
Michael Bayer (@zzzeek) wrote: yeah, it seems like really old setuptools recognize the dependencies, but im actually not sure if that's because it just is taking everything in extras_require or it actually knows what ":python_version" means. |
Donald Stufft (@dstufft) wrote: There are some PEPs that include them, but ultimately you're right. We've historically been pretty bad about documentation and though I think we're better than before we have a long way to go. |
Michael Bayer (@zzzeek) wrote: so this is a tricky-ish case on my end, I can put out the all-new-syntax only, and what actually happens is that Mako gracefully degrades if MarkupSafe isn't present anyway. So if this syntax were failing for users on old pips/setuptools, I'd probably never hear about it. argparse is already going to be there 99.99% of the time anyway. They'd just be using a less "secure" and slower string escaping tool. |
Michael Bayer (@zzzeek) wrote: why is there no "bdist_wheel" command when I run |
Donald Stufft (@dstufft) wrote: Have you installed wheel? |
Michael Bayer (@zzzeek) wrote: oh, have to install "wheel". How strange you can install pip and not have "wheel" installed. |
Michael Bayer (@zzzeek) wrote: how big a deal is it that if one says "pypy setup.py bdist_wheel" on SQLAlchemy, which has C extensions that are optional, you get a "universal" py2k wheel w/o the C extensions? is there no chance that wheels can somehow include the "im a 2.7-only wheel" without actual C extensions being present ? |
Michael Bayer (@zzzeek) wrote: every problem ive seem to have had with wheels comes down to this urge it has to build "universal" packages. If there were only a way to say, "no-universal=1", and you always get a python version as well as platform on your .wheel (e.g. pypy specific, 2.7 specific, 3.2 specific, etc), it would be great. Wheels work terrifically for caching things you've already downloaded and built. I don't see at all why its so important that these wheels are cross-python-version however, unless you're distributing a pure-Python package as a wheel on pypi, which still seems to be fairly pointless as if you just put up a .tar.gz your pip will have your wheel locally anyway. If I run a different Python version or interpreter, I'd expect that all the wheels would be entirely different. |
Donald Stufft (@dstufft) wrote: Ah, optional C extensions which don't get installed on PyPy. So the basic answer is, if you do nothing and someone installs SQLAlchemy with CPython first they'll get a wheel cached that has the compiled bits and are only good for just that Python, if they install it with PyPy last it will create a "universal" py2 wheel, however pip will prefer a wheel with C extensions over a wheel without them, so in that scenario things will just work. HOWEVER, if the operation is inversed and PyPy is used to install before anything else, then pip will see that there is already an acceptable wheel available and will use that instead of building the C extensions on CPython. There is a small hack you can use to make this not the case though, CFFI does it so that it creates a "compiled" wheel on PyPy that doesn't actually contain any compiled code. You can just do this: zzzeek/sqlalchemy#194 |
Donald Stufft (@dstufft) wrote: Probably wheel should get a keyword argument that does what that PR does instead of relying on something like in that PR, but that should do what you want. |
Michael Bayer (@zzzeek) wrote: That's great (yeah a flag would be nicer), that helps a lot over there. thanks! |
Michael Bayer (@zzzeek) wrote:
→ 28caab3 |
hello from the future. a PR to drop Python 2.6 and older Python 3's is at https://gerrit.sqlalchemy.org/#/c/sqlalchemy/mako/+/957 this would allow us to remove all the computed stuff in setup.py and do a wheel. |
Mike Bayer has proposed a fix for this issue in the master branch: Bump Python versions, remove conditional imports https://gerrit.sqlalchemy.org/1380 |
Migrated issue, originally created by RobertR (@rbtcollins)
Terrible title, sorry.
So Mako computes a value for install_requires in setup.py. This results in the wheel thats built being python version specific - concretely if you build a wheel using Python 3.2 it won't depend on MarkupSafe, but on 3.2 or 2.6/2.7 it will will.
This shows up when wheels are reused across Python versions (one way to do this is via the pip wheel cache): pypa/pip#3025 - if Python 3.2 is used to install mako, then when an install is done on 3.3 MarkupSafe isn't installed. Conversely if Python 3.3 is used to do the first install, then on 3.2 MarkupSafe will be installed even though its (presumably) incompatible.
A similar issue applies to the argparse conditional: if the wheel is built on 2.6, it will drag in argparse when installed on 2.7/3.x.
Your setup.cfg has universal=1 so it looks like you intend to build wheels suitable for multiple Python versions, but to do so you cannot use computed requirements - you must instead use conditional requirements.
Attached is a patch to do this.
Unfortunately, sufficiently old versions of pip and setuptools don't support computed requirements. I don't know the minimum versions that do, but pip 1.5.6 (the default on my Trusty install) does - the following is from a wheel built with the patch attached.:
Attachments: mako-setup.patch
The text was updated successfully, but these errors were encountered: