Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symbolic macros using attr.string_list #25257

Open
fishy opened this issue Feb 11, 2025 · 6 comments
Open

Symbolic macros using attr.string_list #25257

fishy opened this issue Feb 11, 2025 · 6 comments
Labels
team-Documentation Documentation improvements that cannot be directly linked to other team labels team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob type: documentation (cleanup) untriaged

Comments

@fishy
Copy link

fishy commented Feb 11, 2025

Page link:

https://bazel.build/rules/lib/toplevel/attr

Problem description (include actual vs expected text, if applicable):

I'm trying to convert a legacy macro to symbolic macro, but have trouble using attr.string_list.

We have oci_push rules for pushing dev and prod oci images, defined as targets like //serviceA:docker_dev, //serviceA:docker_prod, etc., and a macro using multirun to push batches of images in CI systems. the macro is defined like this:

load("@rules_multirun//:defs.bzl", "multirun")

_rule_prefixes = [
    "//serviceA:docker_",
    ...,
]

def multirun_rule(name, visibility, suffixes):
    """This macro generates multirun rules.

    Args:
      name: The name of the rule.
      visibility: The visibility of the rule.
      suffixes: Suffixes of the combination of "dev" and "prod".
    """
    commands = []
    for prefix in _rule_prefixes:
        for suffix in suffixes:
            commands.append(prefix + suffix)
    multirun(
        name = name,
        commands = commands,
        jobs = 0,  # Set to 0 to run in parallel, defaults to sequential
        visibility = visibility,
    )

and then in top-level BUILD.bazel we just define 3 rules to be used in different CI pipelines:

multirun_rule(
    name = "docker_dev",
    suffixes = ["dev"],
    visibility = ["//visibility:public"],
)

multirun_rule(
    name = "docker_prod",
    suffixes = ["prod"],
    visibility = ["//visibility:public"],
)

multirun_rule(
    name = "docker_all",
    suffixes = [
        "dev",
        "prod",
    ],
    visibility = ["//visibility:public"],
)

in order to convert it to symbolic macro, I just renamed multirun_rule to _multirun_rule_impl, then added:

multirun_rule = macro(
    attrs = {
        "suffixes": attr.string_list(mandatory = True, doc = 'Suffixes of the combination of "dev" and "prod"'),
    },
    implementation = _multirun_rule_impl,
)

but this causes it to complain about:

                for suffix in suffixes:
Error: type 'select' is not iterable

Where do you see this issue? (include link to specific section of the page, if applicable)

the doc for attr.string_list doesn't really explain how to access the actual string list. For how to use them it points to https://bazel.build/extending/rules#implementation_function, which only explains how to use them in rules, but not how to use them in symbolic macros (in rules they were accessed via ctx, which is not available in symbolic macros)

Any other information you'd like to share?

No response

@fishy fishy added team-Documentation Documentation improvements that cannot be directly linked to other team labels type: documentation (cleanup) untriaged labels Feb 11, 2025
@fmeum
Copy link
Collaborator

fmeum commented Feb 11, 2025

You could set configurable = False on the attribute to get a list you can iterate in the macro, see https://bazel.build/extending/macros#attributes.

@fishy
Copy link
Author

fishy commented Feb 11, 2025

Thanks, @fmeum . That can workaround this particular case, but I still think there can be better docs to help people using attr.string_list in symbolic macros properly.

@satyanandak satyanandak added the team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob label Feb 12, 2025
@meisterT
Copy link
Member

cc @susinmotion @tetromino

@tetromino
Copy link
Contributor

tetromino commented Feb 12, 2025

The docs are there! Please see https://bazel.build/extending/macros#selects:

If an attribute is configurable (the default) and its value is not None, then the macro implementation function will see the attribute value as wrapped in a trivial select. This makes it easier for the macro author to catch bugs where they did not anticipate that the attribute value could be a select.
[...]
If my_macro is invoked with deps = ["//a"], that will cause _my_macro_impl to be invoked with its deps parameter set to select({"//conditions:default": ["//a"]}). If this causes the implementation function to fail (say, because the code tried to index into the value as in deps[0], which is not allowed for selects), the macro author can then make a choice: either they can rewrite their macro to only use operations compatible with select, or they can mark the attribute as nonconfigurable (attr.label_list(configurable = False)). The latter ensures that users are not permitted to pass a select value in.

@fishy - is there a wording change or additional explanation that you would suggest we add?

@fishy
Copy link
Author

fishy commented Feb 12, 2025

@tetromino Thanks for the link! I think the current doc answers my original question in this issue. So feel free to close this issue.

On a side note, we just noticed that for attr.string there are some kinda inconsistency depending on how it is used. Say foo is an arg defined via attr.string and passed in value of "bar". "foo/%s" % (foo) in symbolic macro implementation would generate a string like foo/select({"//conditions:default": "bar"}), but "foo/" + foo would generate foo/bar. Is that expected?

@tetromino
Copy link
Contributor

tetromino commented Feb 12, 2025

On a side note, we just noticed that for attr.string there are some kinda inconsistency depending on how it is used. Say foo is an arg defined via attr.string and passed in value of "bar". "foo/%s" % (foo) in symbolic macro implementation would generate a string like foo/select({"//conditions:default": "bar"}), but "foo/" + foo would generate foo/bar. Is that expected?

@fishy - this is unfortunately expected because + has special magical powers (laziness when one of its sides is a select value), and % doesn't, but it's obviously very confusing. Fixing this behavior would be a bit tricky (and would be an incompatible change). Please file an issue about it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Documentation Documentation improvements that cannot be directly linked to other team labels team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob type: documentation (cleanup) untriaged
Projects
None yet
Development

No branches or pull requests

7 participants