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

Revert erroring when readme content does not render #4099

Merged
merged 3 commits into from
May 29, 2018

Conversation

theacodes
Copy link
Contributor

This temporarily disables the validation added #3980 which is preventing several users from uploading packages (#4079).

@@ -919,6 +919,7 @@ def test_fails_with_invalid_names(self, pyramid_config, db_request, name):
"for more information."
).format(name)

@pytest.mark.xfail(reason='https://github.com/pypa/warehouse/issues/4079')
Copy link
Member

Choose a reason for hiding this comment

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

black will want these to be " instead of ', though that landed while I was away :-D not sure if we enforce it in linter yet.

description_content_type = form.description_content_type.data
if not description_content_type:
description_content_type = "text/x-rst"
rendered = readme.render(
Copy link
Member

Choose a reason for hiding this comment

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

import of warehouse.utils.readme on line 55 no longer used.

@ewdurbin
Copy link
Member

Looks like the original PR no longer cleanly reverts which is a bummer, so the comment out approach is OK to keep from losing too much context but isn't the standard approach.

Do we know what the long term fix is for the issue?

@theacodes
Copy link
Contributor Author

Do we know what the long term fix is for the issue?

Not sure, I'll continue investigating.

@ewdurbin
Copy link
Member

ewdurbin commented May 29, 2018

@theacodes perfect. if you can push the lint issue fixups, we'll get this merged and deployed to unblock publications.

@theacodes
Copy link
Contributor Author

delinted

@di
Copy link
Member

di commented May 29, 2018

It seems to me like this is working as expected, and that the issue lies with either twine, wheel or setuptools versions being old and not supporting Metadata 2.1, or users using distutils instead of twine to upload.

@theacodes
Copy link
Contributor Author

theacodes commented May 29, 2018 via email

@ewdurbin
Copy link
Member

gonna merge up to unblock users. any opposition @di?

@ewdurbin
Copy link
Member

On closer inspection I agree with @di that this is a metadata/client issue. I'm not sure about the revert given that, but do see at least one scenario where the wheel upload worked but sdist did not. This still seems like it could be a client issue but is confusing enough that it probably requires further investigation before shipping the validation.

@ewdurbin ewdurbin merged commit 559b6eb into pypi:master May 29, 2018
@ocefpaf
Copy link

ocefpaf commented May 29, 2018

It seems to me like this is working as expected, and that the issue lies with either twine, wheel or setuptools versions being old and not supporting Metadata 2.1, or users using distutils instead of twine to upload.

I'm experiencing this using Travis-CI auto deployment feature. According to the logs Travis-CI is using

  • setuptools 39.2.0
  • twine 1.11.0
  • wheel 0.31.1

those look new enough but I'm not sure if Travis-CI is calling twine or setup.py to upload.

See https://travis-ci.org/pyoceans/erddapy/jobs/385360404#L1591

Thanks to this PR the deployment worked as expected: https://travis-ci.org/pyoceans/erddapy/jobs/385409165

(But the Markdown description is not rendering as expected 😬)

@di
Copy link
Member

di commented May 31, 2018

(But the Markdown description is not rendering as expected 😬)

Right, this is what we were trying to prevent. After taking a look at this, I'm fairly sure the problem is either out of date dependencies or using setup.py upload instead of twine. For any environment for which these things are out of the user's control, using a Markdown description is essentially unsupported.

@ocefpaf
Copy link

ocefpaf commented May 31, 2018

Right, this is what we were trying to prevent.

I get the same if I upload manually with all tools updated and twine :-/

using a Markdown description is essentially unsupported.

That is fine IMO as long as the upload is not blocked.

Thanks everybody for the info and the quick action!

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.

4 participants