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

Fix upload metadata #1576

Merged
merged 19 commits into from Nov 12, 2018
Merged

Fix upload metadata #1576

merged 19 commits into from Nov 12, 2018

Conversation

@pganssle
Copy link
Member

@pganssle pganssle commented Nov 5, 2018

Summary of changes

This PR adds get_metadata_version to distutils.dist.DistributionMetadata and also monkey patches distutils.dist.DistributionMetadata.read_pkg_file to populate the metadata_version attribute. We also have overridden distutils.command.upload.upload_file so that the metadata version is retrieved from get_metadata_version rather than hard-coded as 1.0.

I felt that it would be preferable for read_pkg_file to actually store the specified metadata version rather than to infer it from the fields present (as is done when writing the metadata file), however, I think the bug I'm concerned about would still be fixed if we always had get_metadata_version infer the version from the keys present.

Closes #1381

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details
pganssle added a commit to pganssle/setuptools that referenced this pull request Nov 5, 2018
@pganssle pganssle requested review from jaraco and benoit-pierre Nov 5, 2018
@pganssle pganssle added this to Submitted PRs in PyPA Sprint Weekend at Bloomberg (2018) via automation Nov 5, 2018
@pganssle
Copy link
Member Author

@pganssle pganssle commented Nov 5, 2018

We may want to pull in the unit tests from distutils for read_pkg_file and upload_file, but in the case of upload_file the command is deprecated anyway, and for read_pkg_file, as long as all the lines are covered I'm OK with waiting until we pull distutils in its entirety into setuptools.

@pganssle pganssle force-pushed the pganssle:fix_upload_metadata branch from 8831858 to db8cdc6 Nov 5, 2018
pganssle added a commit to pganssle/setuptools that referenced this pull request Nov 5, 2018
@pganssle pganssle force-pushed the pganssle:fix_upload_metadata branch from 6160ab7 to ae881ef Nov 6, 2018
pganssle added a commit to pganssle/setuptools that referenced this pull request Nov 6, 2018
@pganssle pganssle force-pushed the pganssle:fix_upload_metadata branch from ae881ef to caa75d3 Nov 6, 2018
pganssle added a commit to pganssle/setuptools that referenced this pull request Nov 6, 2018
@pganssle pganssle force-pushed the pganssle:fix_upload_metadata branch from caa75d3 to 1bff011 Nov 6, 2018
pganssle added a commit to pganssle/setuptools that referenced this pull request Nov 6, 2018
@pganssle
Copy link
Member Author

@pganssle pganssle commented Nov 6, 2018

@jaraco @benoit-pierre This is mostly ready for review. I think I may need a few more tests, I didn't realize that we had the diff coverage required configured to be 81% or something and not 100%. That feels like a mistake, so I'll probably add a few more tests to get the diff coverage to 100% (or at least closer to it), but if y'all can give some feedback on what I have so far I'd appreciate it.

