Skip to content

Conversation

kcon-stackav
Copy link
Contributor

@kcon-stackav kcon-stackav commented Oct 2, 2024

The RECORD file written when wheels are patched using pip.override() is not quoting filenames as needed, so wheels that (unfortunately) include files whose name contain commas would be written in such a way that https://pypi.org/project/installer/ would fail to parse them, resulting in an error like:

installer.records.InvalidRecordEntry: Row Index 360: expected 3 elements, got 5

This PR fixes that by using csv to read and write RECORD file entries which takes care of quoting elements of record entries as needed. See PEP376 for more info about the RECORD file format here: https://peps.python.org/pep-0376/#record

Fixes #2261

Copy link

google-cla bot commented Oct 2, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@aignas
Copy link
Collaborator

aignas commented Oct 3, 2024

The change in general LGTM, but the CI failure is a little weird. Don't have to look into this in-depth now though.

@kcon-stackav kcon-stackav force-pushed the kevin/fix-pip-override-for-commas-in-filenames branch 2 times, most recently from f3642a7 to 1717016 Compare October 4, 2024 00:33
@kcon-stackav kcon-stackav requested a review from groodt as a code owner October 4, 2024 00:33
@kcon-stackav kcon-stackav force-pushed the kevin/fix-pip-override-for-commas-in-filenames branch from 1717016 to 1c77342 Compare October 4, 2024 00:37
@kcon-stackav kcon-stackav changed the title fix(bzlmod): Quote filenames in patched wheel RECORD files fix(bzlmod): Quote wheel RECORD file entry elements if needed Oct 4, 2024
@aignas
Copy link
Collaborator

aignas commented Oct 7, 2024

Could you please add a unit test in the examples/wheel folder to see that it really works and add a note to the CHANGELOG.md file that this got fixed?

I am not sure this is fix(bzlmod) as it fixes the wheel building for WORKSPACE and bzlmod, so just having a fix(py_wheel) would be good enough, I think.

@kcon-stackav kcon-stackav force-pushed the kevin/fix-pip-override-for-commas-in-filenames branch from 1c77342 to dd5112c Compare October 7, 2024 18:41
@kcon-stackav kcon-stackav changed the title fix(bzlmod): Quote wheel RECORD file entry elements if needed fix(py_wheel): Quote wheel RECORD file entry elements if needed Oct 7, 2024
@kcon-stackav kcon-stackav force-pushed the kevin/fix-pip-override-for-commas-in-filenames branch from dd5112c to 5bfa599 Compare October 7, 2024 18:57
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for adding the tests.

@aignas aignas added this pull request to the merge queue Oct 8, 2024
Merged via the queue into bazel-contrib:main with commit a2773de Oct 8, 2024
4 checks passed
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.

Using pip.override() to patch a wheel that includes a file with a name containing commas fails

2 participants