Skip to content

fix: sdist resulting in duplicate repo name when requirements line does not match - #2658

Closed
chrisirhc wants to merge 15 commits into
bazel-contrib:mainfrom
chrisirhc:chua/03-11-investigate-exp-index
Closed

fix: sdist resulting in duplicate repo name when requirements line does not match#2658
chrisirhc wants to merge 15 commits into
bazel-contrib:mainfrom
chrisirhc:chua/03-11-investigate-exp-index

Conversation

@chrisirhc

@chrisirhcchrisirhc commented Mar 11, 2025

Copy link
Copy Markdown
Contributor

Attempt to fix#2648

chrisirhc
chrisirhc commented Mar 11, 2025
View reviewed changes
Comment threadtests/pypi/extension/extension_tests.bzl
whl_config_setting(
filename = "simple-0.0.1-py3-none-any.whl",
version = "3.15",
target_platforms = ("cp315_linux_aarch64", "cp315_linux_arm", "cp315_linux_ppc", "cp315_linux_s390x", "cp315_linux_x86_64", "cp315_osx_aarch64", "cp315_osx_x86_64", "cp315_windows_x86_64"),

@chrisirhcchrisirhcMar 11, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm not sure why my changes introduced this change. My guess is that extension.bzl may have some handling for any.whl

@chrisirhc

chrisirhc commented Mar 13, 2025

Copy link
Copy Markdown
ContributorAuthor

One of the errors from the CI:

(14:29:58) ERROR: /private/var/tmp/_bazel_buildkite/c90e6e99c06413157d7a728d39ca2945/external/+pip+dev_pip/markupsafe/BUILD.bazel:6:12: Illegal ambiguous match on configurable attribute "actual" in @@+pip+dev_pip//markupsafe:pkg:
--
| @@+pip+dev_pip//_config:is_cp311_osx_x86_64
| @@+pip+dev_pip//_config:is_cp311_cp311_osx_universal2

@chrisirhcchrisirhc mentioned this pull request Mar 13, 2025
Closed
aignas
aignas reviewed Mar 29, 2025
View reviewed changes
Comment threadpython/private/pypi/parse_requirements.bzl
index_urls = index_urls.get(whl_name),
logger = logger,
)

@aignasaignasMar 29, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The idea of adding a common sdist feels like a hack. Maybe having a different algorithm/grouping would be better. Maybe having per sha grouping could be better.

@chrisirhcchrisirhcApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Took a stab at changing it to use the sha instead.

@chrisirhc
chrisirhcforce-pushed the chua/03-11-investigate-exp-index branch from 608355a to fa04ed5CompareApril 4, 2025 05:57
@aignasaignas mentioned this pull request May 24, 2025
Merged
@aignas

aignas commented May 28, 2025

Copy link
Copy Markdown
Collaborator

I have tried fixing this issue in a slightly different way by merging all of the sources and the WIP PR is in #2932, if the approach works, then I'll add an extra test for sdists specifically to cover the case discussed in this PR.

github-merge-queueBot pushed a commit that referenced this pull request Jun 2, 2025
@aignas
fix(pypi): correctly aggregate the requirements files (#2932)
948fcec
This implements the actual fix where we are aggregating the whls and
sdists
correctly from multiple different requirements lines.
Fixes#2648.
Closes#2658.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aignasaignasaignas left review comments
@rickeylevrickeylevAwaiting requested review from rickeylevrickeylev will be requested when the pull request is marked ready for reviewrickeylev is a code owner
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

experimental_index_url can only be used in one pip.parse(), subsequent calls fail

2 participants

@chrisirhc@aignas