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

make sure that wheel files with a single digit version do work #1445

Merged
merged 1 commit into from Jan 15, 2014

Conversation

Projects
None yet
5 participants
@schmir
Contributor

schmir commented Jan 9, 2014

this is a rather serious bug, since it makes 'pip wheel ...' fail with
the following error if the wheelhouse contains such a wheel:

,----
| ...
| File "/home/ralf/.ve/dev/local/lib/python2.7/site-packages/pip/wheel.py", line 420, in init
| self.version = wheel_info.group('ver').replace('_', '-')
| AttributeError: 'NoneType' object has no attribute 'replace'
`----

make sure that wheel files with a single digit version do work
this is a rather serious bug, since it makes 'pip wheel ...' fail with
the following error if the wheelhouse contains such a wheel:

,----
| ...
|   File "/home/ralf/.ve/dev/local/lib/python2.7/site-packages/pip/wheel.py", line 420, in __init__
|     self.version = wheel_info.group('ver').replace('_', '-')
| AttributeError: 'NoneType' object has no attribute 'replace'
`----
@schmir

This comment has been minimized.

Show comment
Hide comment
@schmir

schmir Jan 9, 2014

Contributor

the regex as is still allows "ver" to become none, like in

>>> import pip.wheel
>>> pip.wheel.Wheel("Cython-cp27-none-linux_x86_64.whl")
Traceback (most recent call last):
  File "", line 1, in 
  File "/home/ralf/.ve/dev/local/lib/python2.7/site-packages/pip/wheel.py", line 420, in __init__
    self.version = wheel_info.group('ver').replace('_', '-')
AttributeError: 'NoneType' object has no attribute 'replace'

I'm not sure if wheels without versions should be allowed or if that is also a mistake.

Contributor

schmir commented Jan 9, 2014

the regex as is still allows "ver" to become none, like in

>>> import pip.wheel
>>> pip.wheel.Wheel("Cython-cp27-none-linux_x86_64.whl")
Traceback (most recent call last):
  File "", line 1, in 
  File "/home/ralf/.ve/dev/local/lib/python2.7/site-packages/pip/wheel.py", line 420, in __init__
    self.version = wheel_info.group('ver').replace('_', '-')
AttributeError: 'NoneType' object has no attribute 'replace'

I'm not sure if wheels without versions should be allowed or if that is also a mistake.

@quietlyconfident

This comment has been minimized.

Show comment
Hide comment
@quietlyconfident

quietlyconfident commented Jan 13, 2014

+1

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Jan 13, 2014

Member

Not saying that we shouldn't be tolerant (and not saying that we should...) but such versions do not conform to PEP 440.

Member

pfmoore commented Jan 13, 2014

Not saying that we shouldn't be tolerant (and not saying that we should...) but such versions do not conform to PEP 440.

@schmir

This comment has been minimized.

Show comment
Hide comment
@schmir

schmir Jan 13, 2014

Contributor
  • pip wheel doesn't complain when it builds such wheels, it's only pip install that fails!
  • version '10' would be fine for the regex, but version '1' is not!
  • pep440 is less than one year old. please give people a chance to fix their versions!
Contributor

schmir commented Jan 13, 2014

  • pip wheel doesn't complain when it builds such wheels, it's only pip install that fails!
  • version '10' would be fine for the regex, but version '1' is not!
  • pep440 is less than one year old. please give people a chance to fix their versions!
@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Jan 13, 2014

Member

Understood on all these points. I'm not arguing that we shouldn't address the issue, just pointing out that all of the various new features in packaging (and wheel is definitely one of these) are inter-related and based round the new PEPs.

I'm only noting this here as a record for whoever reviews the change. I don't even know why PEP 440 disallows single-component versions.

Member

pfmoore commented Jan 13, 2014

Understood on all these points. I'm not arguing that we shouldn't address the issue, just pointing out that all of the various new features in packaging (and wheel is definitely one of these) are inter-related and based round the new PEPs.

