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

wheel file names conflate "_" and "-" in versions #1150

Closed
davedash opened this Issue Aug 20, 2013 · 35 comments

Comments

Projects
None yet
4 participants
@davedash

davedash commented Aug 20, 2013

Hi, not sure if this is a pip issue or a wheel issue.

I created a bunch of wheels by doing:

pip wheel -r requirements.txt

One of the wheels created was translate_toolkit-1.9.0_pinterest3-py27-none-any.whl. The package's setup.py however defined the version as 1.9.0-pinterest3 as did the requriement string in requirements.txt.

So naturally when I tried to install it via pip install --use-wheel I ran into this:

  Ignoring link http://pypi.pinadmin.com/custom/translate_toolkit-1.9.0_pinterest3-py27-none-any.whl (from http://pypi.pinadmin.com/custom/), version 1.9.0_pinterest3 doesn't match ==1.9.0-pinterest3
  Downloading translate-toolkit-1.9.0-pinterest3.tar.gz (1.0MB):

I feel like the error is with pip wheel, it seems to have converted -s to _ in lots of places. If this is something that's not supported, I'm happy to change our versioning.

I also looked over pep 440 but as far as I coudl tell the hyphen is a suitable modifier to tack onto the end of a version number.

/cc @carljm

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 20, 2013

Member

I think the replacing of - with _ is part of Wheel.

PEP440 does not allow a dash (or anything that doesn't roughly match N[.N]+[{a|b|c|rc}N][.postN][.devN]) however there was recently raised the question of how to handle forks (which I'm guessing this is).

But PEP440 isn't exactly relevant here because pip should support non PEP440 versions (other than pre-release vs not pre-release distinctions). This is I believe a mixture of Wheel escaping the - to a _, and pip not doing the inverse inside of a version.

The real question is which behavior is correct. PEP440 doesn't have a real answer to the fork versioning problem. Should pip normalize version strings to handle wheel, or should Wheel not be translating? But I do believe that's the fundamental problem.

Member

dstufft commented Aug 20, 2013

I think the replacing of - with _ is part of Wheel.

PEP440 does not allow a dash (or anything that doesn't roughly match N[.N]+[{a|b|c|rc}N][.postN][.devN]) however there was recently raised the question of how to handle forks (which I'm guessing this is).

But PEP440 isn't exactly relevant here because pip should support non PEP440 versions (other than pre-release vs not pre-release distinctions). This is I believe a mixture of Wheel escaping the - to a _, and pip not doing the inverse inside of a version.

The real question is which behavior is correct. PEP440 doesn't have a real answer to the fork versioning problem. Should pip normalize version strings to handle wheel, or should Wheel not be translating? But I do believe that's the fundamental problem.

@davedash

This comment has been minimized.

Show comment
Hide comment
@davedash

davedash Aug 20, 2013

@dstufft yup, this is just a fork - or more likely we just fixed some weird packaging thing for that package and it didn't have a good way for us to contribute that change back upstream (this happens a lot :( ).

I think it's fair for pip to expect the versions to match without having to do any translations. Here's a case:

~VIRTUAL_ENV » pip wheel qds-sdk==1.0.3-beta                                                                                                                                      davedash@immacomputer-2
Downloading/unpacking qds-sdk==1.0.3-beta
  Downloading qds_sdk-1.0.3-beta.tar.gz
  Storing download in cache at /tmp/.pip-dave/cache/https%3A%2F%2Fpypi.python.org%2Fpackages%2Fsource%2Fq%2Fqds_sdk%2Fqds_sdk-1.0.3-beta.tar.gz
  Running setup.py egg_info for package qds-sdk

Downloading/unpacking python-cjson (from qds-sdk==1.0.3-beta)
  Downloading python-cjson-1.0.5.tar.gz
  Storing download in cache at /tmp/.pip-dave/cache/https%3A%2F%2Fpypi.python.org%2Fpackages%2Fsource%2Fp%2Fpython-cjson%2Fpython-cjson-1.0.5.tar.gz
  Running setup.py egg_info for package python-cjson

Downloading/unpacking requests>=1.2.3 (from qds-sdk==1.0.3-beta)
  Downloading requests-1.2.3-py27-none-any.whl (377kB): 377kB downloaded
  Storing download in cache at /tmp/.pip-dave/cache/http%3A%2F%2Fpypi.pinadmin.com%2Fcustom%2Fwheels%2Frequests-1.2.3-py27-none-any.whl
