Fix handling of continuation lines in Description metadata field

[dnicolodi]

Fixes #1218.

[sigmavirus24]

Suggested change

	or all(line.startswith(" |") for line in lines[1:])
	or all(line.startswith(" ") for line in lines[1:])
	or all(line.startswith((" |", " ")) for line in lines[1:])

[dnicolodi]

This would implement a different check. The code as written tests whether all the line start with 8 spacer or all the lines start with 7 spaced and a pipe. The proposed code tests whether all the lines start with either 8 spaces or 7 spaces and a pipe. I don't think we should treat intermixed continuation style as valid. There is a test that ensures that someone else is not tempted into the same erroneous simplification.

[dnicolodi]

Anyone?

[dnicolodi]

Can anyone have a look at this? It is my understanding that this solves a problem encountered by projects using old setuptools.

[woodruffw]

Can anyone have a look at this? It is my understanding that this solves a problem encountered by projects using old setuptools.

I'll take a look at it in the coming days; the current setuptools adoption curve looks pretty good in terms of people probably not encountering this very often, so IMO it's a relatively low-priority issue.

Stats here: https://clickpy.clickhouse.com/dashboard/setuptools

[dnicolodi]

@woodruffw What should I be looking at in the stats page you linked? I think that those are download statistics for setuptools. However, the problem with the Description field has been reported by OpenStack developers. IIUC OpenStack wheels are built (or were built) with the setuptools package provided by the platform. This kind of usage would not influence download statistics. Am I looking a the wrong thing?

[woodruffw]

This kind of usage would not influence download statistics. Am I looking a the wrong thing?

You're looking at the right thing; you're right that OpenStack's numbers wouldn't influence the stats here. My larger point was that the ecosystem as a whole has a pretty healthy adoption curve for newer versions of setuptools, even if OpenStack is baking an older version for their own purposes.

In other words, there are three factors here that make me hesitant to add a non-standard compat kludge here:

1. The version of setuptools in question is half a decade old and is running primarily on EOL Python versions;
2. The overwhelming majority of users (i.e. those outside of OpenStack) are not hitting this, per the adoption curve;
3. It's my personal opinion that tools like twine should act like a forcing function for standards-conformant behavior, rather than allowing old incompatibilities to continue percolating through the ecosystem. Alex Gaynor had a great post on this recently.

[dnicolodi]

I'm definitely onboard with nudging people toward adoption of more standard compliant tools. However:

1. Half of the fix in this PR is for standard compliant serialization of metadata, although for a format that is discouraged and not used by any modern release of any tool I know of.
2. There are other compatibility kludge for broken setuptools behavior in the codebase. It is hard to justify why the one added here is worst that the others. The setuptools maintainers clearly stated that fixing these other issues is not a priority and will not happen any time soon. If we are in the busyness of nudging people toward standard compliance, we should remove these other compatibility kludge and force the hand of the setuptools maintainers. However, I'm quite sure that twine will be the project getting the bug reports from angry users, not setuptools.
3. The change in behavior from accepting the old setuptools serialization format to error on it happened in a minor release. I don't know how much twine follows semantic versioning, but a PR of mines caused the change in behavior and I felt compelled to fix it. 4. If there is no intention of fixing this issue, this should be stated and motivated in the issue that prompted this PR. Leaving the issue open gives the impression that the deserialization issue is acknowledged and the resolution is pending, not that the behavior is intentional and users should fix their tooling and not hold off updating twine till a release containing the patch in this PR is available.

[woodruffw]

1. Half of the fix in this PR is for standard compliant serialization of metadata, although for a format that is discouraged and not used by any modern release of any tool I know of.

Sorry, I might be missing something here -- packaging should be handling the two compliant serializations for us, right? AFAIK neither is discouraged although Metadata 2.1 has a "may" for putting it in the message body instead.

2. There are other compatibility kludge for broken setuptools behavior in the codebase. It is hard to justify why the one added here is worst that the others.

I agree, although the directionality matters: one kludge is currently present, while this is one that would be (re-)added.

3. The change in behavior from accepting the old setuptools serialization format to error on it happened in a minor release. I don't know how much twine follows semantic versioning, but a PR of mines caused the change in behavior and I felt compelled to fix it.

