Need replacement for ctx.expand_make_variables()

[aiuto]

Description of the problem / feature request:

The Skylark linter complains that expand_make_variables() is deprecated, but the recommended replacement is not sufficient. For example: expand_make_variables will expand 'foo-$(BAR)' correctly, while the canonical use of ctx var:
version = ctx.attr.version.format(**ctx.var)
will not, because it only does %{name} expansion. We need a global method to do $(VAR) substitution, or to transform the string into something .format compatible.

Feature requests: what underlying problem are you trying to solve with this feature?

Add a global method to do $(VAR) substitution from a dictionary. E.g.
expand_variables(string, dict)
usage
expand_variables(ctx.attr.my_attr, **ctx.var)

OR, a method to convert $(X) substitutions to string format compatible text.

make_to_format(ctx.attr.my_attr).format(**ctx.var)

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

A rule with

• A string attribute, X.
• "Make style" variable substitution is required for X. E.g. $(TARGET_CPU)

What operating system are you running Bazel on?

Linux, but not applicable.

What's the output of bazel info release?

bazel-bin: /home/aiuto/.cache/bazel/_bazel_aiuto/adc5a676660c54154e7bf375b1444975/execroot/google3/bazel-out/k8-fastbuild/bin
bazel-genfiles: /home/aiuto/.cache/bazel/_bazel_aiuto/adc5a676660c54154e7bf375b1444975/execroot/google3/bazel-out/k8-fastbuild/genfiles
bazel-testlogs: /home/aiuto/.cache/bazel/_bazel_aiuto/adc5a676660c54154e7bf375b1444975/execroot/google3/bazel-out/k8-fastbuild/testlogs
character-encoding: file.encoding = ISO-8859-1, defaultCharset = ISO-8859-1
command_log: /home/aiuto/.cache/bazel/_bazel_aiuto/adc5a676660c54154e7bf375b1444975/command.log
committed-heap-size: 1277MB
execution_root: /home/aiuto/.cache/bazel/_bazel_aiuto/adc5a676660c54154e7bf375b1444975/execroot/google3
gc-count: 9
gc-time: 359ms
install_base: /home/aiuto/.cache/bazel/_bazel_aiuto/install/c25ea2c3043bcba07b93dde10595066c
java-home: /home/aiuto/.cache/bazel/_bazel_aiuto/install/c25ea2c3043bcba07b93dde10595066c/_embedded_binaries/embedded_tools/jdk
java-runtime: OpenJDK Runtime Environment (build 9.0.7.1+1) by Azul Systems, Inc.
java-vm: OpenJDK 64-Bit Server VM (build 9.0.7.1+1, mixed mode) by Azul Systems, Inc.
max-heap-size: 15014MB
output_base: /home/aiuto/.cache/bazel/_bazel_aiuto/adc5a676660c54154e7bf375b1444975
output_path: /home/aiuto/.cache/bazel/_bazel_aiuto/adc5a676660c54154e7bf375b1444975/execroot/google3/bazel-out
package_path: %workspace%
release: release 0.16.0
repository_cache: /home/aiuto/.cache/bazel/_bazel_aiuto/cache/repos/v1
server_pid: 165154
used-heap-size: 58MB
workspace: /google/src/cloud/aiuto/curl/google3

Have you found anything relevant by searching the web?

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

[laurentlb]

I would recommend to change the format when possible. Instead of "$(TARGET_CPU)", prefer "{TARGET_CPU}".

This won't work if you port a native rule, and need to be compatible, though. Are there any other use-cases? I don't think this is needed for new rules (written from scratch).

[litghost]

In general you cannot use the "{variable name}" form of the "$(variable name)" form. For example, genrule's cannot abstractly expand variables in the "{variable name}", only "$(variable name)" form. So if you make a Skylark function that consumes a string which may contain variables, you cannot both expand it in a genrule and expand it in a rule implementation without relying on expand_make_variables.

So the original poster is correct, expand_make_variables() cannot be replaced with str.format(**ctx.var), they are not equivalent.

[laurentlb]

Is it an important use-case, to use the same string in both genrule and your new rule? When you create a rule, you typically don't need a genrule anymore.

genrule has many problems, I believe we should make a simpler alternative and also use the {variable} format instead of $(variable).

[litghost]

"Is it an important use-case, to use the same string in both genrule and your new rule? When you create a rule, you typically don't need a genrule anymore." -> Yes.

There are many cases where a genrule already exists (sometimes multiple) and they live alongside custom rule/impl. In general, the rule/impl are avoided when genrule's suffice given that the macro/genrule syntax is much simpler to use the rule/impl.

Also this is existing rule/impl and genrules, not new ones. In specific case I am thinking of, there are two rule/impl's, and four genrules that already exist that share a string.

"genrule has many problems, " -> Like what? genrules are far better documented, and much simpler to reason about than full rule definitions. In our team's usage we have maybe 2-3 rule/impl instances, and more than 50-100 different genrules.

"I believe we should make a simpler alternative and also use the {variable} format instead of $(variable)." -> I don't disagree, but bazel doesn't have that alternative. So bazel marked ctx.expand_make_variables() as deprecated without an equivalent replacement. The entire point of this issue is that ctx.expand_make_variables() is marked deprecated without a suitable replacement strategy.

[jvoung]

Is there a recommendation for expand_make_variables + introspecting on java_binary, cc_library, etc. attributes like copts and jvm_flags that are 'Subject to "Make variable" substitution'?

The usecase of introspection I'm thinking of is mostly copts to pass along to an IDE (e.g., $(GENDIR) bazelbuild/intellij#406). The jvm_flags is a bit more speculative (haven't yet seen a case where someone uses make variables). For copts, suppose the IDE works in conjunction with clangd, then the clangd instance should know the copts flags to best match how bazel is compiling a file. Is it up to some of native rule APIs (e.g., cc_common, and something else for jvm_flags)?

[laurentlb]

cc @c-parsons

[jvoung]

Hi, any suggestions for the copts, jvm_flags, etc. question?

[aiuto]

@jvoung It sounds like your concern is best handled by having your rule look into the toolchain, find the flags and add them to the dict for expand_make_variables. Automatically allowing expansion to peer into any toolchain is fraught with problems.

That said, you still have the basic problem. We need a way to call expand on the $(var) syntax. Or, we have to make a breaking change that forces the definition of "make variables" to be in the ${syntax}.
That is terrible for a few reasons

• The word "Make" in "Make variables" implies Make syntax. So $(VAR) is the right way to express the variable.
• Requiring %{var} expansion leaks our use of Python syntax in Starlark into the user space. It is foreign to users migrating from every other build system (except maybe SCons and Gyp). Make, Bash and cmd.exe users have to learn a new syntax.

We should probably rename the concept to "expand variables" and allow all the syntaxes.
$var $(var) %{var} %var%, with escapes allowed because you know someone will need it.
The documentation about expansion, could explain that we allow Make, bash and cmd.exe syntax as an aid to people who want to migrate. Possibly we give a multi-year migration window to get people to migrate to the %{var} syntax and gradually phase out the rest. Even though it is a trivial change to the text of the rules, we are making a large conceptual change by changing make variable expansion to pythonesque variable expansion.

In any case, we either have to keep the equivalent to expand_make_variables or do something that will break people.

[mboes]

I'm curious now, why whas expand_make_variables deprecated in the first place, especially when expand_location was not?