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

Add support for yanked releases and files (PEP-592) #5841

Merged
merged 6 commits into from
Aug 21, 2022

Conversation

radoering
Copy link
Member

Pull Request Check List

Resolves: #2453

Requires a poetry-core version with python-poetry/poetry-core#400

  • Added tests for changed code.
  • Updated documentation for changed code.

@radoering radoering marked this pull request as draft June 13, 2022 16:56
@radoering radoering mentioned this pull request Jun 14, 2022
@neersighted neersighted force-pushed the yanked-release-pep592 branch from e911136 to 953810a Compare June 19, 2022 22:20
@dimbleby
Copy link
Contributor

A recent example of the desirability of understanding the yanked flag: the most recent version of cryptography that is currently available is 37.0.3 - and it is yanked "due to a regression in OpenSSL that was producing heap corruption".

cryptography will appear in a lot of dependency trees, as of right now poetry will be locking to that yanked version.

@radoering radoering force-pushed the yanked-release-pep592 branch from 953810a to 6e0fe2c Compare July 13, 2022 04:29
@radoering radoering marked this pull request as ready for review July 13, 2022 04:38
@radoering radoering force-pushed the yanked-release-pep592 branch 2 times, most recently from 21b53d6 to e5fc3d9 Compare July 13, 2022 15:12
@radoering radoering force-pushed the yanked-release-pep592 branch from e5fc3d9 to fe40a74 Compare July 31, 2022 16:04
@radoering radoering requested a review from a team July 31, 2022 20:15
@radoering radoering force-pushed the yanked-release-pep592 branch 3 times, most recently from bee321e to 602bb1b Compare August 17, 2022 17:36
@radoering radoering requested a review from a team August 17, 2022 20:23
@radoering radoering force-pushed the yanked-release-pep592 branch from 602bb1b to 404be7e Compare August 21, 2022 07:45
Copy link
Member

@mkniewallner mkniewallner left a comment

Choose a reason for hiding this comment

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

Tested locally against PyPI repository (didn't test other repositories), it works as expected.

As possible follow-ups improvements (in future PRs):

  • Maybe we could reflect the "yanked" information when using poetry show?
  • Would a strict flag on the different commands (install, lock --check, etc.) or a global configuration that prevents installation/produces failure whenever a yanked dependency is used be something that users want? The warning is clearly noticeable locally, but on a CI, it may be suitable to ensure that no yanked dependency is used when installing dependencies, or when checking that the lock file is up-to-date.

src/poetry/repositories/link_sources/html.py Outdated Show resolved Hide resolved
src/poetry/repositories/link_sources/base.py Outdated Show resolved Hide resolved
Comment on lines +147 to +150
# PEP 592: PyPI always yanks entire releases, not individual files,
# so we just have to look for the first file
yanked = self._get_yanked(release[0])
packages.append(Package(info["info"]["name"], version, yanked=yanked))
Copy link
Member

Choose a reason for hiding this comment

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

Clearly outside the scope of this PR, but I wonder if PyPI lets you upload a new file on a yanked release, and if so, if this sets yanked / yanked_reason attributes accordingly.

It's a bit sad that we have to assume that a random file that is yanked means that the entire release is yanked, just because of a limitation from the structure of the API, especially since the releases don't seem to be ordered by upload_time:

$ curl -s https://pypi.org/pypi/cryptography/json | jq '.releases["37.0.3"][].upload_time'
"2022-06-21T19:07:43"
"2022-06-21T19:07:48"
"2022-06-21T19:08:26"
"2022-06-21T19:09:32"
"2022-06-21T19:09:38"
"2022-06-21T19:08:32"
"2022-06-21T19:08:03"
"2022-06-21T19:09:44"
"2022-06-21T19:08:55"
"2022-06-21T19:08:59"
"2022-06-21T19:09:04"
"2022-06-21T19:08:40"
"2022-06-21T19:08:10"
"2022-06-21T19:07:52"
"2022-06-21T19:08:45"
"2022-06-21T19:08:15"
"2022-06-21T19:09:08"
"2022-06-21T19:07:57"
"2022-06-21T19:08:49"
"2022-06-21T19:08:20"
"2022-06-21T19:09:11"
"2022-06-21T19:07:26"

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. For now I'd just rely on the PEP which says:

In Warehouse, the user experience will be implemented in terms of yanking or unyanking an entire release, rather than as an operation on individual files, which will then be exposed via the API as individual files being yanked.

src/poetry/repositories/pypi_repository.py Outdated Show resolved Hide resolved
src/poetry/repositories/pypi_repository.py Outdated Show resolved Hide resolved
@radoering
Copy link
Member Author

As possible follow-ups improvements (in future PRs):

Both sound reasonable.

@radoering radoering force-pushed the yanked-release-pep592 branch from 404be7e to d60a56d Compare August 21, 2022 19:03
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
@radoering radoering deleted the yanked-release-pep592 branch November 24, 2024 12:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for PEP-592 (yanked releases)
3 participants