Need replacement for ctx.expand_make_variables()

aiuto commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

"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 commented 6 years ago

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) https://github.com/bazelbuild/intellij/issues/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 commented 6 years ago

cc @c-parsons

jvoung commented 5 years ago

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

aiuto commented 5 years ago

@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 commented 5 years ago

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

aiuto commented 5 years ago

We really should just address this. Asking everyone to change from $(var) to {var} is going to be a long migration. In the interim, rule writers will have to support both, so we'll need tooling to change $(var) to the new format (for which I give strong downvotes to {var}, as that is very Pythonic, and not what many users are used to $(var), ${var} and %var% are more widely known).

But if we do add the tooling to convert from other formats to python string format style for the migration duration, why don't we just put the code back in. We can move it from ctx and just make it a core Starlark method.

brandjon commented 3 years ago

I agree this requires a little more study before we go ahead with the removal.

aiuto commented 2 years ago

cc: @buildbreaker2021

buildbreaker2021 commented 2 years ago

There is https://github.com/bazelbuild/bazel/blob/master/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl#L856-L868

Even though the name suggests it works for copts, it can be used for other make variable expansion as well. It is possible to provide it with additional variable definitions and also supports shell tokenization.

The method is incomplete, in a sense that some special variables like $@ are not handled by the expansion.

Also something worth noting is that it is only available for builtin rules.

comius commented 4 months ago

Maybe relevant: https://github.com/bazelbuild/bazel-skylib/pull/486

aiuto commented 4 months ago

And we don't seem to be able to get that checked in either. Sigh.

bazelbuild / bazel