I don't think there's a strict policy on following semver, but I think it's good to follow. At the same time, cases like this demonstrate one of semver's weaknesses: this is either a deprecation/removal (in which case it needs a major bump) or it's a bugfix for unintentionally permissive behavior (in which case it's fine as a patch).

In a case like this I'm (selfishly) motivated to treat it as the latter 🙂

4. If there is no intention of fixing this issue, this should be stated and motivated in the issue that prompted this PR. Leaving the issue open gives the impression that the deserialization issue is acknowledged and the resolution is pending, not that the behavior is intentional and users should fix their tooling and not hold off updating twine till a release containing the patch in this PR is available.

It's not that there's no intention -- the issue in question (#1218) hasn't received that many "me too"s, and so doing a fix along with a release cycle isn't the highest priority. If more people pop up with this issue, it would make sense to prioritize it more.

(I'm sorry if this is not a satisfying answer -- I realize it can make it feel like PRs/bugs are in limbo.)

[dnicolodi]

1. Half of the fix in this PR is for standard compliant serialization of metadata, although for a format that is discouraged and not used by any modern release of any tool I know of.

Sorry, I might be missing something here -- packaging should be handling the two compliant serializations for us, right? AFAIK neither is discouraged although Metadata 2.1 has a "may" for putting it in the message body instead.

Nope, packaging does not handle the indentation used to have Description as an RFC 822 header, see pypa/packaging#867.

[dnicolodi]

It's not that there's no intention -- the issue in question (#1218) hasn't received that many "me too"s, and so doing a fix along with a release cycle isn't the highest priority. If more people pop up with this issue, it would make sense to prioritize it more.

I really dislike "me too" comments on bug reports as in the general case they do not serve any purpose: a bug is a bug and usually needs to be fixed. The may serve a purpose on feature requests, but this was classified as a bug. Unless there is a clear statement that this issue is going to be resolved only if a certain numbers of users report it, it is unreasonable to expect users to add "me too" comments to an issue that has already been triaged and for which there is already a fix available.

Also, I don't like mixing a popularity context with technical arguments.

(I'm sorry if this is not a satisfying answer -- I realize it can make it feel like PRs/bugs are in limbo.)

It not a feeling: you are explicitly saying that issues and PRs are not looked at unless there is a minimum number (much larger than one) of users demanding that someone pays attention to them. If this hold true for an 80 lines patch, I cannot imagine how many users someone would need to mobilize to have a larger contribution looked as.

[sigmavirus24]

@dnicolodi you seem to be reacting to this in a way that isn't coming across as kind of collaborative but rather accusatory and combative.

If you're upset you spent time working on this, that's reasonable and understandable. If you're upset on some pseudo-moral ground of fixing anything someone somewhere deems a bug, you may want to reconsider your participation. No project will ever be bug free. Fixing 100% of bugs is not reasonable and impact is a very important consideration for people stretched too thinly like William and myself.

As it stands you've made meaningful and significant contributions to this project which I and others greatly appreciate. I would be sad to see you go. I don't want you to feel like you're wasting time either or becoming agitated over something not being accepted. That's not healthy or something I wish for anyone. Consider taking time to understand your motivations and if they align with how this project is run for your own well-being

[woodruffw]

Nope, packaging does not handle the indentation used to have Description as an RFC 822 header, see pypa/packaging#867.

Ah, this was my misunderstanding; thanks for linking. I think perhaps if we start with just the standard behavior here, that would be pretty easy to review (and isn't remotely controversial, unlike the non-standard kludge).

Also, I don't like mixing a popularity context with technical arguments.

To be clear, I don't like "me too" comments either. That's not what I meant; I meant the little 👍 👎 reactions on posts that people use to signal their interest. I think those can be a valuable signal for how work should be prioritized.

(I also don't see things so black-and-white: part of maintaining a project is deciding how to slice your time, and as it stands I - unfortunately - slice my time pretty thinly. Part of making sound technical decisions is knowing one's own limits, including whether one should prioritize reviewing one thing that doesn't seem to be causing too much breakage versus the 50+ other things going on.)