commit e9a4e93ed71 changed the meaning of the patches parameter to patch() in tools/build_defs/repo/utils.bzl

[dws]

Description of the bug:

Prior to commit e9a4e93, patch() in tools/build_defs/repo/utils.bzl processed its patches parameter as follows:

    if patches == None and hasattr(ctx.attr, "patches"):
        patches = ctx.attr.patches
    if patches == None:
        patches = []

This idiom is consistent with how patch() processes patch_cmds and patch_cmds_win, and means that if a custom repository rule has supplied patches or patch_cmds or patch_cmds_win explicitly, then patch() will not look at the attributes of the repository rule.

commit e9a4e93 altered this logic to

    if patches == None:
        patches = []
    if hasattr(ctx.attr, "patches") and ctx.attr.patches:
        patches += ctx.attr.patches

With this new logic, patch() will now process the sum of any explicitly supplied patches, and any patches supplied to the repository rule.

This change in semantics broke a custom repository rule that called patch() multiple times, and which relied on being able to pass patches = [] to force patch() to ignore the repository rule's ctx.attr.patches.

I cannot see anything in the description of commit e9a4e93 to explain why it makes this breaking change. It appears to be completely orthogonal to adding support for remote_patches. Side note: if patch() is now to support a remote_patches attribute on the repository rules, why did it not add a corresponding remote_patches parameter to patch()?

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Write a custom repository rule that calls patch() with patches = [] and observe that it applies patches from the patches attribute of a call to that repository rule.

Which operating system are you running Bazel on?

Ubuntu 20.04

What is the output of bazel info release?

release 5.3.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

[dws]

@meteorcloudy FYI

[meteorcloudy]

@dws If a repository rule already has the patches attribute, it'll be confusing for users that those patches are not applied, right? Can you provide a simple reproduce case that reflects your use case?

[dws]

@dws If a repository rule already has the patches attribute, it'll be confusing for users that those patches are not applied, right? Can you provide a simple reproduce case that reflects your use case?

I can describe the general idea pretty simply. This custom repository rule has a number of additional attributes to permit specifying a variety of things that all need to be downloaded into a single repository. As a part of this, it has attributes base_package_patch_cmds and base_package_patch_cmds_win which need to be applied at a certain point in the processing. The rule has TWO calls to patch(). The first is

    patch(
        ctx,
        patches = [],
        patch_cmds = ctx.attr.base_package_patch_cmds,
        patch_cmds_win = ctx.attr.base_package_patch_cmds_win,
    )

and the second, done after the remaining artifacts are downloaded, is the more usual

    patch(ctx)

So when users of this repository rule supply patches, they do get processed only once, by the second call to patch().
The first call to patch() permits processing the special set of patch commands WITHOUT applying patches.
The changes in Bazel 5 broke the ability to choose whether to process the attributes from the rule, and as a result, the rule was trying to apply the patches TWICE under Bazel 5.

Note that the comment at the top of patch() says:

    This rule is intended to be used in the implementation function of
    a repository rule. If the parameters `patches`, `patch_tool`,
    `patch_args`, `patch_cmds` and `patch_cmds_win` are not specified
    then they are taken from `ctx.attr`.

which strongly suggests the Bazel 4 either-or behavior of the code: The routine will use EITHER the attribute from the repository rule, OR, the explicitly supplied attribute, but not both.

For this reason, I think the Bazel 4 behavior for the patches parameter should be restored.
By the same token, I'd argue that patch() should now have new parameters for remote_patches and remote_patch_strip in order to permit callers of patch() to choose whether to pay attention to the attributes from the repository rule. This would make its handling of these new parameters more consistent with its existing handling of other parameters.

[meteorcloudy]

@dws Thanks for the clarification! Now I understand the use case, sorry for the breakage, do you have time to compose a PR to fix this?