I'm only noting this here as a record for whoever reviews the change. I don't even know why PEP 440 disallows single-component versions.

@schmir

This comment has been minimized.

Show comment
Hide comment
@schmir

schmir Jan 15, 2014

Contributor

pywin32 is a popular package that uses single digit versions.

Contributor

schmir commented Jan 15, 2014

pywin32 is a popular package that uses single digit versions.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Jan 15, 2014

Member

Note that wheel support for single digit versions is only a small part of the picture. If Metadata 2.0 doesn't support them, other things will fail as well. I'll flag this up on distutils-sig as it's a wider question than just for pip. Feel free to contribute to the discussion over there.

Member

pfmoore commented Jan 15, 2014

Note that wheel support for single digit versions is only a small part of the picture. If Metadata 2.0 doesn't support them, other things will fail as well. I'll flag this up on distutils-sig as it's a wider question than just for pip. Feel free to contribute to the discussion over there.

@quietlyconfident

This comment has been minimized.

Show comment
Hide comment
@quietlyconfident

quietlyconfident Jan 15, 2014

Agreed that there should be a wider discussion.

However, Postel’s Law suggests that pip should do its best to deal with nonconforming package metadata where the intent is clear, if that is possible.

In this case, even though some packages have non-conforming metadata, pip is usually able to install them, and separately to build wheels from them. So it seems like there should probably be enough information about the user’s intent here to do install those built wheels.

On Jan 15, 2014, at 11:12 AM, Paul Moore notifications@github.com wrote:

Note that wheel support for single digit versions is only a small part of the picture. If Metadata 2.0 doesn't support them, other things will fail as well. I'll flag this up on distutils-sig as it's a wider question than just for pip. Feel free to contribute to the discussion over there.


Reply to this email directly or view it on GitHub.

quietlyconfident commented Jan 15, 2014

Agreed that there should be a wider discussion.

However, Postel’s Law suggests that pip should do its best to deal with nonconforming package metadata where the intent is clear, if that is possible.

In this case, even though some packages have non-conforming metadata, pip is usually able to install them, and separately to build wheels from them. So it seems like there should probably be enough information about the user’s intent here to do install those built wheels.

On Jan 15, 2014, at 11:12 AM, Paul Moore notifications@github.com wrote:

Note that wheel support for single digit versions is only a small part of the picture. If Metadata 2.0 doesn't support them, other things will fail as well. I'll flag this up on distutils-sig as it's a wider question than just for pip. Feel free to contribute to the discussion over there.


Reply to this email directly or view it on GitHub.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Jan 15, 2014

Member

Let's see what distutils-sig says. Also, @dstufft, @dholth, @qwcode do any of you have an opinion?

I'm not comfortable with unilaterally accepting this, so I'll wait for some feedback.

Member

pfmoore commented Jan 15, 2014

Let's see what distutils-sig says. Also, @dstufft, @dholth, @qwcode do any of you have an opinion?

I'm not comfortable with unilaterally accepting this, so I'll wait for some feedback.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Jan 15, 2014

Contributor

to me, PEP440 looks like it accepts single digit versions? it even says "Another change to the version scheme is to allow single number versions"

N[.N]+[{a|b|c|rc}N][.postN][.devN]

I think the [N]+ is about allowing versions like 1.234, not meaning that .N is required. but if it's present, it can be 1 or more.

Contributor

qwcode commented Jan 15, 2014

to me, PEP440 looks like it accepts single digit versions? it even says "Another change to the version scheme is to allow single number versions"

N[.N]+[{a|b|c|rc}N][.postN][.devN]

I think the [N]+ is about allowing versions like 1.234, not meaning that .N is required. but if it's present, it can be 1 or more.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Jan 15, 2014

Contributor

I think the [N]+ is about allowing versions like 1.234

correction, I think it's about allowing 1.2.3.4

Contributor

qwcode commented Jan 15, 2014

I think the [N]+ is about allowing versions like 1.234

correction, I think it's about allowing 1.2.3.4

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Jan 15, 2014

