-
Notifications
You must be signed in to change notification settings - Fork 88
fix: stabilize RSS feed GUIDs for multi-arch wheel publication #1382
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
Open
ryanpetrello
wants to merge
1
commit into
pulp:main
Choose a base branch
from
ryanpetrello:fix/rss-guid-stability
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+166
−8
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| RSS feed GUIDs are now stable when additional platform wheels arrive for the same release. GUIDs only change when a new PEP 427 build tag is introduced, preventing feed readers from showing duplicate entries for multi-architecture builds. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| from email.utils import getaddresses | ||
| from urllib.parse import urljoin | ||
|
|
||
| from django.contrib.postgres.aggregates import ArrayAgg | ||
| from django.db.models import F, FilteredRelation, Max, Min, Q | ||
| from django.http.response import HttpResponse, HttpResponseNotFound | ||
| from django.utils.decorators import method_decorator | ||
|
|
@@ -15,6 +16,8 @@ | |
| from pulp_python.app.cache import PythonApiCache, find_base_path_cached | ||
| from pulp_python.app.pypi.views import PyPIMixin, _etag_func | ||
|
|
||
| _WHEEL_BUILD_TAG_RE = re.compile(r"^.+?-.+?-(?P<build>\d[^-]*?)-[^-]+-[^-]+-[^-]+\.whl$") | ||
|
|
||
| UPDATES_LIMIT = 500 | ||
| PACKAGES_LIMIT = 40 | ||
| PROJECT_RELEASES_LIMIT = 40 | ||
|
|
@@ -70,6 +73,7 @@ def iter_releases(content, repo_ver, name_normalized=None, limit=UPDATES_LIMIT): | |
| name=Min("name"), | ||
| summary=Min("summary"), | ||
| author_email=Min("author_email"), | ||
| filenames=ArrayAgg("filename", distinct=True, ordering="filename"), | ||
| ) | ||
| .order_by("-added_at", "name_normalized", "version")[:limit] | ||
| ) | ||
|
|
@@ -91,14 +95,30 @@ def iter_projects(content, repo_ver, limit=PACKAGES_LIMIT): | |
| ) | ||
|
|
||
|
|
||
| def _item_dict(title, link, description, author_email, pubdate): | ||
| def _build_tag_fragment(filenames): | ||
| """Extract sorted distinct build tags from wheel filenames for GUID stability. | ||
|
|
||
| Returns a fragment like ``#builds=1,2`` when build tags are present, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use single ticks. |
||
| or an empty string for sdists and wheels without build tags. | ||
| """ | ||
| tags = set() | ||
| for fn in filenames or (): | ||
| m = _WHEEL_BUILD_TAG_RE.match(fn) | ||
| if m: | ||
| tags.add(m.group("build")) | ||
| if not tags: | ||
| return "" | ||
| return "#builds=" + ",".join(sorted(tags)) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
|
|
||
| def _item_dict(title, link, description, author_email, pubdate, filenames=()): | ||
| return { | ||
| "title": sanitize_xml_text(title), | ||
| "link": link, | ||
| "description": sanitize_xml_text(description), | ||
| "author_email": format_author(author_email), | ||
| "pubdate": pubdate, | ||
| "unique_id": f"{link}#{pubdate.isoformat()}", | ||
| "unique_id": f"{link}{_build_tag_fragment(filenames)}", | ||
| } | ||
|
|
||
|
|
||
|
|
@@ -139,6 +159,7 @@ def render_updates_feed(index_url, releases): | |
| description=release["summary"], | ||
| author_email=release["author_email"], | ||
| pubdate=release["added_at"], | ||
| filenames=release.get("filenames", ()), | ||
| ) | ||
| for release in releases | ||
| ] | ||
|
|
@@ -178,6 +199,7 @@ def render_project_releases_feed(index_url, project_name, releases): | |
| description=release["summary"], | ||
| author_email=release["author_email"], | ||
| pubdate=release["added_at"], | ||
| filenames=release.get("filenames", ()), | ||
| ) | ||
| for release in releases | ||
| ] | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| import re | ||
|
|
||
| import pytest | ||
|
|
||
| # Duplicated here to avoid importing feeds.py, which pulls in Django/DRF and | ||
| # requires a configured Django settings module that the unit test runner lacks. | ||
|
Comment on lines
+5
to
+6
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you add pytest-django to unittest_requirements.txt then you could import this constant with no issue. |
||
| _WHEEL_BUILD_TAG_RE = re.compile(r"^.+?-.+?-(?P<build>\d[^-]*?)-[^-]+-[^-]+-[^-]+\.whl$") | ||
|
|
||
|
|
||
| def _build_tag_fragment(filenames): | ||
| tags = set() | ||
| for fn in filenames or (): | ||
| m = _WHEEL_BUILD_TAG_RE.match(fn) | ||
| if m: | ||
| tags.add(m.group("build")) | ||
| if not tags: | ||
| return "" | ||
| return "#builds=" + ",".join(sorted(tags)) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "filenames, expected", | ||
| [ | ||
| ([], ""), | ||
| (["shelf-reader-0.1.tar.gz"], ""), | ||
| (["shelf_reader-0.1-py2-none-any.whl"], ""), | ||
| ( | ||
| ["docling_parse-7.19.1-1-cp312-cp312-linux_x86_64.whl"], | ||
| "#builds=1", | ||
| ), | ||
| ( | ||
| [ | ||
| "docling_parse-7.19.1-1-cp312-cp312-linux_x86_64.whl", | ||
| "docling_parse-7.19.1-1-cp312-cp312-linux_aarch64.whl", | ||
| "docling_parse-7.19.1-1-cp312-cp312-linux_ppc64le.whl", | ||
| ], | ||
| "#builds=1", | ||
| ), | ||
| ( | ||
| [ | ||
| "ctranslate2-4.5.0-1-cp312-cp312-linux_x86_64.whl", | ||
| "ctranslate2-4.5.0-2-cp312-cp312-linux_x86_64.whl", | ||
| ], | ||
| "#builds=1,2", | ||
| ), | ||
| ( | ||
| [ | ||
| "foo-1.0-1-cp312-cp312-linux_x86_64.whl", | ||
| "foo-1.0.tar.gz", | ||
| ], | ||
| "#builds=1", | ||
| ), | ||
| (None, ""), | ||
| ], | ||
| ids=[ | ||
| "empty", | ||
| "sdist-only", | ||
| "wheel-no-build-tag", | ||
| "single-build-tag", | ||
| "same-build-tag-multi-arch", | ||
| "two-build-tags", | ||
| "mixed-sdist-and-tagged-wheel", | ||
| "none", | ||
| ], | ||
| ) | ||
| def test_build_tag_fragment(filenames, expected): | ||
| assert _build_tag_fragment(filenames) == expected | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this feature is one you requested, but I wonder if we are deviating from PyPI's behavior. Can you show me an example of a package on PyPI that updates their entries based on build-tags?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a pretty good point 🤔 - we may be coloring this RSS implementation with our desired behavior, and not with parity in terms of what PyPI actually does.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, @mprpic, we may actually be at an impasse here. Looking at what PyPI actually exposes in its RSS feed, it doesn't provide a
<guid>element, which means it just falls back to the<link>.PyPI's link is https://pypi.org/project/{name}/{version}/ -- one entry per (name, version), no timestamp, no build tag.
So if we're modeling this after the way that PyPI behaves, the link is the GUID; adding more wheels to an existing release doesn't create a new RSS entry.
So even though the original limitation you discovered doesn't work the way we expected it to, it matches what PyPI does (and I understand why Pulp maintainers would want to mirror the PyPI implementation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at an example it seems that PyPI doesn't even use GUID: https://pypi.org/rss/project/tensorflow/releases.xml
On the other hand maybe this is a limitation of PyPI and their RSS feeds would be more useful with this.