Allow + in wheel filenames

[ksunden]

The latest releases of pip (released today, v20.3.4 and v21.0) add a verification step which checks the info in the generated filename against the package. (pypa/pip#9320, specifically these lines)

This caused versions with extra info (e.g. git branch) appended to the version using + to fail the check, since flit replaces all non alphanumeric characters with _ in the filename (the actual metadata still has the plus sign, but the check is against the filename).

I checked against behavior of setuptools, which retains + in the filename of the wheel.

This simply allows the + character in the version part of the filename (I believe it is not allowed in the package name itself)

As to whether a filename with an _ in place of + should fail such a verification, I'm not totally convinced, but it is easy enough to allow the character.

[ksunden]

To be clear, this is not a problem for released versions (which must not have the local identifier to be uploaded to pypi), but it is affecting my CI systems.

[ksunden]

ping @takluyver

[flying-sheep]

@takluyver can you please appoint additional maintainers? Flit is used by many people and it would make sense to have a faster response time to bugs like this.

[takluyver]

I believe that pip and setuptools are going against the spec here, because that regex comes directly from the spec, specifically PEP 427, which also says

Each component of the filename is escaped by replacing runs of non-alphanumeric characters with an underscore _

It may be that we end up changing the spec and fixing Flit rather than fixing setuptools or pip. But I'd like to check that with the pip maintainers.

[takluyver]

I have opened an issue on pip to discuss this: pypa/pip#9628

[flying-sheep]

OK! See https://discuss.python.org/t/escaping-versions-for-wheel-sdist-and-dist-info-names/5605/19:

I think Flit can provide best forward-compatibility by normalising the version part (by either packaging.utils.canonicalize_version(s) or str(packaging.version.Version(s)))

So we should either go ahead with the change in this PR or switch to one of the suggested alternatives.

[takluyver]

Thanks! We're already normalising the version number (search for normalise_version).

Do you have a workaround for the time being? I'd prefer to wait until a change to the spec is accepted before doing a new release, to avoid having an intermediate state that might not fit with either the old or new version of the spec.

[flying-sheep]

What @uranusjr meant is that the version part of the wheel filename should be the normalized version without any additional mangling.

Therefore this PR should help already.

[flying-sheep]

In compliance with the recently changed spec, this should be the correct:

Suggested change

	return '{}-{}-{}.whl'.format(
	re.sub(r"[^\w\d.]+", "_", self.metadata.name, flags=re.UNICODE),
	re.sub(r"[^\w\d.]+", "_", self.metadata.version, flags=re.UNICODE),
	re.sub(r"[^\w\d\+.]+", "_", self.metadata.version, flags=re.UNICODE),
	tag)
	# See https://packaging.python.org/specifications/binary-distribution-format/#escaping-and-unicode
	assert '-' not in self.metadata.version, 'Normalized versions can’t have dashes'
	return '{}-{}-{}.whl'.format(
	re.sub(r"[-_.]+", "_", self.metadata.name, flags=re.UNICODE),
	self.metadata.version,
	tag)

[uranusjr]

Where does self.metadata.version come from? Dashes are allowed in Core metadata (the field used in the {name}-{version}.dist-info/METADATA file), so the assertion may not be correct depending on how Flit implements this.

[flying-sheep]

Metadata is created via

https://github.com/takluyver/flit/blob/8f93601d05abe00c833663df541200114dbf3ad1/flit_core/flit_core/common.py#L364-L368

where get_info_from_module does

https://github.com/takluyver/flit/blob/8f93601d05abe00c833663df541200114dbf3ad1/flit_core/flit_core/common.py#L180

and check_version calls

https://github.com/takluyver/flit/blob/8f93601d05abe00c833663df541200114dbf3ad1/flit_core/flit_core/common.py#L205

[takluyver]

I think @flying-sheep's suggestion is correct - so long as our implementation of version normalisation is correct (and I've read PEP 440 correctly!), there should be no dashes in version.

I'd also like to check when constructing the filename that the compatibility tag has exactly two dashes ({python tag}-{abi tag}-{platform tag}), to avoid any nasty surprises. But that check can be added later.

[flying-sheep]

We need to adapt both the wheel filename and the dist_info name. @uranusjr both have identical normalization, right?

https://github.com/takluyver/flit/blob/8f93601d05abe00c833663df541200114dbf3ad1/flit_core/flit_core/common.py#L380-L381

Once we do that, this also fixes #394

[uranusjr]

Assuming Flit only accepts PEP 440 versions, the normalisation logic should be identical in both places, yes.

[takluyver]

That's a good point. Flit can allow non-pep-440 version numbers with FLIT_ALLOW_INVALID=1 set. That's not recommended as a normal mode of operation - it's meant as an escape hatch to get around bugs in Flit's validation. But we should be aware that it's possible.

[flying-sheep]

Well, let’s get something released that works at all, then we can address deprecated corner cases whenever?

[takluyver]

Superseded by #395; thanks @ksunden for making the PR, though.