Allow setting rule name for analysis tests

[hvadehra]

After bazelbuild/bazel@3a267cb, it is possible for rules to be exported without a top-level assigment. This will help in removing duplication and boilerplate for rule tests.

Design doc: https://docs.google.com/document/d/12sC92mq7WasTvDWsm4U782oCegitfi1jbgOnMIGCe-A/edit#heading=h.5mcn15i0e1ch

[hvadehra]

Now that bazel 5.0 is released, this is now ready for review @brandjon

[tetromino]

Question: would this break Skylib on Bazel 4.x?

[tetromino]

We'd need:

• ideally - a way to avoid breaking Bazel 4.x (as a hack - maybe check for a bazel 5.x-only feature using hasattr?)
• a test (so we can verify this works on Bazel 4.x and 5 and at head)
• the docs to state that the feature is Bazel 5-only
• to regenerate the docs using ./docs/regenerate_docs.sh

If I understand correctly, this PR is considered critical, so some of these things I can work on. But I really wish to avoid breaking Bazel 4 users.

[comius]

We'd need:

• ideally - a way to avoid breaking Bazel 4.x (as a hack - maybe check for a bazel 5.x-only feature using hasattr?)

There are two types of backward compatibility. Backward compatibility with Bazel version and backward compatibility with skylib users.

We won't be able to keep Bazel backwards compatibility indefinitely and I wonder what is the purpose of version checking hacks? In this particular case, you can't check directly if the rule call has name parameter. You could check if there is a JavaPluginInfo in java_common, or something else that appeared in Bazel 5.0, but that would introduce tech-debt in my opinion.

There is a proposal how to version rules_* repositories to keep versions in sync with Bazel. https://docs.google.com/document/d/1s_8AihGXbYujNWU_VjKNYKQb8_QGGGq7iKwAVQgjbn0/edit#heading=h.3d0j52sbj6iq

skylib is much similar to other rules_* and I'd say rules_* are its primary user. Instead of keeping Bazel compatibility, I'd propose we follow the @philwo's proposal. This simply means tagging skylib for Bazel 4.0.0 and setting up CI for Bazel 4.0.0 on that tag. The docs also contains info on how to do this.

In addition to this, bzlmod should be able to take care and select the correct version skylib or warn the user their Bazel version is not compatible.

• a test (so we can verify this works on Bazel 4.x and 5 and at head)

A test would need CI setup for Bazel 4.x and Bazel 5.x. Even without this PR, the owners of the skylib should set up CI, if they desire Bazel backwards compatibility - otherwise how do you know a PR doesn't break it accidentally? As such I would consider this out of scope for this PR.

• the docs to state that the feature is Bazel 5-only
• to regenerate the docs using ./docs/regenerate_docs.sh

If I understand correctly, this PR is considered critical, so some of these things I can work on. But I really wish to avoid breaking Bazel 4 users.

cc @aiuto @brandjon @gregestren

[aiuto]

A test would need CI setup for Bazel 4.x and Bazel 5.x. Even without this PR, the owners of the skylib should set up CI, if they desire Bazel backwards compatibility - otherwise how do you know a PR doesn't break it accidentally? As such I would consider this out of scope for this PR.

I think we need to invert this. Having each rule set test against LTS 4.x does not help. What has to happen is that bazel team runs a 4.x CI that has a WORKSPACE including a suite of skylib, rules_[cpp,go,java,python,nodejs,...], protobuf, grpc all at specific versions or tags. The WORKSPACE would set the versions before any of the rules, so that it wins in any maybe() cases. That CI would then run the tests on the rules.

Only with something like that do you know that bazel 4.x + some version of skylib + some version of rules_go actually work together.

If a rules author does not fork their rules by version, this CI will catch breaking changes. If they do, then they will have to submit a PR against the CI to update the recommended version for 4.x.

I'm not a fan of forcing rule authors to maintain multiple branch tracks if they can make their rules backwards compatible with a bazel version test. We know people are doing that in one way or another, so we should be providing ways to make their checks more sustainable.

[comius]

cc @meteorcloudy

I think we need to invert this. Having each rule set test against LTS 4.x does not help.
What has to happen is that bazel team runs a 4.x CI that has a WORKSPACE including a suite of skylib, rules_[cpp,go,java,python,nodejs,...], protobuf, grpc all at specific versions or tags. The WORKSPACE would set the versions before any of the rules, so that it wins in any maybe() cases. That CI would then run the tests on the rules.

You seem to be describing LTS 4.x downstream test. I think this was also part of a plan.

Only with something like that do you know that bazel 4.x + some version of skylib + some version of rules_go actually work together.

I believe if skylib was sufficiently unit tested a lot of breakages would be revealed already when running it on bazel 4.x. Integration tests / downstream tests are following in test hierarchy and should not be a replacement for poor unit testing. Having them is of course better than not.

I'm not a fan of forcing rule authors to maintain multiple branch tracks if they can make their rules backwards compatible with a bazel version test. We know people are doing that in one way or another, so we should be providing ways to make their checks more sustainable.

There's was a discussion on PR bazelbuild/bazel#14508 about this. I'm not a fan of version tests, however the discussion led to a check_version function that I'd find sustainable.

I know people are doing version checks, but there's always a limit to them, and you need to have some mechanism of versioning eventually.

[meteorcloudy]

What has to happen is that bazel team runs a 4.x CI that has a WORKSPACE including a suite of skylib, rules_[cpp,go,java,python,nodejs,...], protobuf, grpc all at specific versions or tags.

This sounds like the Bazel federation approach, which didn't make it through. But I guess providing a set of build rules and dependencies that works together is still a good idea, we could probably give it another try with Bzlmod, one advantage is that we don't have to worry much about dependency resolution.

In future, the BCR will provide information on which checked-in module versions are compatible with which Bazel version. But I think if a rule wants to maintain compatibility with older Bazel version at HEAD, they should setup presubmit like Ivo suggested. Adding a Bazel downstream test for 4.x is also a good idea, then we can notify downstream projects and they can make an informed decision. I'll look into that.

[tetromino]

I think the reasonable way to go forward is to declare that Skylib 1.2.x will continue to support Bazel 4.2; Skylib 1.3 and higher will require at least Bazel 5. I've added a new note to that effect in the 1.2.0 release notes: https://github.com/bazelbuild/bazel-skylib/releases/tag/1.2.0. Having made that decision, we can gradually improve tests to better guarantee it.

Regarding this PR specifically - before merging, we still need (1) a test (or just an update to an existing test, for simplicity!) that uses the new parameter; and (2) for the docs to be regenerated using ./docs/regenerate_docs.sh

[tetromino]

I tried writing the simplest imaginable test using this feature:

def _dummy_analysis_test(ctx):
    env = analysistest.begin(ctx)
    # <test stuff>
    return analysistest.end(env)

analysistest.make(
    _dummy_analysis_test,
    name = "dummy_analysis_test",
)

dummy_analysis_test(
    name = "my_dummy_analysis_test",
    target_under_test = ":foobar",
)

and it failed with name 'dummy_analysis_test' is not defined (did you mean '_dummy_analysis_test'?) (this is using Bazel 5.1.0).

Can you please add a working test, or at least a bit of documentation/example explaining how this feature should be used?

[tetromino]

It appears that the rule.name feature might allow multiple rule classes to be de defined with the same name under some circumstances, resulting in Bad Things Happening to Bazel's internal consistency guarantees.

Tracked internally as b/226379109.

We probably will need to delay this PR until a future Bazel release in which these issues are fixed.

[comius]

We dropped this approach.