Building wheels for collected packages: qds-sdk, python-cjson, requests
  Running setup.py bdist_wheel for qds-sdk
  Destination directory: /Users/davedash/.virtualenvs/f96fe15134affa18/wheelhouse
  Running setup.py bdist_wheel for python-cjson
  Destination directory: /Users/davedash/.virtualenvs/f96fe15134affa18/wheelhouse
  Skipping building wheel: http://pypi.pinadmin.com/custom/wheels/requests-1.2.3-py27-none-any.whl
Successfully built qds-sdk python-cjson
Cleaning up...
(f96fe15134affa18)------------------------------------------------------------
~VIRTUAL_ENV » tree wheelhouse                                                                                                                                                    davedash@immacomputer-2
wheelhouse
├── python_cjson-1.0.5-cp27-none-macosx_10_8_intel.whl
└── qds_sdk-1.0.3_beta-py27-none-any.whl

0 directories, 2 files

I'll try to file an issue with wheel.

davedash commented Aug 20, 2013

@dstufft yup, this is just a fork - or more likely we just fixed some weird packaging thing for that package and it didn't have a good way for us to contribute that change back upstream (this happens a lot :( ).

I think it's fair for pip to expect the versions to match without having to do any translations. Here's a case:

~VIRTUAL_ENV » pip wheel qds-sdk==1.0.3-beta                                                                                                                                      davedash@immacomputer-2
Downloading/unpacking qds-sdk==1.0.3-beta
  Downloading qds_sdk-1.0.3-beta.tar.gz
  Storing download in cache at /tmp/.pip-dave/cache/https%3A%2F%2Fpypi.python.org%2Fpackages%2Fsource%2Fq%2Fqds_sdk%2Fqds_sdk-1.0.3-beta.tar.gz
  Running setup.py egg_info for package qds-sdk

Downloading/unpacking python-cjson (from qds-sdk==1.0.3-beta)
  Downloading python-cjson-1.0.5.tar.gz
  Storing download in cache at /tmp/.pip-dave/cache/https%3A%2F%2Fpypi.python.org%2Fpackages%2Fsource%2Fp%2Fpython-cjson%2Fpython-cjson-1.0.5.tar.gz
  Running setup.py egg_info for package python-cjson

Downloading/unpacking requests>=1.2.3 (from qds-sdk==1.0.3-beta)
  Downloading requests-1.2.3-py27-none-any.whl (377kB): 377kB downloaded
  Storing download in cache at /tmp/.pip-dave/cache/http%3A%2F%2Fpypi.pinadmin.com%2Fcustom%2Fwheels%2Frequests-1.2.3-py27-none-any.whl
Building wheels for collected packages: qds-sdk, python-cjson, requests
  Running setup.py bdist_wheel for qds-sdk
  Destination directory: /Users/davedash/.virtualenvs/f96fe15134affa18/wheelhouse
  Running setup.py bdist_wheel for python-cjson
  Destination directory: /Users/davedash/.virtualenvs/f96fe15134affa18/wheelhouse
  Skipping building wheel: http://pypi.pinadmin.com/custom/wheels/requests-1.2.3-py27-none-any.whl
Successfully built qds-sdk python-cjson
Cleaning up...
(f96fe15134affa18)------------------------------------------------------------
~VIRTUAL_ENV » tree wheelhouse                                                                                                                                                    davedash@immacomputer-2
wheelhouse
├── python_cjson-1.0.5-cp27-none-macosx_10_8_intel.whl
└── qds_sdk-1.0.3_beta-py27-none-any.whl

0 directories, 2 files

I'll try to file an issue with wheel.

@davedash

This comment has been minimized.

Show comment
Hide comment
@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 20, 2013

Member

The one problem with not having to do any translations is prior to PEP440 a valid version was literally any string. So for sane file names some translation needs to be done. (The question becomes where do we draw the line).

I think the question of forks (even just packaging related ones) is an important one though and one that PEP440 needs some method of addressing. So I'm going to take that back to the PEP authors and try to make something happen (which doesn't actually solve your issue).

Probably if you used a period or a _ instead of a - it would work FWIW, that's not a long term solution but could be a short term one.

Member

dstufft commented Aug 20, 2013

