Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

native.bazel_version is not available for macros or rules #8305

Closed
haberman opened this issue May 13, 2019 · 21 comments
Closed

native.bazel_version is not available for macros or rules #8305

haberman opened this issue May 13, 2019 · 21 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Starlark-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols type: feature request

Comments

@haberman
Copy link
Contributor

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.

@laurentlb
Copy link
Contributor

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

@laurentlb laurentlb added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. untriaged type: documentation (cleanup) and removed untriaged team-Starlark labels May 14, 2019
@haberman
Copy link
Contributor Author

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.

@laurentlb
Copy link
Contributor

Can you clarify why you need it?

@haberman
Copy link
Contributor Author

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 #8254 and #8226

@laurentlb
Copy link
Contributor

Can you do the version check in the workspace file?

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

@haberman
Copy link
Contributor Author

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?

@aehlig
Copy link
Contributor

aehlig commented May 20, 2019

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.

@laurentlb
Copy link
Contributor

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 laurentlb added P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Starlark type: feature request and removed team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. untriaged type: documentation (cleanup) labels May 23, 2019
@gholms
Copy link
Contributor

gholms commented May 31, 2019

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.

wchargin added a commit to wchargin/rules_rust that referenced this issue Oct 21, 2020
We use a synthetic `bazel_version` repository to make the Bazel version
available in more contexts (see [bazelbuild/bazel#8305][i8305]). But
some other repositories do so, too, with the same repository name and a
different repository structure. In particular, it is not currently
possible to use `rules_rust` and certain versions of `upb` (downstream
of protobuf) in the same repository, due to their definition here:

https://github.com/protocolbuffers/upb/blob/c1357afb2e39df671d89eaec49033b5329f36a3e/bazel/repository_defs.bzl#L7-L10

An easy workaround is to disambiguate the name. It looks much easier to
change `rules_rust` than to change `upb` and update its long chain of
workspace dependencies, hence this patch. :-)

See my comment on bazelbuild#268 for more details and a full repro:
bazelbuild#268 (comment)

[i8305]: bazelbuild/bazel#8305

Test Plan:
Tested by adding the README’s workspace stanza to TensorBoard (at
current master, 8d629954c251). It fails at `rules_rust = 5998baf`,
but succeeds with a `local_repository` that has this patch.

wchargin-branch: disambiguate-bazel-version-repo
wchargin-source: 7d8ca1dd816cca59f4e1099257e6bd3a77475368
wchargin added a commit to wchargin/rules_rust that referenced this issue Oct 21, 2020
We use a synthetic `bazel_version` repository to make the Bazel version
available in more contexts (see [bazelbuild/bazel#8305][i8305]). But
some other repositories do so, too, with the same repository name and a
different repository structure. In particular, it is not currently
possible to use `rules_rust` and certain versions of `upb` (downstream
of protobuf) in the same repository, due to their definition here:

https://github.com/protocolbuffers/upb/blob/c1357afb2e39df671d89eaec49033b5329f36a3e/bazel/repository_defs.bzl#L7-L10

An easy workaround is to disambiguate the name. It looks much easier to
change `rules_rust` than to change `upb` and update its long chain of
workspace dependencies, hence this patch. :-)

See my comment on bazelbuild#268 for more details and a full repro:
bazelbuild#268 (comment)

[i8305]: bazelbuild/bazel#8305

Test Plan:
Tested by adding the README’s workspace stanza to TensorBoard (at
current master, 8d629954c251). It fails at `rules_rust = 5998baf`,
but succeeds with a `local_repository` that has this patch. Also, in
this repo, `git grep @bazel_version` no longer has any matches.

wchargin-branch: disambiguate-bazel-version-repo
wchargin-source: e712603c3a3ebc45364cc2dcc9eca341301b5197
dfreese pushed a commit to bazelbuild/rules_rust that referenced this issue Oct 23, 2020
We use a synthetic `bazel_version` repository to make the Bazel version
available in more contexts (see [bazelbuild/bazel#8305][i8305]). But
some other repositories do so, too, with the same repository name and a
different repository structure. In particular, it is not currently
possible to use `rules_rust` and certain versions of `upb` (downstream
of protobuf) in the same repository, due to their definition here:

https://github.com/protocolbuffers/upb/blob/c1357afb2e39df671d89eaec49033b5329f36a3e/bazel/repository_defs.bzl#L7-L10

An easy workaround is to disambiguate the name. It looks much easier to
change `rules_rust` than to change `upb` and update its long chain of
workspace dependencies, hence this patch. :-)

See my comment on #268 for more details and a full repro:
#268 (comment)

[i8305]: bazelbuild/bazel#8305

Test Plan:
Tested by adding the README’s workspace stanza to TensorBoard (at
current master, 8d629954c251). It fails at `rules_rust = 5998baf`,
but succeeds with a `local_repository` that has this patch. Also, in
this repo, `git grep @bazel_version` no longer has any matches.

wchargin-branch: disambiguate-bazel-version-repo
wchargin-source: e712603c3a3ebc45364cc2dcc9eca341301b5197
@keith
Copy link
Member

keith commented Mar 9, 2022

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. #14996

@rickeylev
Copy link
Contributor

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).

@Wyverald
Copy link
Member

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

@fmeum
Copy link
Collaborator

fmeum commented Nov 28, 2022

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.

@Wyverald
Copy link
Member

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.

@chancila
Copy link
Contributor

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?

@fmeum
Copy link
Collaborator

fmeum commented Dec 16, 2022

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.

phst added a commit to phst/rules_elisp that referenced this issue Dec 14, 2023
phst added a commit to phst/rules_elisp that referenced this issue Dec 14, 2023
Copy link

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 github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 20, 2024
@fmeum
Copy link
Collaborator

fmeum commented Feb 20, 2024

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 fmeum closed this as completed Feb 20, 2024
@fmeum fmeum closed this as not planned Won't fix, can't repro, duplicate, stale Feb 20, 2024
@fmeum fmeum closed this as completed Feb 20, 2024
@haberman
Copy link
Contributor Author

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

@fmeum
Copy link
Collaborator

fmeum commented Feb 28, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Starlark-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.