classification
Title: distutils2 sdist does not complain about version that is not PEP 386 compliant
Type: Stage: resolved
Components: Distutils2 Versions: Python 3.3, 3rd party
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: eric.araujo Nosy List: SilentGhost, eric.araujo, gotcha, rik.poggi, swamiyeswanth, tarek
Priority: normal Keywords: easy, patch

Created on 2011-01-29 14:55 by gotcha, last changed 2014-03-13 20:30 by eric.araujo. This issue is now closed.

Files
File name Uploaded Description Edit
diff swamiyeswanth, 2011-02-20 15:25
patch1.diff swamiyeswanth, 2011-03-21 06:06
issue11060_test_v1.diff berker.peksag, 2011-12-01 12:47 review
test_pypi_dist.diff rik.poggi, 2012-04-05 13:01 test on irrational versions review
metadata_set_extract.py rik.poggi, 2012-04-06 10:33 extract from packaging/metadata.py (set method from Metadata class)
test_support.diff rik.poggi, 2012-04-06 23:04 shows shuffling of metadata fields while testing review
issue_tests.diff rik.poggi, 2012-04-09 16:24 failing tests - metadata should warn and check cmd should raise review
Messages (22)
msg127425 - (view) Author: Godefroid Chapelle (gotcha) * Date: 2011-01-29 14:55
sdist does not complain about 

version = 0.4.5dev

which is not PEP 386 compliant
msg127909 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-02-04 17:30
Strange, the regex comes straight from the PEP, which has had careful review.  It must be somewhere else in the code.  Would you like to work on a patch, or a test first?  We have to find out if sdist is responsible, or metadata, or version, etc.
msg128802 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-02-18 19:05
I think Version should be strict, then Metadata could have an option for strictness (defaulting to true), so that mkcfg, sdist and other would strict mode but users could get more flexibility when needed.
msg128838 - (view) Author: yeswanth (swamiyeswanth) Date: 2011-02-19 10:07
I have gone through the source of mkcfg.py and though of implementing version check by calling distutils2.is_valid_version() and then if it results false , call suggest_normalized_version() to ask the user if he would like the suggested version or enter a new version ...if it returns none, prompting the version again . For this i need to create a new function say "check_version()" and call it just after version is asked for in the query_user() function call.
msg128883 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-02-19 21:50
You don’t need a new function (and certainly not in the __init__ submodule, the right place would be the version module), just do something like this:

try:
    NormalizedVersion(text)
except IrrationalVersionError:
    # version is invalid

Alternative: use suggest_normalized_version, which returns a string or None, then display according text to prompt the user to accept the suggested version or type another.
msg128905 - (view) Author: yeswanth (swamiyeswanth) Date: 2011-02-20 12:59
I guess this needs to be reviewed . Added support for checking version in mkcfg.py by making use of suggest_normalized_version .. (didnt not an error  for non matching suggestion , though it will prompt for the version  it again )
msg128906 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-02-20 13:03
Not bad!  You need to test the script, at least manually, if possible with unit tests.  This typo slipped in:

+		    self.data.get['version']=suggested_version
Remove “.get” (also add spaces around operators).
msg128908 - (view) Author: yeswanth (swamiyeswanth) Date: 2011-02-20 15:24
Sorry for the typo. I ran the code though and it worked fine. I guess might have patched a wrong file :( . Made the changes as you asked. Will try for the unit tests :)
msg130124 - (view) Author: yeswanth (swamiyeswanth) Date: 2011-03-05 16:47
I am trying for unit tests here , but would need some assistance. I included a file here , please review that. first of all i am not able to access self.data['version'](shows an exception , here). suggest me a solution here .
msg130142 - (view) Author: yeswanth (swamiyeswanth) Date: 2011-03-06 03:48
is there any default version in pep 386?
msg131565 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-03-20 23:21
data['version'] is not always present.  Can you find what part of the code fills it?

