bazel icon indicating copy to clipboard operation
bazel copied to clipboard

native.bazel_version is not available for macros or rules

Open haberman opened this issue 6 years ago • 17 comments

Description of the problem / feature request:

I would like access to native.bazel_version from BUILD macros and rules. It appears that native.bazel_version only works from WORKSPACE macros (as tested here).

This also means that versions.bzl from bazel-skylib only works from WORKSPACE files. This is very surprising and appears to be an undocumented limitation. If you try to invoke it from a BUILD file you get:

ERROR: /usr/local/google/home/haberman/.cache/bazel/_bazel_haberman/17449526a2508fe9f4def2619c761b7e/external/com_google_protobuf/BUILD:284:2: in //:build_defs.bzl%_upb_proto_libra
ry_aspect aspect on proto_library rule @com_google_protobuf//:field_mask_proto:
Traceback (most recent call last):                                                                                                                                                 
        File "/usr/local/google/home/haberman/.cache/bazel/_bazel_haberman/17449526a2508fe9f4def2619c761b7e/external/com_google_protobuf/BUILD", line 284
                //:build_defs.bzl%_upb_proto_library_aspect(...)                                                                                                                   
        File "/usr/local/google/home/haberman/code/upb/build_defs.bzl", line 383, in _upb_proto_aspect_impl
                cc_library_func(ctx = ctx, name = ctx.rule.attr.na..., <3 more arguments>)
        File "/usr/local/google/home/haberman/code/upb/build_defs.bzl", line 287, in cc_library_func
                versions.get()                                                                                                                                                      
        File "/usr/local/google/home/haberman/.cache/bazel/_bazel_haberman/17449526a2508fe9f4def2619c761b7e/external/bazel_skylib/lib/versions.bzl", line 20, in versions.get       
                native.bazel_version                                                                                                                                                
no native function or rule 'bazel_version'  

It took a lot of searching to figure out the reason for this error: versions.bzl only works from WORKSPACE files.

Since the native module is only available during loading (not analysis), it might be better to have a separate module for bazel_version that is available during both loading and analysis. Then bazel-skylib's versions.bzl could use that. That way it could work from any context.

Feature requests: what underlying problem are you trying to solve with this feature?

My project only works with certain versions of Bazel. I would like to warn users if they are building my project with an unsupported Bazel version, and tell them what the supported versions are.

Also, supporting older Bazel versions sometimes requires legacy fallback code. I would like to be able to switch out different logic based on the Bazel version.

What operating system are you running Bazel on?

Linux and macOS.

What's the output of bazel info release?

I'm currently supporting Bazel 0.24.1 and 0.25.2.

Have you found anything relevant by searching the web?

I discovered that native.bazel_version is tested, but only from WORKSPACE files: https://github.com/bazelbuild/bazel/blob/master/src/test/shell/bazel/skylark_repository_test.sh#L1159-L1199

I also discovered that native.bazel_version has never actually been documented in https://docs.bazel.build/versions/master/skylark/lib/native.html, despite the fact it is used from bazel-skylib.

haberman avatar May 13 '19 16:05 haberman

bazel_version is not in the documentation at all. This should be in workspace documentation.

laurentlb avatar May 14 '19 18:05 laurentlb

While there may be a documentation bug wrt. the existing behavior, my feature request is to have bazel_version available from macros and rules.

For now I'm working around this by using a repository rule to write bazel_version to a file that can then be imported by rules: https://github.com/haberman/upb/blob/bazel25/repository_defs.bzl

I would rather not have to hack around this. bazel_version and versions.bzl should work from rules.

haberman avatar May 14 '19 19:05 haberman

Can you clarify why you need it?

laurentlb avatar May 14 '19 20:05 laurentlb

Quoting from my original report:

My project only works with certain versions of Bazel. I would like to warn users if they are building my project with an unsupported Bazel version, and tell them what the supported versions are.

Also, supporting older Bazel versions sometimes requires legacy fallback code. I would like to be able to switch out different logic based on the Bazel version.

