twine<=3.1.1 bug: AttributeError: 'NoneType' object has no attribute 'group'

[webknjaz]

Your Environment

GitHub Actions workflow

1. Your operating system: macOS-latest

2. Version of python you are running:

python.org-provided dmgs for Python 3.8, 3.7, 3.6, 3.5, 2.7

3. How did you install twine? Did you use your operating system's package manager or pip or something else?

pip install and via tox (b/c I tried many ways during debugging)

4. Version of twine you have installed:

v3.1.1

5. Which package repository are you targeting?

PyPI/TestPyPI but it doesn't matter in the context of this issue because it's about twine check, not uploading the dists.

The Issue

So I'm in progress of packaging platform-specific dists. I was building them
using pip wheel in a tox env and then running delocate outside.
Then I started moving delocate into a toxenv (separate) and after that twine check started spitting out this traceback:

Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/runpy.py", line 193, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/twine/__main__.py", line 31, in <module>
    sys.exit(main())
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/twine/__main__.py", line 25, in main
    return dispatch(sys.argv[1:])
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/twine/cli.py", line 74, in dispatch
    return main(args.args)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/twine/commands/check.py", line 158, in main
    return check(args.dists)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/twine/commands/check.py", line 121, in check
    warnings, is_ok = _check_file(filename, render_warning_stream)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/twine/commands/check.py", line 73, in _check_file
    package = PackageFile.from_filename(filename, comment=None)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/twine/package.py", line 104, in from_filename
    py_version = meta.py_version
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/twine/wheel.py", line 47, in py_version
    return wheel_info.group("pyver")
AttributeError: 'NoneType' object has no attribute 'group'
Checking dist/*.whl: 
##[error]Process completed with exit code 1.

(here's that line in src: https://github.com/pypa/twine/blob/3.1.1/twine/wheel.py#L47)

Now, I understand that it looks like a bug in delocate (or/and maybe the isolation that tox creates for it) but still twine should error out more gracefully.

Imagine my surprise when I discovered that this has been "fixed" in master just a day before I started seeing it: 7111240#diff-c533b522e297bb77ebe6ed9089c754beR48-R51.

I'm not sure if it counts as a fix because it's a commit that adds annotations and the change looks like a side-effect, plus no tests for this were added. Should it return any when the regex doesn't match? This is still to be identified.

Steps to Reproduce

This is all I have atm: https://github.com/ansible/pylibssh/runs/625207843?check_suite_focus=true#step:15:68

[webknjaz]

Oh, here's the root cause: -rw-r--r-- 1 runner staff 1.4M Apr 28 08:30 *.whl

So the reproducer must be something like touch '*.whl' && pip install twine && twine check *.whl.

[bhrutledge]

Thanks @webknjaz. I missed the change in twine.wheel during the review. However, it mirrors behavior in twine.wininst that's been there for 6 years:

twine/twine/wininst.py

Lines 19 to 25 in da8d62f

	@property
	def py_version(self) -> str:
	m = wininst_file_re.match(self.filename)
	if m is None:
	return "any"
	else:
	return m.group("pyver")

Looks like that ultimately ends up in the package metadata:

twine/twine/package.py

Lines 112 to 120 in da8d62f

	def metadata_dictionary(self) -> Dict[str, MetadataValue]:
	meta = self.metadata
	data = {
	# identify release
	"name": self.safe_name,
	"version": meta.version,
	# file content
	"filetype": self.filetype,
	"pyversion": self.python_version,

@sigmavirus24 or @di any insight? Is sending "pyversion" : "any" acceptable?

[webknjaz]

I think the action item here is to document the desired behavior by adding a regression test.

[sigmavirus24]

Is sending "pyversion" : "any" acceptable?

I don't think so. I'm not confident that's allowed per the Wheel PEP

[deveshks]

I made that change, to reflect what was done in wininst.py as pointed above. I also added a test which stubs wheel_file_re.match to return None and follow the return "any" branch.

twine/tests/test_wheel.py

Lines 40 to 43 in da8d62f

	def test_version_parsing_missing_pyver(monkeypatch, example_wheel):
	wheel.wheel_file_re = pretend.stub(match=lambda a: None)
	assert example_wheel.py_version == "any"

Both the implementation and the test can be modified according to the accepted value if the {python_tag} is not present in {distribution}-{version}(-{build tag})?-{python tag}-{abi tag}-{platform tag}.whl.

[sigmavirus24]

@deveshks that doesn't answer the question of whether the Wheel PEP allows for "pyversion" to be "any" or if this is inventing things for the sake of testing

[deveshks]

that doesn't answer the question of whether the Wheel PEP allows for "pyversion" to be "any" or if this is inventing things for the sake of testing

I mentioned the test since there was a comment in the issue about a test not being added for the change, and I wanted to outline the test.

I also think that setting pyversion to any is not the correct solution if wheel_file_re.match is None as well, but I couldn't find any relevant details in PEP 427 to what will the allowed value be in such a case.

[deveshks]

@sigmavirus24

In order to move this issue forward towards a fix, may I know what would be an allowed value instead of pyversion: any ? Accordingly I can submit a fix for this in both wheel.py and wininst.py, and modify the appropriate unit tests.

[di]

This field is basically a freeform text field. Here's all the current unique values that exist for it on PyPI: https://gist.github.com/di/93019b51582899c294cfd76ee2d0a4a3

The only requirement that PyPI enforces is that this is source for source distributions.

I think in this case we should just pick some default value. In my opinion, "any" is acceptable.

[bhrutledge]

Thanks, @di. So, is there any action to take? Is it okay to close this?

[di]

I think in this case we should just pick some default value. In my opinion, "any" is acceptable.

I'm saying that if twine can't infer this value from the filename we should use some sane default and not raise an exception. I'm proposing we use "any".

[bhrutledge]

I'm saying that if twine can't infer this value from the filename we should use some sane default and not raise an exception. I'm proposing we use "any".

I think that's already done on master; details above: #612 (comment).

[di]

Sorry, I had missed that. Yes, I would consider this fixed in #607 (although it probably shouldn't have been, since that PR was just adding type annotations).

[webknjaz]

I think the action item would be to add a test case to document such a decision before closing this issue.

[deveshks]

I think the action item would be to add a test case to document such a decision before closing this issue.

There is a test added for it as described in a comment above (#612 (comment)). Should this be enough?

[di]

Yes.