There is no default in the PEP, but a tool like mkcfg could suggest a useful value, say 0.1dev0.
msg131615 - (view) Author: yeswanth (swamiyeswanth) Date: 2011-03-21 06:06
Added some test cases for it .
msg131637 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-03-21 12:26
Nice test.  Remarks: you don’t need to instantiate MainProgram; the test would be better in test_version; you should test that “0.4.5dev” is rejected, as it’s what was initially reported.

Testing versions in mkcfg will be a bit more difficult; testing versions in sdist and bdist_dumb should be easy.
msg131641 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2011-03-21 12:32
Please, don't use tabs to indent your code. Also check the trailing spaces and tabs.
msg148722 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-12-01 14:00
Hi Berker Peksag, thanks for your interest in contributing.  Your patch is not what I had in mind: In test_command_sdist, a test should create an sdist and check that it fails with an error message, not instantiate a version object.  Distutils2 has many layers; at the base, version needs to reject an incorrect version with a Python exception (and we should test that in test_version), at the top, commands like check, sdist and bdist should catch that exception and display a user-friendly error message (and their tests should create Python files and run the command).  Does this help?
msg157550 - (view) Author: Rik Poggi (rik.poggi) Date: 2012-04-05 07:59
Hi, I'd like to contribute to this bug if it's possible.

I would've directly started, but it seems the situation has moved/changed a bit, so I'm not sure where the tests are needed.

There's a try/except, like the one mentioned above, in pypi/dist.py. Was that a consequence of this bug? Are tests needed for that conversion?

The sdist command seems to be not complaining about any kind of irrational/invalid version. Should this be fixed? Should sdist check for a valid version number and do something like what Éric Araujo mentioned in the last comment?
msg157581 - (view) Author: Rik Poggi (rik.poggi) Date: 2012-04-05 13:01
Strictly related or not to this bug, a bit more test coverage shouldn't hurt.

So while waiting for a reply I started writing a couple of tests for pypi/dist.py, hope they look good.
msg157602 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-04-05 16:29
Thanks for the tests!  I should have some time this week-end to review them and explain clearly what this bug is about and where tests should go.
msg157656 - (view) Author: Rik Poggi (rik.poggi) Date: 2012-04-06 10:33
Thanks, I'll wait! In the meanwhile I couldn't help to dig a little deeper and found that the Metadata class is currently logging a warning. Should the commands raise something when there's a warning in strict mode?

I was playing with the check command about this when I stumbled in an odd looking piece of code in metadata.py with a FIXME note: "# FIXME this rejects UNKNOWN, is that right?" (see attached file). I'm not sure how, but it seems related to a "random" (I couldn't find any pattern) log message that sometimes give: 

"'UNKNOWN': '0.4.5dev' is not a valid version (field 'Version')"

and others:

"'Name': '0.4.5dev' is not a valid version (field 'Version')"

(I had this with consecutive execution from a simple test that I wrote in test_command_check, with metadata['name'] == 'Name').

I hoped to not have wasted your time, but I thought that it could may be related to this bug since it seems that the version gets rightfully (but strangefully) warned as not valid from this "middle-layer" check point.
msg157710 - (view) Author: Rik Poggi (rik.poggi) Date: 2012-04-06 23:04
My assumption that the "random" log message was related to this bug seems to be completely wrong. It seems, instead, related to a starstar call of create_dist in the support module that will loose the order (of an OrderedDict obviously). The behaviour is still strange because in order to reproduce it I had to re-run the test different. (test_star_star is the "random" failing one) Attaching the diff.
msg157858 - (view) Author: Rik Poggi (rik.poggi) Date: 2012-04-09 16:24
Moving on, I've understood what the bug is about. I've made a couple of tests for this issue. I'm waiting for a review before adding others (if necessary).

The fix is not going to be easy, because I'm not sure about the Metadata design.

