Skip to content

fix(whl_library): actually apply patches and warn if RECORD file patch is needed#1637

Merged
aignas merged 2 commits intobazel-contrib:mainfrom
aignas:fix/fix-patch-application
Dec 21, 2023
Merged

fix(whl_library): actually apply patches and warn if RECORD file patch is needed#1637
aignas merged 2 commits intobazel-contrib:mainfrom
aignas:fix/fix-patch-application

Conversation

@aignas
Copy link
Copy Markdown
Collaborator

@aignas aignas commented Dec 20, 2023 •
edited
Loading

Before this PR there was a typo, that was actually causing the
patching function to not use the provided patches. With this change
we are finally correctly doing it. After fixing this bug I noticed
that repository_ctx.patch results in files with CRLF on Windows,
which made me make the RECORD mismatch to be a warning rather than
a hard failure to make the CI happy and allow users on Windows to
patch wheels but see a warning if they have a multi-platform bazel
setup.

The CRLF endings on Windows issue is fixed in
bazelbuild/bazel@07e0d31

Related #1631, #1639.

@aignas
fix(whl_library): actually apply patches if specified
2b9c686 
Before this PR there was a typo, that was actually causing the
patching function to not use the provided patches. With this change
we are finally corrcetly doing it.
@aignas aignas requested a review from rickeylev as a code owner December 20, 2023 01:05
@aignas
Copy link
Copy Markdown
Collaborator Author

aignas commented Dec 20, 2023

It seems that the tests started failing on windows, because bazel's rctx.patch has different behaviour on Windows as it is replacing the line endings with CRLF as opposed to leaving them LF.

I could reproduce this by changing the repackaging code with:

--- a/python/private/repack_whl.py
+++ b/python/private/repack_whl.py
@@ -148,6 +148,8 @@ def main(sys_argv):

         with _WhlFile(args.output, mode="w", distinfo_dir=distinfo_dir) as out:
             for p in _files_to_pack(patched_wheel_dir, record_contents):
+                if p.name == "METADATA":
+                    p.write_text(p.read_text().replace("\n", "\r\n"))
                 rel_path = p.relative_to(patched_wheel_dir)
                 out.add_file(str(rel_path), p)

And the test started failing with:

repack_whl: ERROR: Please also patch the RECORD file with:
--- a/requests-2.25.1.dist-info/RECORD
+++ b/requests-2.25.1.dist-info/RECORD
@@ -17,7 +17,7 @@
 requests/structures.py,sha256=msAtr9mq1JxHd-JRyiILfdFlpbJwvvFuP3rfUQT_QxE,3005
 requests/utils.py,sha256=_K9AgkN6efPe-a-zgZurXzds5PBC0CzDkyjAE2oCQFQ,30529
 requests-2.25.1.dist-info/LICENSE,sha256=CeipvOyAZxBGUsFoaFqwkx54aPnIKEtm9a5u2uXxEws,10142
-requests-2.25.1.dist-info/METADATA,sha256=fRSAA0u0Bi0heD4zYq91wdNUTJlbzhK6_iDOcRRNDx4,4177
+requests-2.25.1.dist-info/METADATA,sha256=FIRoD6JzkS0gW2tgRJemIR-Dj9FVZPLY7ALIWbib-bg,4280
 requests-2.25.1.dist-info/WHEEL,sha256=Z-nyYpwrcSqxfdux5Mbn_DQ525iP7J2DG3JgGvOYyTQ,110
 requests-2.25.1.dist-info/top_level.txt,sha256=fMSVmHfb5rbGOo6xv-O_tUX6j-WyixssE-SnwcDRxNQ,9
 requests-2.25.1.dist-info/RECORD,,

which is exactly the same suggested sha value as on failing Windows tests.

@aignas aignas mentioned this pull request Dec 20, 2023
Closed
@aignas aignas changed the title fix(whl_library): actually apply patches if specified fix(whl_library): actually apply patches and warn if RECORD file patch is needed Dec 20, 2023
@aignas
Copy link
Copy Markdown
Collaborator Author

aignas commented Dec 20, 2023

@rickeylev, feel free to merge if you agree with the warning refactor.

rickeylev
rickeylev approved these changes Dec 21, 2023
View reviewed changes
@rickeylev rickeylev added this pull request to the merge queue Dec 21, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 21, 2023
@aignas aignas added this pull request to the merge queue Dec 21, 2023
Merged via the queue into bazel-contrib:main with commit 10150e5 Dec 21, 2023
@aignas aignas mentioned this pull request Mar 4, 2024
Merged
@aignas aignas deleted the fix/fix-patch-application branch May 13, 2024 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rickeylev rickeylev rickeylev approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aignas @rickeylev