The one problem with not having to do any translations is prior to PEP440 a valid version was literally any string. So for sane file names some translation needs to be done. (The question becomes where do we draw the line).

I think the question of forks (even just packaging related ones) is an important one though and one that PEP440 needs some method of addressing. So I'm going to take that back to the PEP authors and try to make something happen (which doesn't actually solve your issue).

Probably if you used a period or a _ instead of a - it would work FWIW, that's not a long term solution but could be a short term one.

@davedash

This comment has been minimized.

Show comment
Hide comment
@davedash

davedash Aug 20, 2013

@dstufft yeah, I'm happy enough to change our versioning scheme - 440 could be a little more clear on it's invalidity though (or at least supply a complete regexp).

Unfortunately some things: https://pypi.python.org/pypi/qds_sdk/1.0.5-beta aren't fully in our control...

If I'm missing the more succinct "This is what conforms to PEP440" let me know where to look for it, it'd be nice to cite to third party developers when I file issues/pull requests to fix things.

davedash commented Aug 20, 2013

@dstufft yeah, I'm happy enough to change our versioning scheme - 440 could be a little more clear on it's invalidity though (or at least supply a complete regexp).

Unfortunately some things: https://pypi.python.org/pypi/qds_sdk/1.0.5-beta aren't fully in our control...

If I'm missing the more succinct "This is what conforms to PEP440" let me know where to look for it, it'd be nice to cite to third party developers when I file issues/pull requests to fix things.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 20, 2013

Member

Yea PEP440 could be a bit better here, but it's basically N[.N]+[{a|b|c|rc}N][.postN][.devN] (taken from the PEP).

Where N is an ascii number, + means and more of, [ ] makes something optional, ( ) are grouping operators and | is an OR. Everything else is a literal.

It's sort of a pseudo regex.

Member

dstufft commented Aug 20, 2013

Yea PEP440 could be a bit better here, but it's basically N[.N]+[{a|b|c|rc}N][.postN][.devN] (taken from the PEP).

Where N is an ascii number, + means and more of, [ ] makes something optional, ( ) are grouping operators and | is an OR. Everything else is a literal.

It's sort of a pseudo regex.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 20, 2013

Member

But just to be clear I think pip or Wheel needs to be better at this because the tooling should not silently create a a version that can't then be installed with ==. It should either work or it should fail up front IMO.

Member

dstufft commented Aug 20, 2013

But just to be clear I think pip or Wheel needs to be better at this because the tooling should not silently create a a version that can't then be installed with ==. It should either work or it should fail up front IMO.

@davedash

This comment has been minimized.

Show comment
Hide comment
@davedash

davedash Aug 20, 2013

