search

bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
23.07k stars 4.04k forks source link

Do not munge `visibility` attribute for targets created by symbolic macros #23855

Open brandjon opened 1 week ago

brandjon commented 1 week ago

@bazel-io flag 8.0.0 This potentially blocks Bazel 8.0 because without fixing this, the bazel query-introspected visibility value of targets created in Symbolic Macros is misleading/inconsistent.

Currently, for targets created within a symbolic macro, the visibility attribute that gets stored on the target is munged to include the instantiating location. This contrasts with the raw visibility attribute that gets stored on targets not in symbolic macros, which matches whatever the visibility = argument passed to the instantiation site was.

We can't munge for targets not in symbolic macros, because it would likely imply a memory regression for the overwhelming majority of the build, and it would change the introspected visibility value for those targets relative to what is seen today in native.existing_rules and bazel query. But at the same time, the munging for targets in symbolic macros is also problematic, both due to the inconsistency and because tooling that reads the introspected values might expect them to match the source text of a BUILD file.

The proper solution is probably to be consistent by

  1. not munging visibility for any target
  2. computing the VisibilityProvider at analysis time without relying on said munging, deferring the concatenation of the callsite location to that point in time
  3. creating a synthetic attribute, $actual_visibility, to hold the munged value (regardless of whether the target is inside a symbolic macro), that can be introspected in bazel query but not in native.existing_rules. This attribute should not be materialized in normal builds.

Symbolic macros themselves also have a visibility attribute, and it must always be munged (as it currently is) so that the implementation function sees the correct value for its visibility parameter. But if we expose symbolic macro attributes via bazel query in the future, we may very well want to avoid the munging to preserve consistency with rules. Note that munging happens at every level of a chain of symbolic macro calls, and the raw visibility for one macro is the munged visibility for its caller.

Implementing this probably just requires a little work in RuleFactory and ConfiguredTargetFactory, plus updating some code comments that explain the outdated invariant. I'm not completely sure what the precedent is for synthetic query-only attributes.

brandjon commented 2 days ago

Let's track the synthetic attribute part separately as FR #23893, without necessarily blocking 8.0 on that part. (The rest of this issue still blocks 8.0.)