Skip to content

fix: add explicit data attribute to compile_pip_requirements and forward it to the generated py_binary#3865

Open
granatam wants to merge 7 commits into
bazel-contrib:mainfrom
granatam:forward-data-to-compile-req-binary
Open

fix: add explicit data attribute to compile_pip_requirements and forward it to the generated py_binary#3865
granatam wants to merge 7 commits into
bazel-contrib:mainfrom
granatam:forward-data-to-compile-req-binary

Conversation

@granatam

Copy link
Copy Markdown

As stated in #3858, the compile_pip_requirements macro accepts a data attribute, but this attribute is not forwarded to the internal py_binary target generated for the .update target.

As a result, using $(location :pip_cert) inside extra_args fails during analysis because the generated py_binary does not declare :pip_cert as a prerequisite.

Before:

ERROR: /.../BUILD.bazel:8:25: in args attribute of py_binary rule //:requirements.update: label '//:pip_cert' in $(location) expression is not a declared prerequisite of this rule. Since this rule was created by the macro 'pip_compile', the error might have been caused by the macro implementation
ERROR: /.../BUILD.bazel:8:25: Analysis of target '//:requirements.update' (config: 7f8856b) failed
ERROR: Analysis of target '//:requirements.update' failed; build aborted

After:

INFO: Analyzed target //:requirements.update (87 packages loaded, 4078 targets configured).
INFO: Found 1 target...
Target //:requirements.update up-to-date:
  bazel-bin/requirements.update
INFO: Elapsed time: 6.240s, Critical Path: 0.11s
INFO: 1 process: 11 action cache hit, 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Running command line: bazel-bin/requirements.update '--src=rules_python_pip_parse_example/requirements.in' rules_python_pip_parse_example/requirements_lock.txt //:requirements '--resolver=backtracking' --allow-unsafe --generate-hashes '--cert=./pip.cert'

Closes #3858

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread python/private/pypi/pip_compile.bzl Outdated
@granatam granatam force-pushed the forward-data-to-compile-req-binary branch from 35041bf to 71ed30d Compare June 30, 2026 19:19
@granatam granatam marked this pull request as ready for review June 30, 2026 19:30
Comment thread .bazelci/presubmit.yml Outdated
Comment on lines +570 to +571
- "bazel query 'labels(data, //:requirements.update)' | grep -Fx '//:requirements.in'"
- "bazel query 'labels(data, //:requirements.update)' | grep -Fx '//:requirements_extra.in'"

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.

Could we use genquery for this target and not have the integration test at all?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Comment thread python/private/pypi/pip_compile.bzl Outdated
)

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

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.

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.

@granatam granatam Jul 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@granatam granatam force-pushed the forward-data-to-compile-req-binary branch from 71ed30d to 6709e72 Compare July 5, 2026 17:22
@granatam granatam force-pushed the forward-data-to-compile-req-binary branch from 6709e72 to 9b8b29b Compare July 5, 2026 17:26
@granatam granatam requested a review from aignas July 5, 2026 17:26
@aignas

aignas commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Could you please take a look at the build failures?

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.

compile_pip_requirements does not forward data attribute to the generated py_binary

2 participants