I think that what should be done is to make available the old version scheme check for metadata-version 1.0 and 1.1.
msg213459 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2014-03-13 20:30
distutils2 development has stopped.
History
Date User Action Args
2014-03-13 20:30:51eric.araujosetstatus: open -> closed
resolution: out of date
messages: + msg213459

stage: test needed -> resolved
2012-10-25 21:45:03berker.peksagsetnosy: - berker.peksag
2012-04-09 16:24:38rik.poggisetfiles: + issue_tests.diff

messages: + msg157858
2012-04-06 23:04:26rik.poggisetfiles: + test_support.diff

messages: + msg157710
2012-04-06 10:33:48rik.poggisetfiles: + metadata_set_extract.py

messages: + msg157656
2012-04-05 16:29:50eric.araujosetmessages: + msg157602
2012-04-05 13:01:22rik.poggisetfiles: + test_pypi_dist.diff

messages: + msg157581
2012-04-05 07:59:47rik.poggisetnosy: + rik.poggi
messages: + msg157550
2011-12-01 14:00:53eric.araujosetassignee: tarek -> eric.araujo
stage: test needed
messages: + msg148722
versions: + Python 3.3
2011-12-01 12:47:36berker.peksagsetfiles: + issue11060_test_v1.diff
nosy: + berker.peksag
2011-03-21 12:32:18SilentGhostsetnosy: + SilentGhost
messages: + msg131641
2011-03-21 12:26:35eric.araujosetnosy: tarek, eric.araujo, swamiyeswanth, gotcha
messages: + msg131637
2011-03-21 06:06:25swamiyeswanthsetfiles: + patch1.diff

messages: + msg131615
keywords: + patch
nosy: tarek, eric.araujo, swamiyeswanth, gotcha
2011-03-21 06:05:47swamiyeswanthsetfiles: - help.txt
nosy: tarek, eric.araujo, swamiyeswanth, gotcha
2011-03-20 23:21:38eric.araujosetnosy: tarek, eric.araujo, swamiyeswanth, gotcha
messages: + msg131565
2011-03-06 03:48:14swamiyeswanthsetnosy: tarek, eric.araujo, swamiyeswanth, gotcha
messages: + msg130142
2011-03-05 16:47:02swamiyeswanthsetfiles: + help.txt
nosy: tarek, eric.araujo, swamiyeswanth, gotcha
messages: + msg130124
2011-02-20 15:25:07swamiyeswanthsetfiles: + diff
nosy: tarek, eric.araujo, swamiyeswanth, gotcha
2011-02-20 15:24:46swamiyeswanthsetfiles: - diff
nosy: tarek, eric.araujo, swamiyeswanth, gotcha
2011-02-20 15:24:26swamiyeswanthsetfiles: + diff
nosy: tarek, eric.araujo, swamiyeswanth, gotcha
messages: + msg128908
2011-02-20 15:22:11swamiyeswanthsetfiles: - diff
nosy: tarek, eric.araujo, swamiyeswanth, gotcha
2011-02-20 13:03:48eric.araujosetnosy: tarek, eric.araujo, swamiyeswanth, gotcha
messages: + msg128906
2011-02-20 12:59:40swamiyeswanthsetfiles: + diff
nosy: tarek, eric.araujo, swamiyeswanth, gotcha
messages: + msg128905
2011-02-19 21:50:43eric.araujosetnosy: tarek, eric.araujo, swamiyeswanth, gotcha
messages: + msg128883
2011-02-19 10:07:06swamiyeswanthsetnosy: tarek, eric.araujo, swamiyeswanth, gotcha
messages: + msg128838
2011-02-18 19:05:59eric.araujosetnosy: + swamiyeswanth
messages: + msg128802
2011-02-04 17:30:18eric.araujosetkeywords: + easy
versions: + 3rd party
messages: + msg127909
nosy: tarek, eric.araujo, gotcha
2011-01-29 14:55:42gotchacreate