The biggest change (which is why I don't feel comfortable just unilaterally merging this) is that we're now monkey-patching more of distutils. We can avoid that in this issue, but I think probably it will be hard to avoid if we also want to solve things like #1578.

@pganssle pganssle force-pushed the pganssle:fix_upload_metadata branch from 1bff011 to c39491d Nov 7, 2018
pganssle added a commit to pganssle/setuptools that referenced this pull request Nov 7, 2018
@pganssle pganssle force-pushed the pganssle:fix_upload_metadata branch from 7e86b7c to c6d5d8e Nov 7, 2018
pganssle added a commit to pganssle/setuptools that referenced this pull request Nov 7, 2018
@pganssle
Copy link
Member Author

@pganssle pganssle commented Nov 7, 2018

OK, so lesson learned about not having 100% diff coverage in a situation like this. Almost everything that I originally thought "maybe I should throw that under a #pragma: nocover rather than write a test for it" ended up having a bug in it that I needed to fix.

I think I have 100% coverage on the diff now, though (codecov is being super weird about displaying diff coverage, but when I manually run tox -e py27,py37 -- --cov I don't think I'm missing anything.

pganssle added 11 commits Oct 27, 2018
This is the baseline, unchanged from the version in distutils.dist, to
be modified before patching.
This turns get_metadata_version into a method on DistributionMetadata,
populated either by inferrence (in the case of package metadata
specified in `setup`) or from the data in a specified PKG-INFO file.

To populate metadata_version from PKG-INFO, we need to monkey patch
read_pkg_file in addition to write_pkg_file.
Previously this value was hard-coded to '1.0', which was inaccurate for
many packages.

Fixes #1381
This creates a wrapper function for writing fields in the PKG-INFO file,
both to simplify the syntax and to add a point where we can inject an
encoding function in order to support Python 2.7 compatibility.
Rather than writing to a file in a temporary directory, we can write to
and read from an in-memory buffer, now that the encoding functionality
in write_pkg_file is fixed.
This is a fixture to create an upload command with a patched version of
urlopen so that no HTTP queries are sent.
pganssle added 7 commits Nov 7, 2018
`test_upload_metadata` was written before the fixture, so this updates
the test to use the fixture.
This fixes an issue where `distutils.spawn.spawn` was not available in
the ported upload_file, which is only used when signing the data.

This also adds a test that the gpg signature command is invoked and
included in the uploaded data.
This fixes uploads when bdist_rpm or bdist_dumb are the command, both of
which insert a comment about what platform they are built for.
The `upload.show_response` feature was not added until Python 3. Rather
than backport it, it is now enabled only if supported.

This also adds a "smoke test" for the feature.
@pganssle pganssle force-pushed the pganssle:fix_upload_metadata branch from c6d5d8e to fe2c9e4 Nov 7, 2018
@pganssle
Copy link
Member Author

@pganssle pganssle commented Nov 10, 2018

I'd like to get this one in for the next release, but I'm OK with pushing it off to the release after that.

I'd like to cut a release soon, so @jaraco @benoit-pierre if you have no objections to merging this let me know preferably in the next few days.

@jaraco
jaraco approved these changes Nov 11, 2018
Copy link
Member

@jaraco jaraco left a comment

I'm okay with this. The approach seems sound. I'm a little uneasy about improving support for a deprecated command, but since you've put in the effort, I see no reason not to roll it out.

PyPA Sprint Weekend at Bloomberg (2018) automation moved this from Submitted PRs to Approved PRs Nov 11, 2018
@pganssle
Copy link
Member Author

@pganssle pganssle commented Nov 11, 2018

@jaraco To clarify, the reason why I wanted to improve the deprecated command is because it's currently polluting PyPI with bad metadata, and I think that when upload starts to raise an error, a bunch of people will just start pinning to the last version of setuptools that "works" rather than switching over to twine. I want the version they pin to to be uploading good metadata. This is, to me, the next step in making setup.py upload raise an exception.

@pganssle pganssle mentioned this pull request Nov 11, 2018
This comment is not used anywhere and `platform.dist()` is deprecated.

See CPython PR #10414: python/cpython#10414
and bpo-35186: https://bugs.python.org/issue35186
@pganssle
Copy link
Member Author

@pganssle pganssle commented Nov 12, 2018

I've also ported python/cpython#10414 in to this branch, because platform.dist() is deprecated and this comment is unnecessary anyway.

@pganssle pganssle merged commit 375138c into pypa:master Nov 12, 2018
5 checks passed
5 checks passed
@codecov
codecov/patch 99.62% of diff hit (target 81.77%)
Details
@codecov
codecov/project 82.17% (+0.39%) compared to b0c7466
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
PyPA Sprint Weekend at Bloomberg (2018) automation moved this from Approved PRs to Merged PRs Nov 12, 2018
@sharov sharov mentioned this pull request Nov 13, 2018
0 of 2 tasks complete
@pganssle pganssle deleted the pganssle:fix_upload_metadata branch Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

3 participants