bazelbuild / bazel-skylib

Common useful functions and rules for Bazel
https://bazel.build/
Apache License 2.0
387 stars 180 forks source link

Expose int- and string-valued build settings as Make variables #440

Closed fmeum closed 1 year ago

fmeum commented 1 year ago

While build settings allow for much cleaner flag and setting definitions than --define, they have the major drawback that rules need to provide dedicated support for them, which isn't the case for native and most community-maintained rules.

This change attempts to bridge this gap by optionally exposing the value of the common build setting types as Make variables to rules that depend on them via the toolchains attribute: If the new make_variable attribute is set, the value of the flag or setting is available as a Make variable with that.

Consistency with pre-defined Make variables is enforced by limiting the character set for make_variable values to [A-Z0-9_]. The new attribute is also only added to int- and string-valued build settings as the other types lack a canonical stringification.

Work towards https://github.com/bazelbuild/bazel/issues/14859

fmeum commented 1 year ago

CC @aranguyen for the configurability point of view

fmeum commented 1 year ago

@aranguyen @brandjon Friendly ping, this has come up in Slack again. It does solve a real need.

fmeum commented 1 year ago

My aim with this PR is to realize the following:

As a result, users could start replacing --defines with build settings, knowing that they won't incur new limitations. If we see increased adoption (even Bazel's IntelliJ plugin still relies on defines), we would have an easier time coming up with more informed designs of new features for build settings.

I don't have a concrete idea for universal support for build settings that doesn't involve explicit rule opt-in. At this point I'm not even sure whether it would be necessary - moving the Starlark versions of Make variable and location expansion used by the built-in rules to Skylib and packaging them in a very friendly API may actually be good enough.

gregestren commented 1 year ago

That all works for me. Thanks.

gregestren commented 1 year ago

We'll have to deal with write access reviewers (I'm not. :( ) after my vacation if no one else gets to this first.