prelude is stricter at head

[laurentlb]

I'm filing this bug to assess the impact of the change.

From the draft release notes:

The prelude file (//tools/build_rules:prelude_bazel) is now processed as a Starlark module, rather than being sourced into the BUILD file textually.

My comment:

With 2a73a73, the prelude is handled in a saner way (cc @brandjon):

• the prelude is evaluated once and then cached (instead of being append in each BUILD file)
• you can no longer call macros/rules from the prelude (but no one was doing that, right?), just load them
• error messages are better, with a proper stack trace

According to @kevingessner:

This is not "a breaking change in theory" for us; it is an actual breaking change that may require significant work to fix. This should not be included in a point release, but rolled out with an --incompatible flag flipped in a major version. What's the motivation for slipping this incompatibility into 3.5.0?

[aiuto]

@alandonovan Need to discuss backpatching a rollback to the release branch vs. hiding change behind a flag.

[brandjon]

For context: The motivation behind this change is to make prelude evaluation more consistent with regular bzl loading. Before this change, the (parsed) prelude contents were syntactically prepended to every BUILD file, sort of like a C-style #include directive. This meant that the prelude's AST was shared by all BUILD files, and therefore couldn't be statically processed in the usual way by the Starlark interpreter (specifically, to record variable scoping information for faster runtime evaluation). It also made BUILD file evaluation even more complex, compared to treating the prelude as just another (virtual) load dependency.

We need to assess the use cases and how difficult they are to migrate. So if you're broken, please chime in.

I also opened #11946 to discuss possibly eliminating the prelude file altogether.

[kevingessner]

Thanks for making this ticket! We use the prelude for two things:

1. To load commonly-used rules that should be available in every package. This includes some that shadow bazel-provided rules (see Feature request: Setting default javacopts for all java_library rules in workspace #3427). It sounds like this usage will be supported going forward.

2. To create universal targets in every package. In particular, we create syntax-checking test targets and some linters that we want in every package. These are macros that are loaded and called from the prelude, e.g.

load("//tools/build_rules:python.bzl", "py_formatting_test")

py_formatting_test()

with that macro defined roughly as:

def _test_impl(name, srcs):
  if srcs:
    native.sh_test(...)

def py_formatting_test():
  _test_impl(name="py_formatting_test", srcs=native.glob(["*.py"])

This doesn't work if the prelude is handled as a regular bzl file, as the macros can't be called during load() ("The native module can be accessed only from a BUILD thread. Wrap the function in a macro and call it from a BUILD file"). So this would break with this change. Is there another way of supporting this kind of target creation?

[aiuto]

Ah ha. Thanks Kevin. That is an interesting use case. We cope with that at Google by doing it metadata hooks at the source code repository level. Essentially, we set up the SCM so that we do the format test at PR pre-merge time, through non-bazel mechanisms. One obvious trade-off is that we have to maintain a more complex SCM than git, but we don't have to run (literally) millions of style checks in addition to the behavioral tests that our continuous testing pipeline is looking at.

[aiuto]

No longer a release blocker for 3.5. I moved the baseline to before this change.

[laurentlb]

@kevingessner Would it be acceptable for you to update the BUILD files and make them explicit?
i.e. add this line everywhere:

py_formatting_test()

With a presubmit check, you can enforce that the call is present. The load statement isn't required if you put the load in the prelude file.

I uderstand it's not ideal for you, but supporting your use-case causes some problems. Right now, this bug is blocking the next Bazel release. Anyone has an opinion on what's the way forward?

We could design a solution to support your use-case, but that would take time (and we need releases in the meantime).

cc @alandonovan

[kevingessner]

@laurentlb I thought the change was reverted, so releases could continue?

#11940 (comment)

We can do something like that, but I definitely liked having it automatic. I'd love to see a feature like that for the future.

[laurentlb]

It was not reverted. Bazel 3.5 used a baseline just before the change, but the change is still at head and is blocking 3.6.

[kevingessner]

Ah, I didn't realize. I was hoping this would be reverted and put behind a flag. I guess if you want to put this breaking change in 3.6, go ahead. I'll work around it when we upgrade.

[aiuto]

I see you added release blocker again.
Just clarifying that you meant a 3.6 release blocker. And now that is less clear.