AcademySoftwareFoundation / rez

An integrated package configuration, build and deployment system for software
https://rez.readthedocs.io
Apache License 2.0
949 stars 338 forks source link

rez-pip does not account for python versions having differing requires #895

Open jopollack opened 4 years ago

jopollack commented 4 years ago

As an example, take cmd2-0.8.9, since this happened to be the one I installed when I discovered this. If you read the Metadata, you'll see this:

Requires-Python: >=2.7
Requires-Dist: pyparsing (>=2.0.1)
Requires-Dist: pyperclip
Requires-Dist: six
Requires-Dist: subprocess32; python_version<'3.0'
Requires-Dist: enum34; python_version<'3.4'
Requires-Dist: contextlib2; python_version<'3.5'

I was installing versions for python-2.7 and python-3.7, and my package.py came out looking like this:

requires = [
    'contextlib2',
    'enum34',
    'subprocess32',
    'six',
    'pyperclip',
    'wcwidth',
    'pyparsing-2.0.1+'
]

variants = [
    ['python-2.7'],
    ['python-3.7']
]

My python-3.7 cmd2 rez-env now fails to resolve, because I don't have contextlib2, enum34 or subprocess32 packages for python-3.7.

nerdvegas commented 4 years ago

Thanks, iirc this should be accounted for and this is probably a bug. A

On Thu, Jun 4, 2020 at 8:57 AM jopollack notifications@github.com wrote:

As an example, take cmd2-0.8.9, since this happened to be the one I installed when I discovered this. If you read the Metadata, you'll see this:

Requires-Python: >=2.7 Requires-Dist: pyparsing (>=2.0.1) Requires-Dist: pyperclip Requires-Dist: six Requires-Dist: subprocess32; python_version<'3.0' Requires-Dist: enum34; python_version<'3.4' Requires-Dist: contextlib2; python_version<'3.5'

I was installing versions for python-2.7 and python-3.7, and my package.py came out looking like this:

requires = [ 'contextlib2', 'enum34', 'subprocess32', 'six', 'pyperclip', 'wcwidth', 'pyparsing-2.0.1+' ]

variants = [ ['python-2.7'], ['python-3.7'] ]

My python-3.7 cmd2 rez-env now fails to resolve, because I don't have contextlib2, enum34 or subprocess32 packages for python-3.7.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nerdvegas/rez/issues/895, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOUSUOQYUL7OXOHCLDAA3RU3IMLANCNFSM4NSC6GDA .

jopollack commented 4 years ago

I'll correct this a bit: It does seem to work if the python package doesn't exist at all. I just ran into this with commonmark, which requires futures-0.14.0+ for python-2 and correctly puts that in the variant, since futures doesn't exist for python-3.

I repeated the rez-pip install for cmd2-0.8.9 using just python-3 in an isolated workflow, and it just installed python-3 versions of the unnecessary packages. So it does resolve, but its pulling in unnecessary packages.

herronelou commented 2 years ago

I've run into this problem quite a few times this week, I have noticed many of the re-pip installed packages are ignoring the python requirements from the pip package, often making a single 2.7 variant if I install from python 2.7, or a 3.7 variant if I install from 3.7, even when the metadata says it requires python-2.7+<3|3.5+ If I run the pip-rez install twice from 2 different python versions, I get 2 variants which are exact copies of each other, using twice the amount of space, and still it would fail if I tried to resolve in a 3.8 or 3.9 context, which should normally work.

If nobody is working on this, I would be willing to look at putting a merge request.

nerdvegas commented 2 years ago

The problem is that we're suffering a kind of confirmation bias here. You've pointed out that there are pip packages where the variants are identical, and so (in theory) the major.minor variants are unnecessary. However, there are lots of packages and edge cases where this is not true, and the problem lies in our ability to detect these cases. Building the major.minor variant is basically the conservative approach - we're erring on the side of caution.

I'll leave it up to others with more python packaging experience than me to delve into the details (hi @maxnbk @JeanChristopheMorinPerso !). Fixing this involves a bigger discussion, I don't think it's a simple thing to solve.

herronelou commented 2 years ago

The above is of course assuming pip and/or setuptools provides that information needed, which I assumed it did due to the metadata. For now, I have written a utility that lets me merge pip variants as a new package so that I can manually clean things up a bit before releasing them, it's a bit manual but I was having issues modifying the variants by hand due to the hashing of pip packages.

nerdvegas commented 2 years ago

Please note: We have two different things going on in this ticket. I want to clarify the original issue here.

According to the initial report it sounds as though python-specific-deps are being added to requires when in fact they should be added into the relevant variant instead.

We're now also talking about how to handle python variants in general, which does overlap with the original issue (and in fact, the original issue is one example of why we're build major.minor variants in the first place!), but isn't quite the same thing.

herronelou commented 2 years ago

Oops, that is correct. Maybe it could be summarised as rez-pip dependencies/variants are not lining up properly with pip's own version of dependencies/variants. Whether that is the per-python dependencies, or the python version itself. I have encountered both issues last week doing a rez install from scratch.