Here is my code that is doing both of these things: https://github.com/haberman/upb/blob/4451b790bd911743d2d4ee8783fe1bb93c2f4c54/build_defs.bzl#L256-L281

This is related to the fact that Bazel 0.25.0 and 0.25.1 are broken for my use case due to https://github.com/bazelbuild/bazel/issues/8254 and https://github.com/bazelbuild/bazel/issues/8226

haberman avatar May 14 '19 20:05 haberman

Can you do the version check in the workspace file?

(or in https://github.com/protocolbuffers/protobuf/blob/master/protobuf_deps.bzl)

laurentlb avatar May 17 '19 20:05 laurentlb

That works for enforcing certain versions, but it doesn't help for the case of falling back to different code for older versions.

Is there a reason not to allow this in rules, or is it just an issue of finding the time to implement it?

How is somebody supposed to know that versions.bzl from bazel-skylib just doesn't work from rules?

haberman avatar May 18 '19 03:05 haberman

I guess, if we allow native.bazel_version at all, we can as well allow it everywhere, as the work around you use, the value is available anyway. However, before we encourage wider use of this value, we probably should ensure that development versions of bazel have a proper version number (e.g., alpha-Version of the release to come) as otherwise we have code that cannot properly be tested on non-releases, making it hard to run such projects on CI, and to properly bisect which bazel commit broke something, etc.

aehlig avatar May 20 '19 12:05 aehlig

How is somebody supposed to know that versions.bzl from bazel-skylib just doesn't work from rules?

You can file a documentation bug against Skylib.

Is there a reason not to allow this in rules

Before we add any feature, we need to go through a process and check the implications. I don't really want to encourage code that behaves differently based on the version number. You can use versions.bzl to tell users when they have an old version of Bazel.

Do you really have to support old versions of Bazel with a different code path?

laurentlb avatar May 22 '19 14:05 laurentlb

We update bazel infrequently enough that we occasionally miss migration periods but still have to support both the old and new versions for a while to give people time to update their code branches and local bazel installations. Macros that let us call things in a manner appropriate for the version in use would simplify these migrations by reducing how frequently we have to patch the old version to support them.

gholms avatar May 31 '19 02:05 gholms

Do you really have to support old versions of Bazel with a different code path?

We update bazel infrequently enough ...

In my case, we ship a C++ library named Drake, and we need to support users as their migrate at different rates. Some users could be using Bazel 2.x in their application, and some could be using 3.x or 4.x, against the same version of Drake. The rules_cc data structures are mutually incompatible between the 2.x and 4.x, so I was looking into changing my starlark code depending on which version of the embedded rules_cc is being used. In the end, I decided to drop support for 2.x instead.

jwnimmer-tri avatar Dec 10 '20 20:12 jwnimmer-tri

Another +1 for this, we had a place where we wanted this because bazel broke support for an API between the LTS version and HEAD, and as rules authors we wanted to continue supporting both for our users, I came up with a huge hack instead. https://github.com/bazelbuild/bazel/issues/14996

keith avatar Mar 09 '22 00:03 keith

Adding a +1 for a way to do version or feature detection. The particular case I ran into is migrating off testing.TestEnvironment. It's been replaced by RunEnvironmentInfo, but, because that's a global only present in Bazel 5.3+, it's hard to write code that uses that new name when available and falls back to the old name when not (they're currently aliases, so its a matter of code tidiness).

rickeylev avatar Nov 28 '22 17:11 rickeylev

as a stopgap, does hasattr(testing, "TestEnvironment") work?

Wyverald avatar Nov 28 '22 17:11 Wyverald

as a stopgap, does hasattr(testing, "TestEnvironment") work?

testing.TestEnvironment is still available, it's only marked as deprecated in docs.

It doesn't stop there, since I only added the env_inherit parameter to RunEnvironmentInfo later, which also can't be feature detected.

fmeum avatar Nov 28 '22 18:11 fmeum

