Skip to content
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

Write long description in message payload #2628

Merged
merged 7 commits into from May 22, 2021
Merged

Conversation

cdce8p
Copy link
Contributor

@cdce8p cdce8p commented Mar 30, 2021

Summary of changes

Write the long description in the message body of the PKG-INFO file if the metadata version >= 2.1.
https://packaging.python.org/specifications/core-metadata/#description

wheel does this conversion already, see: wheel/metadata.py#L86-L89

Pull Request Checklist

@jaraco
Copy link
Member

jaraco commented Apr 3, 2021

Thanks for the contrib.

I'm not sure this change is a safe one. As you've pointed out, the 'wheel' package converts from egg-info to dist-info, and it assumes the PKG-INFO file has the description in a field. Won't that break if Setuptools starts putting the long description in metadata 2.1 format? Note also that metadata 2.1 allows the long description (and other multi-line values) to remain in the fields. My instinct is this change is only likely to cause disruption.

Can you write up a motivation about what benefit this change adds or what problem it solves?

setuptools/dist.py Outdated Show resolved Hide resolved
self.long_description = _read_field('description')
self.description = _read_field('summary')
self.long_description = _read_long_description()
if self.long_description is None and self.metadata_version >= StrictVersion('2.1'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a solution that doesn't select on metadata version? Note that long description in Metadata 2.1 could still use the Description field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will try to read the long description from the header field first. Only if the result is None and the mv >= '2.1' we try to read it from the payload as well. It will work that way, since in 2.1 an UNKNOWN value for the long description is added either in the header field or the payload.

@@ -83,6 +84,24 @@ def _read_list(name):
return None
return values

def _read_long_description():
value = msg['description']
if value in ('UNKNOWN', None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm uncomfortable with these "UNKNOWN" literals. Does Setuptools add "UNKNOWN" for unsupplied long descriptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UNKNOWN is the distutils default for get_long_description(). Since this is used when writing the metadata file, either the header field or the payload will contain UNKNOWN in case no description is given.

def get_long_description(self):
return self.long_description or "UNKNOWN"

setuptools/dist.py Outdated Show resolved Hide resolved
@cdce8p
Copy link
Contributor Author

cdce8p commented Apr 3, 2021

I'm not sure this change is a safe one. As you've pointed out, the 'wheel' package converts from egg-info to dist-info, and it assumes the PKG-INFO file has the description in a field. Won't that break if Setuptools starts putting the long description in metadata 2.1 format? Note also that metadata 2.1 allows the long description (and other multi-line values) to remain in the fields. My instinct is this change is only likely to cause disruption.

At least for wheel, this isn't an issue. wheel uses email.parser.Parser.parse to parse EGG-INFO and then msg["description"] to access the field. If the header field doesn't exist, this will return None. If it returns a string, wheel will dedent it and write it into the payload of the message, deleting the header field afterwards.
With this PR, the long description will be automatically written into the payload part of EGG-INFO, so wheel just doesn't need to do the dedent step. It's already present in the payload after parse.

Can you write up a motivation about what benefit this change adds or what problem it solves?

The main point would probably be to improve readability of the EGG-INFO file. If the long description is written in the payload instead of the header, all fields are easily readable. Additionally this is also the format that wheel uses, so it would lead to a better standard of sorts. Of course it works now too, so there isn't an active need to change it on that front.

@cdce8p
Copy link
Contributor Author

cdce8p commented Apr 15, 2021

I've rebased and updated this PR, after #2635 was merged. I would appreciate any additional feedback.

/CC: @jaraco @webknjaz

Comment on lines 867 to 869
environ = os.environ.copy().update(
HOME=env.paths['home'],
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dict.update() does not return anything, it updates the dict in-place. Meaning that environ=None always in this test.

I believe what you meant to do is:

Suggested change
environ = os.environ.copy().update(
HOME=env.paths['home'],
)
environ = dict(os.environ, HOME=env.paths['home'])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh I just copied that part from the test above without taking a closer look 😅 Turns out it works perfectly even completely without it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. So this means there's a bug in other tests...

setuptools/dist.py Outdated Show resolved Hide resolved
@cdce8p cdce8p force-pushed the long-desc branch 2 times, most recently from cd91195 to a9c72b3 Compare May 9, 2021 18:18
@cdce8p
Copy link
Contributor Author

cdce8p commented May 9, 2021

@jaraco Would you mind taking a look at this one again? Since both, #2635 and #2641 are now merged, the change is much more reasonable.

@jaraco jaraco merged commit 18d751d into pypa:main May 22, 2021
@cdce8p cdce8p deleted the long-desc branch May 22, 2021 23:33
@mhertz
Copy link

mhertz commented Jul 14, 2021

Sorry, but is there any way to stop this from happening? Currently I have to downgrade python-setuptools to 56 every time building an egg, and I see no switches plus no matter how messing with setup.py then does this i.e adds (long)description or Unknown into payload which result in error in program using the egg(yeah issue with program I know and did report it as bug there, but in mean-time merely). The metadata spec states can place in payload, but always happens. I tried omit field from setup.py and tried edit pkg-info manually and rebuild(removing payload and add instead to header, and/or change metadata-version field), but just gets unknown in payload, and pkg-info overwritten each time etc.

Thanks in advance.

@jaraco
Copy link
Member

jaraco commented Jul 15, 2021

@mhertz Sorry to hear this caused disruption. Now that I think about it, forcing projects to jump from metadata 1.x to 2.x probably should have been a backward-incompatible change.

What you're proposing is that Setuptools provide a flag and two different ways of rendering the metadata. Such a change would almost certainly complicate the implementation and require additional tests - a lot of work for one use-case. It sounds like you have already identified a fix for the root cause and also a (albeit clumsy) workaround (downgrade Setuptools).

Why not work to expedite the fix in the offending program?

This project may consider an option to fall back to the prior behavior, maybe even though a third-party plugin. But before it does that, it would really help to have an issue to track the issue. In particular:

  • What are the circumstances that produce the unwanted behavior (what program or programs fail to work with the newer metadata format)?
  • In what environments does this occur and why is downgrading Setuptools problemmatic?
  • What is the impact of the unwanted behavior? How much is the disruption?
  • How can one replicate the undisireable behavior (minimal repro)?

Finally, would you be willing to help contribute the solution?

@mhertz
Copy link

mhertz commented Jul 15, 2021

Thanks alot for your nice detailed reply, much appreciated :)

You're right of-course and I agree fully. I'm not good enough to make such changes honestly, and don't see the need on second thought, well more after reading your post, but regardless :)

It's of-course better to fix offending program than to complicate this project and wrong way to go about it obviously. Anyway I just thought as when the metadata spec states that it can do this, then maybe was a way to control this, or atleast explain the circumstances of this i.e. when happens - I checked if was contollable in some way already e.g. from long_description_content_type or alike etc, but didn't find any, but as said, agree with this isn't an issue of this project, and also downgrading for me is an easy workaround for the offending program in question, obviously, and sorry for exagerating the issue :)

The offending program is the deluge torrent client, which accepts plugins in egg format, and when enabling such plugin then also reads various fields from PKG-INFO e.g. name, version and description etc, and then shows error as description now returns none. By that token, I apologise as also just realized that I actually entirely misunderstood the issue altogether, as instead was that the offending program relies on description alias which also is warned upon in docs, unless i'm mistaken again, and instead should use summary merely, which also btw was my original posted solution in my bug-ticket to project, though instead should have made PR in retrospect, but just thought the issue was about description now being in payload, but which is long-description and unrelated here, sorry. I'll update my deluge bug-ticket shortly, as currently have erronimous info in it about root-cause, but again, nothing to do with you guys. https://dev.deluge-torrent.org/ticket/3476

So, thanks again for your response, and i'm sorry for the noise and agree with your points entirly, thank you :)

Edit: Sorry, my comment about description being an alias for summary, I got from setuptools docs, but in a section about using setup.cfg files I see now, but in actual core-metadata spec, description isn't listed as alias, and also description is the one in payload, optionally, and not long-description. Just wanted to correct that from above.

Edit2: Again sorry, long-description added into payload as a "metadata spec description entry" - confusing for me with metadata to setuptools mappings I.e I believe description = sumarry and long-description = description etc, and not even sure I'll 100% correct here still lol :)

@jaraco
Copy link
Member

jaraco commented Jul 18, 2021

I believe description = sumarry and long-description = description etc, and not even sure I'll 100% correct here still lol :)

Yes. I believe you got this right this time. Sorry it's confusing, but glad you're getting it figured out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants