Added native.bazel_version for use in BUILD and bzl files

[UebelAndre]

This change is intended to improve rules maintainer's ability to avoid breaking changes by gating certain functionality behind a version check. Users can now write rules which check the version of Bazel as an indication that certain functionality will be available.

@rules_python//python/pip_install:requirements.bzl is one such example where this rule could be used.

diff --git a/python/pip_install/requirements.bzl b/python/pip_install/requirements.bzl
index b040ebd..219466f 100644
--- a/python/pip_install/requirements.bzl
+++ b/python/pip_install/requirements.bzl
@@ -75,7 +75,7 @@ def compile_pip_requirements(
     }

     # cheap way to detect the bazel version
-    _bazel_version_4_or_greater = "propeller_optimize" in dir(native)
+    _bazel_version_4_or_greater = native.bazel_version[0:2] not in ("0.", "1.", "2.", "3.")

     # Bazel 4.0 added the "env" attribute to py_test/py_binary
     if _bazel_version_4_or_greater:

Or, for users of bazel_skylib:

diff --git a/python/pip_install/requirements.bzl b/python/pip_install/requirements.bzl
index b040ebd..f5dd545 100644
--- a/python/pip_install/requirements.bzl
+++ b/python/pip_install/requirements.bzl
@@ -1,5 +1,6 @@
 """Rules to verify and update pip-compile locked requirements.txt"""

+load("@bazel_skylib//lib:versions.bzl", "versions")
 load("//python:defs.bzl", "py_binary", "py_test")
 load("//python/pip_install:repositories.bzl", "requirement")

@@ -75,7 +76,7 @@ def compile_pip_requirements(
     }

     # cheap way to detect the bazel version
-    _bazel_version_4_or_greater = "propeller_optimize" in dir(native)
+    _bazel_version_4_or_greater = versions.is_at_least("4.0.0", native.bazel_version)

     # Bazel 4.0 added the "env" attribute to py_test/py_binary
     if _bazel_version_4_or_greater:

closes #8305

[UebelAndre]

Hey, @meteorcloudy, philwo suggested that you might be a good reviewer for this change. Do you have bandwidth to take a look?

[meteorcloudy]

See https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java;l=382;bpv=1;bpt=0

native.bazel_version is already available if your .bzl file is used in the WORKSPACE file (repository rules). The change you made is making it also available to the bzl files used in the BUILD files (build rules). I think it's fine, but the test you added is testing the repository rule, so can you please fix that?

FYI @Wyverald We should also make native.bazel_version available in Bzlmod?

[Wyverald]

I feel a bit uneasy about introducing this. It's essentially another source of non-hermetic input, so it somewhat makes sense for repo rules, but this change would make it available to build rules (and even build macros). IMO the use case is rare enough to warrant the need to jump through hoops (e.g. by using a repo rule to store that information) in favor of the API essentially endorsing its use. cc @brandjon @comius as this is really about the build API.

Nevertheless we should indeed look at adding this for bzlmod.

[UebelAndre]

@meteorcloudy The rest I added tests both. I just wasn't sure where to add a test. Happy to move it if you can let me know where

[UebelAndre]

I feel a bit uneasy about introducing this. It's essentially another source of non-hermetic input, so it somewhat makes sense for repo rules, but this change would make it available to build rules (and even build macros).

@Wyverald Can you elaborate here? Why would this be more or less hermetic than getting the value from a generated repository? At the end of the day, both cases produce a string in starlark and not some generated target or file. Having to write a repository rule and load it from there just feels like unnecessary boiler-plate. It now requires consumers or rules to load a new macro to provide the repository instead of just having access to the Bazel version.

[UebelAndre]

Nevertheless we should indeed look at adding this for bzlmod.

@Wyverald @meteorcloudy is that something that should be done in this PR? Where would that code live? I'm pretty unfamiliar with Bazel's code structure.

[comius]

I think that we should not add the ability to do version checks in BUILD or bzl files.

Version checks are hard to understand for the readers of the code. This could be amended with extended documentation about, what feature was added and when, in which version of Bazel. Considering the given Python use case, the "feature check" (hasattr(native, "xxx")) is on other hand self-documenting (except that instead of storing the result in _bazel_version_4_or_greater, the result should be used where a feature is needed or worked around)

Version checks need to be tested against all supported Bazel versions and removed when they are no longer tested. I'm also afraid this will never happen. Maintaining version checking code can potentially become a nightmare. Usually a developer has a single version of Bazel and modifying the version checking code, people will never check if it works with different Bazel versions.

As we are moving language specific rules and modules out of Bazel, version checks should in the future be mostly against modules the current module depends on, and not Bazel itself. bzlmod provides functionality to resolve correct dependent modules. And furthermore also to limit Bazel version with which a version of the module is suppose to work.

There is also a proposal by @philwo, how to version the rules repositories. https://docs.google.com/document/d/1s_8AihGXbYujNWU_VjKNYKQb8_QGGGq7iKwAVQgjbn0/edit#heading=h.3d0j52sbj6iq

So instead of this PR, I propose cleaning up rules_python, to rather do feature checks where needed and in the future to depend on bzlmod functionality to select compatible versions of other modules or enforce Bazel version constraints.

[UebelAndre]

@comius how would I handle cases where a bug fix went into a newer version of bazel that's required for enabling certain functionality? I'd run into this case and wanted to inform users that they needed to update Bazel if they wanted to use a particular rule or attribute but was unable to do so as there was no existing repository rule which exposed native.bazel_version for me to do the check 😞

[comius]

@comius how would I handle cases where a bug fix went into a newer version of bazel that's required for enabling certain functionality? I'd run into this case and wanted to inform users that they needed to update Bazel if they wanted to use a particular rule or attribute but was unable to do so as there was no existing repository rule which exposed native.bazel_version for me to do the check 😞

A principled answer is to let this be handled by Bazel, fixing at head and cherry-picking the fixes to LTS versions we are currently maintaining. And not additionally warning off user.

I see some problems if you try to inform/warn the user. Before there is a fix you don't know at what version it is going to be fixed. You could make a guess, but you could also make a wrong guess (this happened to me with Java toolchainisation in bazel_toolchains repo and caused a lot of problems on each Bazel release).

After there is a fix in Bazel, I think the warning for the user has only a really limited value. It would only happen to users who upgraded just the rules repo without upgrading Bazel.

I agree feature checks are hard or impossible in case of bugs. Hopefully bzlmod comes to the rescue.

To keep rules' implementation and macros simple, I would prefer to close this PR now without merging it.
If it happens that bzlmod is not sufficient solution we can revisit this.

[UebelAndre]

A principled answer is to let this be handled by Bazel, fixing at head and cherry-picking the fixes to LTS versions we are currently maintaining. And not additionally warning off user.

I see some problems if you try to inform/warn the user. Before there is a fix you don't know at what version it is going to be fixed. You could make a guess, but you could also make a wrong guess (this happened to me with Java toolchainisation in bazel_toolchains repo and caused a lot of problems on each Bazel release).

I feel like there is a disconnect here. When there's a bug in Bazel itself, failures may not be obvious as to what the issue is and will require users to investigate something that may already be a known issue. This can be very time consuming. I don't think it makes any sense to perform a version check as a means for warning users that functionality is broken and to guess an arbitrary release where it might be fixed (doing this sounds kinda crazy to me). However, there may be cases (and I've experienced this with rust_toolchains bazelbuild/rules_rust#635) where a bug was found that required a fix in upstream Bazel. The fix went in but users continued to run into this issue. To avoid unnecessary headache for my team, I added a version check using a repository rule to detect the version of Bazel and inform them that they needed to update. The repository rule required an additional load and would not have worked in the upstream rules_rust without forcing users to also load another macro in their workspace file which (IMO) made the change feel unacceptable to upstream to rules_rust. The introduction of native.bazel_version for rules and macros is not a new concept since it can be done using the repository rules but is generally an unusable pattern because of the loads needed.

I agree feature checks are hard or impossible in case of bugs. Hopefully bzlmod comes to the rescue.

To keep rules' implementation and macros simple, I would prefer to close this PR now without merging it. If it happens that bzlmod is not sufficient solution we can revisit this.

My understanding of bzlmod is that it may address issues for problems in rules, not Bazel itself. I feel these changes are allowing rule authors to solve for the issues in Bazel itself in an effective way. I agree with the desire to keep rules clean just as much as I agree with the desire for there to be no bugs. But because the latter is impossible, I think it requires that rules and macros have a few extra lines of code to sufficiently handle or warn about issues.

If you feel strongly that these changes should still not be merged (which would be disappointing 😞), I would say the linked issue (#8305) should also be closed.

[comius]

What about adding a call native.check_bazel_version(compatible_versions: str, message: str) that fails when the Bazel version doesn't match what's in compatible_versions?

compatible_versions might be define the same way as in https://docs.google.com/document/d/1moQfNcEIttsk6vYanNKIy3ZuK53hQUFq1b1r0rmsYVg/edit#

Such a call would support the use case you mention and it would potentially prevent a lot of problems that could be introduced with native.bazel_version kind of checks.

[UebelAndre]

What about adding a call native.check_bazel_version(compatible_versions: str, message: str) that fails when the Bazel version doesn't match what's in compatible_versions?

compatible_versions might be define the same way as in https://docs.google.com/document/d/1moQfNcEIttsk6vYanNKIy3ZuK53hQUFq1b1r0rmsYVg/edit#

Such a call would support the use case you mention and it would potentially prevent a lot of problems that could be introduced with native.bazel_version kind of checks.

I think I like that. How would I go about implementing that? Again, the Bazel repo is pretty unfamiliar to me. Where do you think the functionality would live and where would the tests go?

[comius]

I think the location for such implementation is the same as you have at the moment. For the testing StarlarkIntegrationTest might be a better location.

Retriaged to P3 as I would accept PR with "native.check_bazel_version(compatible_versions: str, message: str) that fails when the Bazel version doesn't match what's in compatible_versions"

[comius]

I believe we reached an agreement for an alternative implementation. As such I will close this PR.