So the maintainer of wheel feels like the onus is on pip to version match more liberally. It seems reasonable (as in, I don't see it breaking thing. I have yet to encounter someone who versions something as 1.0-beta and 1.0_beta).

davedash commented Aug 20, 2013

So the maintainer of wheel feels like the onus is on pip to version match more liberally. It seems reasonable (as in, I don't see it breaking thing. I have yet to encounter someone who versions something as 1.0-beta and 1.0_beta).

@davedash

This comment has been minimized.

Show comment
Hide comment
@davedash

davedash Aug 20, 2013

So I wasn't sure if this was the right place to change things. I could have it translate the - to _ and yield both the original and translated version strings.

Not familiar with the internals of pip to know if that's a good spot, if it is I can put together a PR.

davedash commented Aug 20, 2013

So I wasn't sure if this was the right place to change things. I could have it translate the - to _ and yield both the original and translated version strings.

Not familiar with the internals of pip to know if that's a good spot, if it is I can put together a PR.

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Aug 20, 2013

Contributor

Hi @davedash - no I don't think that's the right place for a fix. The fix, if it should be in pip at all (which I'm not yet convinced of) should be in PackageFinder, or at least somewhere in index.py. Probably somewhere in here: https://github.com/pypa/pip/blob/develop/pip/index.py#L453

Contributor

carljm commented Aug 20, 2013

Hi @davedash - no I don't think that's the right place for a fix. The fix, if it should be in pip at all (which I'm not yet convinced of) should be in PackageFinder, or at least somewhere in index.py. Probably somewhere in here: https://github.com/pypa/pip/blob/develop/pip/index.py#L453

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Aug 20, 2013

Contributor

Ok, it does seem that the wheel filename format has different needs than any previous Python distribution format filename, which indicates that perhaps a fix in pip is indicated.

I don't think this is a simple fix, though. In general pip defers version parsing to pkg_resources.parse_version, and parse_version does not consider - and _ equivalent in version strings. So we can't easily carry the equivalency the whole way through the chain (and if we did it would need to be via a change to pkg_resources, not pip). And if they aren't equivalent everywhere, it's hard to make them equivalent for finding wheel files, because given a wheel file with an _ in the version we have to parse it one way or the other to add to the list of "available versions", and we have no way to know what character it really is supposed to be (i.e. which way the version would be specified in a user's requirements file). I guess we could add both versions (the original with _ and one with it changed to -) to the list of available versions, both pointing to the same file if they are chosen as best version? That gets ugly, especially if you consider that a version with two _ in it would result in four different versions (all possible combinations) being added to the list of possible candidate versions.

Which leads me to think that if we make this change, we need to go all the way to the root and change pkg_resources.parse_version, turning - and _ into indistinguishable characters all through the Python legacy-versioning ecosystem. Which is perhaps feasible (if Jason Coombs is amenable), though it seems odd to make a change like that to the semantics of a legacy versioning system that we're in the midst of trying to phase out entirely (in favor of PEP 440).

@dholth I'd be interested if you have further thoughts on what approach pip should take here. (also /cc @qwcode)

Contributor

carljm commented Aug 20, 2013

Ok, it does seem that the wheel filename format has different needs than any previous Python distribution format filename, which indicates that perhaps a fix in pip is indicated.

I don't think this is a simple fix, though. In general pip defers version parsing to pkg_resources.parse_version, and parse_version does not consider - and _ equivalent in version strings. So we can't easily carry the equivalency the whole way through the chain (and if we did it would need to be via a change to pkg_resources, not pip). And if they aren't equivalent everywhere, it's hard to make them equivalent for finding wheel files, because given a wheel file with an _ in the version we have to parse it one way or the other to add to the list of "available versions", and we have no way to know what character it really is supposed to be (i.e. which way the version would be specified in a user's requirements file). I guess we could add both versions (the original with _ and one with it changed to -) to the list of available versions, both pointing to the same file if they are chosen as best version? That gets ugly, especially if you consider that a version with two _ in it would result in four different versions (all possible combinations) being added to the list of possible candidate versions.

Which leads me to think that if we make this change, we need to go all the way to the root and change pkg_resources.parse_version, turning - and _ into indistinguishable characters all through the Python legacy-versioning ecosystem. Which is perhaps feasible (if Jason Coombs is amenable), though it seems odd to make a change like that to the semantics of a legacy versioning system that we're in the midst of trying to phase out entirely (in favor of PEP 440).

@dholth I'd be interested if you have further thoughts on what approach pip should take here. (also /cc @qwcode)

@davedash

This comment has been minimized.

Show comment
Hide comment
@davedash

davedash Aug 20, 2013

though it seems odd to make a change like that to the semantics of a legacy
versioning system that we're in the midst of trying to phase out entirely (in favor > of PEP 440).

If this is the case, then maybe the onus is on the packagers to change their version strings. It's trivial for me to do this on packages that I maintain internally at work, and I don't mind talking to other developers and encourage them to do the same.

So I'd accept a "WONTFIX".

davedash commented Aug 20, 2013

though it seems odd to make a change like that to the semantics of a legacy
versioning system that we're in the midst of trying to phase out entirely (in favor > of PEP 440).

If this is the case, then maybe the onus is on the packagers to change their version strings. It's trivial for me to do this on packages that I maintain internally at work, and I don't mind talking to other developers and encourage them to do the same.

So I'd accept a "WONTFIX".

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 21, 2013

Contributor

PEP440 doesn't have a real answer to the fork versioning problem.

btw, there is an issue to add a scheme for forks: a .localN scheme
https://bitbucket.org/pypa/pypi-metadata-formats/issue/1/add-local-numbering-to-pep-440
which nick created based on this thread: http://mail.python.org/pipermail/distutils-sig/2013-August/022264.html

Contributor

qwcode commented Aug 21, 2013

PEP440 doesn't have a real answer to the fork versioning problem.

btw, there is an issue to add a scheme for forks: a .localN scheme
https://bitbucket.org/pypa/pypi-metadata-formats/issue/1/add-local-numbering-to-pep-440
which nick created based on this thread: http://mail.python.org/pipermail/distutils-sig/2013-August/022264.html

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 21, 2013

Contributor

but short of the .localN scheme, what we've resolved to use at my workplace is .postN for now, because it will be PEP440 compliant and work with current tools.

Contributor

qwcode commented Aug 21, 2013

but short of the .localN scheme, what we've resolved to use at my workplace is .postN for now, because it will be PEP440 compliant and work with current tools.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 21, 2013

Contributor

btw, the wheel issue for this: https://bitbucket.org/dholth/wheel/issue/78/wheel-rewrites-versions-preventing
I responded over there, since I think wheel bears original responsibility for conflating "-" and "_"

in response to @carljm

  • no, I don't think we should try to change pkg_resources. we should leave that be.
  • if wheel doesn't make the change I suggest, then I think the simplest change is to assume "-" was meant, since that will be much more common for forking. (the change would be here I think: https://github.com/pypa/pip/blob/develop/pip/wheel.py#L249)
Contributor

qwcode commented Aug 21, 2013

btw, the wheel issue for this: https://bitbucket.org/dholth/wheel/issue/78/wheel-rewrites-versions-preventing
I responded over there, since I think wheel bears original responsibility for conflating "-" and "_"

in response to @carljm

  • no, I don't think we should try to change pkg_resources. we should leave that be.
  • if wheel doesn't make the change I suggest, then I think the simplest change is to assume "-" was meant, since that will be much more common for forking. (the change would be here I think: https://github.com/pypa/pip/blob/develop/pip/wheel.py#L249)
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 21, 2013

Contributor

to be clearer, even if wheel makes the change I suggest, we still have to add the code to convert '_' to '-' in the version, it's just a matter of whether we can justifiably assume it, or whether we're just playing the odds.

Contributor

qwcode commented Aug 21, 2013

to be clearer, even if wheel makes the change I suggest, we still have to add the code to convert '_' to '-' in the version, it's just a matter of whether we can justifiably assume it, or whether we're just playing the odds.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 21, 2013

Member

I hate the .localN suffix :[ But I already told @ncoghlan that!

Member

dstufft commented Aug 21, 2013

I hate the .localN suffix :[ But I already told @ncoghlan that!

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 21, 2013

Contributor

and your solution? do you do a lot of local forking?

Contributor

qwcode commented Aug 21, 2013

and your solution? do you do a lot of local forking?

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 21, 2013

Contributor

but let's don't side track here with that. can you post your hate to the distutils thread or the issue

Contributor

qwcode commented Aug 21, 2013

but let's don't side track here with that. can you post your hate to the distutils thread or the issue

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 21, 2013

Contributor

if wheel doesn't make the change I suggest, then I think the simplest change is to assume "-" was meant

but making assumptions nags at me.
we'll see how the wheel maintainer responds to making wheels fail to build when versions have "_".

Contributor

qwcode commented Aug 21, 2013

if wheel doesn't make the change I suggest, then I think the simplest change is to assume "-" was meant

but making assumptions nags at me.
we'll see how the wheel maintainer responds to making wheels fail to build when versions have "_".

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 21, 2013

Contributor

change pkg_resources.parse_version, turning - and _ into indistinguishable characters

here's the scenario that concerns me with that.

  • before: 1.4_1 < 1.4a1
  • after: 1.4_1 > 1.4a1

before, "_" is effectively a pre-release symbol:
"Strings like "a", "b", "c", "alpha", "beta", "candidate" and so on (that come before "final" alphabetically) are assumed to be pre-release versions"

after, "_" (converted to "-") marks it as a qualifier to a final:
"strings like "-" and any alpha string that alphabetically follows "final" represents a "patch level"

Contributor

qwcode commented Aug 21, 2013

change pkg_resources.parse_version, turning - and _ into indistinguishable characters

here's the scenario that concerns me with that.

  • before: 1.4_1 < 1.4a1
  • after: 1.4_1 > 1.4a1

before, "_" is effectively a pre-release symbol:
"Strings like "a", "b", "c", "alpha", "beta", "candidate" and so on (that come before "final" alphabetically) are assumed to be pre-release versions"

after, "_" (converted to "-") marks it as a qualifier to a final:
"strings like "-" and any alpha string that alphabetically follows "final" represents a "patch level"

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Aug 21, 2013

Contributor

@qwcode yep I think I agree with you 100%. Making them interchangeable is too big a change to the semantics of pkg_resources versioning. But given that IME underscore is much less common in version strings than hyphens (I suppose @dstufft might have data sitting around to verify that hypothesis), and given wheel's legitimate need for hyphen to be unambiguously a separator character in the filename, I think having wheel refuse to build wheels if there's an underscore in the version, and then pip/wheel assume that underscores in the version are always really hyphens, is probably the best option. And you're right about where the change would be in pip; I didn't realize that PackageFinder outsourced the parsing of wheel versions from filenames to the Wheel class.

(Also @davedash I'm not ignoring your comment that you'd be ok with wontfix; even if you're ok with it I think this is the kind of issue that will give people the impression that wheels are broken, so I do think it's important that we find a workable fix.)

Contributor

carljm commented Aug 21, 2013

@qwcode yep I think I agree with you 100%. Making them interchangeable is too big a change to the semantics of pkg_resources versioning. But given that IME underscore is much less common in version strings than hyphens (I suppose @dstufft might have data sitting around to verify that hypothesis), and given wheel's legitimate need for hyphen to be unambiguously a separator character in the filename, I think having wheel refuse to build wheels if there's an underscore in the version, and then pip/wheel assume that underscores in the version are always really hyphens, is probably the best option. And you're right about where the change would be in pip; I didn't realize that PackageFinder outsourced the parsing of wheel versions from filenames to the Wheel class.

(Also @davedash I'm not ignoring your comment that you'd be ok with wontfix; even if you're ok with it I think this is the kind of issue that will give people the impression that wheels are broken, so I do think it's important that we find a workable fix.)

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 21, 2013

Member

From PyPI:

SELECT COUNT(*) FROM releases WHERE version ~* '_';
 count
-------
    84
(1 row)

And just for reference:

SELECT COUNT(*) FROM releases;
 count
--------
 169327
(1 row)
Member

dstufft commented Aug 21, 2013

From PyPI:

SELECT COUNT(*) FROM releases WHERE version ~* '_';
 count
-------
    84
(1 row)

And just for reference:

SELECT COUNT(*) FROM releases;
 count
--------
 169327
(1 row)
@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 21, 2013

Member

Just for completeness sake, here's a list of the actual version strings that do contain a _: https://gist.github.com/dstufft/6296910.

Member

dstufft commented Aug 21, 2013

Just for completeness sake, here's a list of the actual version strings that do contain a _: https://gist.github.com/dstufft/6296910.

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Aug 21, 2013

Contributor

@dstufft how many use a hyphen in the version, for comparison?

Contributor

carljm commented Aug 21, 2013

@dstufft how many use a hyphen in the version, for comparison?

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 21, 2013

Member
SELECT COUNT(*) FROM releases WHERE version ~* '-';
 count
-------
  5223
(1 row)
Member

dstufft commented Aug 21, 2013

SELECT COUNT(*) FROM releases WHERE version ~* '-';
 count
-------
  5223
(1 row)
@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Aug 21, 2013

Contributor

So the status quo means wheels are broken for 5223 packages; seems worth a fix that makes wheels broken for 84 instead (if we can't find a good fix that makes it work for all of them).

Contributor

carljm commented Aug 21, 2013

So the status quo means wheels are broken for 5223 packages; seems worth a fix that makes wheels broken for 84 instead (if we can't find a good fix that makes it work for all of them).

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 21, 2013

Contributor

outsourced the parsing of wheel versions from filenames to the Wheel class

I added the wheel class back when I added the logic for pip to prioritize wheels based on the pep425 tags. Didn't want to add any more "inline" logic.

seems worth a fix that makes wheels broken for 84 instead

yes, we can add the _ -> - conversion immediately, and we're much better off.
I can do that (and add a test for it).
but ideally, wheel disallows _ as well.

Contributor

qwcode commented Aug 21, 2013

outsourced the parsing of wheel versions from filenames to the Wheel class

I added the wheel class back when I added the logic for pip to prioritize wheels based on the pep425 tags. Didn't want to add any more "inline" logic.

seems worth a fix that makes wheels broken for 84 instead

yes, we can add the _ -> - conversion immediately, and we're much better off.
I can do that (and add a test for it).
but ideally, wheel disallows _ as well.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 22, 2013

Contributor

merged #1158, but will leave open for the sake of the few "_"-versioned projects, and to see if the wheel project makes any changes related to this.

Contributor

qwcode commented Aug 22, 2013

merged #1158, but will leave open for the sake of the few "_"-versioned projects, and to see if the wheel project makes any changes related to this.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 22, 2013

Contributor

actually, it seems projects with "_" verisions won't have an issue after this change.

Requirement.parse treats '-' and '_' the same.

so , I can require myproject==1.1_1, and the resulting requirement object will accept 1.1-1
(pip's finder uses this approach of seeing if a version string is in in the requirement)

Contributor

qwcode commented Aug 22, 2013

actually, it seems projects with "_" verisions won't have an issue after this change.

Requirement.parse treats '-' and '_' the same.

so , I can require myproject==1.1_1, and the resulting requirement object will accept 1.1-1
(pip's finder uses this approach of seeing if a version string is in in the requirement)

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 22, 2013

Contributor

Requirement.parse treats '-' and '_' the same.

sorry, I was completely wrong here. too late. my mistake. : (

Contributor

qwcode commented Aug 22, 2013

Requirement.parse treats '-' and '_' the same.

sorry, I was completely wrong here. too late. my mistake. : (

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 22, 2013

Member

It still shouldn't matter if we treat - and _ the same, I think it would only matte rif they have both - and _ like 1.0-foo AND 1.0_foo.

Member

dstufft commented Aug 22, 2013

It still shouldn't matter if we treat - and _ the same, I think it would only matte rif they have both - and _ like 1.0-foo AND 1.0_foo.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 22, 2013

Contributor

the merged change just makes it so we assume - was meant by _ in the wheel filename.

that still leaves the issue of projects with _ in their version.

e.g., this requirement, project==1.1_4 will end being compared with 1.1-4 (converted from the wheel filename after the latest change) and it will fail.

to make it work, we'd need to do something like carl mentioned above, which is a little hairy, but doable.
(know that we're not doing the matching, but rather using the in relationship with requirement objects)

but this is important, sdists convert _ to - in the filename, so projects having _ in versions were already crippled in terms of being found, so wheel is not any worse now, so we can rest on that I think.

Contributor

qwcode commented Aug 22, 2013

the merged change just makes it so we assume - was meant by _ in the wheel filename.

that still leaves the issue of projects with _ in their version.

e.g., this requirement, project==1.1_4 will end being compared with 1.1-4 (converted from the wheel filename after the latest change) and it will fail.

to make it work, we'd need to do something like carl mentioned above, which is a little hairy, but doable.
(know that we're not doing the matching, but rather using the in relationship with requirement objects)

but this is important, sdists convert _ to - in the filename, so projects having _ in versions were already crippled in terms of being found, so wheel is not any worse now, so we can rest on that I think.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 22, 2013

Contributor

Requirement.parse treats '-' and '_' the same.
sorry, I was completely wrong here. too late. my mistake. : (

but wait, I was partly right, just confused.

>>> r = pkg_resources.Requirement.parse('name==1.5_1')
>>> '1.5-1' in r
True
>>> r = pkg_resources.Requirement.parse('name==1.5-1')
>>> '1.5_1' in r
False

so, if a project is using _ versions (e.g. '1.1_1'):

  • the sdist will have 1.1-1 (pip will use it as is)
  • the wheel will have 1.1_1 (pip will convert it to '-')
  • and a user looking for project==1.1_1 will be fulfilled by both of these archives in pip
Contributor

qwcode commented Aug 22, 2013

Requirement.parse treats '-' and '_' the same.
sorry, I was completely wrong here. too late. my mistake. : (

but wait, I was partly right, just confused.

>>> r = pkg_resources.Requirement.parse('name==1.5_1')
>>> '1.5-1' in r
True
>>> r = pkg_resources.Requirement.parse('name==1.5-1')
>>> '1.5_1' in r
False

so, if a project is using _ versions (e.g. '1.1_1'):

  • the sdist will have 1.1-1 (pip will use it as is)
  • the wheel will have 1.1_1 (pip will convert it to '-')
  • and a user looking for project==1.1_1 will be fulfilled by both of these archives in pip
@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Aug 22, 2013

Contributor

In that case, it seems to me that this is now fully resolved and can be closed.

Contributor

carljm commented Aug 22, 2013

In that case, it seems to me that this is now fully resolved and can be closed.

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