fix: add explicit data attribute to compile_pip_requirements and forward it to the generated py_binary#3865
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds an explicit data attribute to the pip_compile macro and forwards it to the generated py_binary. The review feedback suggests defensively normalizing the data parameter to an empty list if it is falsy or None to prevent potential TypeError crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
35041bf to
71ed30d
Compare
| - "bazel query 'labels(data, //:requirements.update)' | grep -Fx '//:requirements.in'" | ||
| - "bazel query 'labels(data, //:requirements.update)' | grep -Fx '//:requirements_extra.in'" |
There was a problem hiding this comment.
Could we use genquery for this target and not have the integration test at all?
There was a problem hiding this comment.
Yep, that would be better.
Also, genquery revealed another issue: the data attribute does not allow duplicate labels. In the test BUILD file we have this:
compile_pip_requirements(
name = "os_specific_requirements",
src = "requirements_os_specific.in",
data = [
"requirements_extra.in",
"requirements_os_specific.in",
],
requirements_darwin = "requirements_lock_darwin.txt",
requirements_linux = "requirements_lock_linux.txt",
requirements_txt = "requirements_lock.txt",
requirements_windows = "requirements_lock_windows.txt",
)
After forwarding the user-provided data directly to the generated py_binary, requirements_os_specific.in appears twice: once from src and once from data:
ERROR: /.../rules_python/tests/integration/compile_pip_requirements/BUILD.bazel:69:25:
Label '//:requirements_os_specific.in' is duplicated in the 'data' attribute of rule 'os_specific_requirements.update'
I am not sure whether we should deduplicate these labels in the macro, or treat this as a user error and let Bazel report it. Currently, I lean toward deduplicating them in the macro.
| ) | ||
|
|
||
| data = [name, requirements_txt] + srcs + [f for f in (requirements_linux, requirements_darwin, requirements_windows) if f != None] + constraints | ||
| data = data + [name, requirements_txt] + srcs + [f for f in (requirements_linux, requirements_darwin, requirements_windows) if f != None] + constraints |
There was a problem hiding this comment.
So to better understand how this was not working previously was because we were including the data via the filegroup target via [name, ...] and not directly via files via data?
Out of curiosity, would the usecase have worked if the parameters did not use $(location)?. I would have thought that the files would have been included but the $(location) function in the args would not have worked correctly.
If my analysis is correct, please update the news file to hint about why thee fix was done.
There was a problem hiding this comment.
So to better understand how this was not working previously was because we were including the data via the filegroup target via [name, ...] and not directly via files via data?
Yep, you're right.
Btw, do we need name in the generated py_binary data at all? As I understand it, name is the default target used by other compile_pip_requirements rules. Since requirements_txt is already added separately, maybe it is enough to pass data and requirements_txt directly. I may be missing another reason for keeping name there though.
Out of curiosity, would the usecase have worked if the parameters did not use
$(location)?. I would have thought that the files would have been included but the$(location)function in the args would not have worked correctly.
Yes, that matches my understanding. Without $(location), the files could still be available at runtime through the generated filegroup. The issue is that $(location :pip_cert) needs :pip_cert to be passed directly to the generated py_binary.
I'll update the news entry.
71ed30d to
6709e72
Compare
…forward it to the generated `py_binary`
6709e72 to
9b8b29b
Compare
|
Could you please take a look at the build failures? |
…tam/rules_python into forward-data-to-compile-req-binary
As stated in #3858, the
compile_pip_requirementsmacro accepts adataattribute, but this attribute is not forwarded to the internalpy_binarytarget generated for the.updatetarget.As a result, using
$(location :pip_cert)insideextra_argsfails during analysis because the generatedpy_binarydoes not declare:pip_certas a prerequisite.Before:
After:
Closes #3858