I see. I think it might be more prudent to offer some sort of general feature detection API, as opposed to a version string check. I'd say this is slightly higher than P4, but the Starlark team might not have the bandwidth unfortunately.

Wyverald avatar Nov 28 '22 18:11 Wyverald

I think if we had some getattr equivalent that can look up global symbols, that + getattr should be enough to derive any breaking changes or new additions in the api at runtime no?

chancila avatar Dec 16 '22 19:12 chancila

I think if we had some getattr equivalent that can look up global symbols, that + getattr should be enough to derive any breaking changes or new additions in the api at runtime no?

That wouldn't quite cover everything as new functionality can also be added as new parameters on an existing method.

fmeum avatar Dec 16 '22 20:12 fmeum

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

github-actions[bot] avatar Feb 20 '24 01:02 github-actions[bot]

If you need to detect a Bazel feature, use https://github.com/bazel-contrib/bazel_features instead. It works within rules or macros and if any particular feature is missing from the list, you can easily add it yourself.

fmeum avatar Feb 20 '24 07:02 fmeum

I just read the doc Bazel feature detection in Starlark where the idea of exposing native.bazel_version to rules and macros was considered and rejected again in March of 2023 (see the heading: "Double down on version comparison").

I find this somewhat unfortunate. As the doc notes, just exposing native.bazel_version solves the problem and is very easy to implement. It would satisfy a longstanding feature request. The only rationale given for rejecting this feature request is:

As noted above, comparing on the version has a major downside in that it results in code that's not very readable or maintainable.

In other words, the fear is that users will sprinkle version checks like this throughout their code:

def foo(...)
    if versions.is_at_least("6.0.0", native.bazel_version):
        # Do something
    else:
        # Do something else

The blessed solution now is to use https://github.com/bazel-contrib/bazel_features, so your code looks like:

def foo(...)
    if bazel_features.toolchains.has_optional_toolchains:
        # Do something
    else:
        # Do something else

The net effect is the same, except that the feature check is more readable. Of course, the user could have done the same in their own code:

has_optional_toolchains = versions.is_at_least("6.0.0", native.bazel_version)

def foo(...)
    if has_optional_toolchains:
        # Do something
    else:
        # Do something else

By adding the intermediary of https://github.com/bazel-contrib/bazel_features, Bazel ensures that users must assign a semantic name for the feature, instead of branching directly on the version. The downside is that users must have a change accepted in a third-party repo that is not under their control, and this may involve delays or differences of opinion about the best way to do something.

For anyone who would prefer to avoid this, and get access to the bazel version directly in their own code, I'd recommend the following workaround, which still works:

# bazel_version_repository.bzl

def _impl(repository_ctx):
    s = "bazel_version = \"" + native.bazel_version + "\""
    repository_ctx.file("bazel_version.bzl", s)
    repository_ctx.file("BUILD", "")

bazel_version_repository = repository_rule(
    implementation = _impl,
    local = True,
)
# WORKSPACE

load(":bazel_version_repository.bzl", "bazel_version_repository")

bazel_version_repository(
    name = "bazel_version",
)
# Now you can use `bazel_version` from macros and rules:

load("@bazel_version//:bazel_version.bzl", "bazel_version")

def foo_macro():
    print(bazel_version)

This is ultimately how the https://github.com/bazel-contrib/bazel_features repo does it internally: https://github.com/bazel-contrib/bazel_features/blob/c73936012bb5766252b4209c6b3e0c935cb91b8e/private/version_repo.bzl#L16

haberman avatar Feb 20 '24 17:02 haberman

@haberman Thanks for explaining the little magic there is behind bazel_features.

If users choose to go with hardcoded version checks, that's totally fine. Just keep in mind that:

  • reviews in bazel_features are usually very quick and we can tag a release right after a merge
  • version ranges are rarely contiguous and can change over time due to prereleases and cherry-picks, which are easier to keep up-to-date when they are maintained in a single location.

fmeum avatar Feb 28 '24 15:02 fmeum