Member

Ah. I read that as .N one or more times (that's what the + means). I'd have used * to mean 0 or more. But the text you quote makes the intent clear.

Given that this patch simply allows version 1 where version 10 is already allowed (no dots anywhere) and single-component versions are OK, this is clearly just a simple bug fix, so I'm fine with it. I'll leave it a short while longer in case anyone else shouts, then merge it.

@schmir thanks for the patch, and sorry for making such a big deal out of it!

Member

pfmoore commented Jan 15, 2014

Ah. I read that as .N one or more times (that's what the + means). I'd have used * to mean 0 or more. But the text you quote makes the intent clear.

Given that this patch simply allows version 1 where version 10 is already allowed (no dots anywhere) and single-component versions are OK, this is clearly just a simple bug fix, so I'm fine with it. I'll leave it a short while longer in case anyone else shouts, then merge it.

@schmir thanks for the patch, and sorry for making such a big deal out of it!

@schmir

This comment has been minimized.

Show comment
Hide comment
@schmir

schmir Jan 15, 2014

Contributor

#1445 (comment) still needs to be addressed then.

Contributor

schmir commented Jan 15, 2014

#1445 (comment) still needs to be addressed then.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Jan 15, 2014

Member

I don't see any reason not to merge this, PEP440 allows single versions (and generally uses [ ] to represent optional).

Member

dstufft commented Jan 15, 2014

I don't see any reason not to merge this, PEP440 allows single versions (and generally uses [ ] to represent optional).

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Jan 15, 2014

Member

What exactly needs addressed still?

Member

dstufft commented Jan 15, 2014

What exactly needs addressed still?

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Jan 15, 2014

Contributor

@schmir as to your comment, the version-less wheel filename you constructed "Cython-cp27-none-linux_x86_64.whl" would never happen, you just made that up?

so, I don't think there's a problem in that our class fails for that, if that's your point

Contributor

qwcode commented Jan 15, 2014

@schmir as to your comment, the version-less wheel filename you constructed "Cython-cp27-none-linux_x86_64.whl" would never happen, you just made that up?

so, I don't think there's a problem in that our class fails for that, if that's your point

@schmir

This comment has been minimized.

Show comment
Hide comment
@schmir

schmir Jan 15, 2014

Contributor

yes, I've made that up. but why shouldn't I be allowed to put such files into my wheelhouse directory?
pip certainly should not bail out with an AttributeError. It should raise InvalidWheelFilename.

Contributor

schmir commented Jan 15, 2014

yes, I've made that up. but why shouldn't I be allowed to put such files into my wheelhouse directory?
pip certainly should not bail out with an AttributeError. It should raise InvalidWheelFilename.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Jan 15, 2014

Contributor

proceeding with merge on this, so that it gets in 1.5.1

@schmir, getting it to raise InvalidWheelFilename is better. feel free to log another issue/PR for that.

Contributor

qwcode commented Jan 15, 2014

proceeding with merge on this, so that it gets in 1.5.1

@schmir, getting it to raise InvalidWheelFilename is better. feel free to log another issue/PR for that.

qwcode added a commit that referenced this pull request Jan 15, 2014

Merge pull request #1445 from schmir/fix-wheel-single-digit-version
make sure that wheel files with a single digit version do work

@qwcode qwcode merged commit ea97076 into pypa:1.5.X Jan 15, 2014

1 check passed

default The Travis CI build passed
Details
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Jan 15, 2014

Contributor

but why shouldn't I be allowed to put such files into my wheelhouse directory?

you could theoretically hack your filenames, but they're not valid wheels.
wheels are constructed from python projects, which have versions.

Contributor

qwcode commented Jan 15, 2014

but why shouldn't I be allowed to put such files into my wheelhouse directory?

you could theoretically hack your filenames, but they're not valid wheels.
wheels are constructed from python projects, which have versions.

@schmir schmir deleted the schmir:fix-wheel-single-digit-version branch Jan 15, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment