Bazel: CI projects are failing with bazel 0.10.0 RC1 due to wrongly identifying the current Bazel version

[iirina]

rules_closure fails to build:

ERROR: /Users/ci/workspace/Global/rules_closure-node=darwin-x86_64/WORKSPACE:6:1: Traceback (most recent call last):

    File "/Users/ci/workspace/Global/rules_closure-node=darwin-x86_64/WORKSPACE", line 6

        closure_repositories()

    File "/Users/ci/workspace/Global/rules_closure-node=darwin-x86_64/closure/repositories.bzl", line 69, in closure_repositories

        _check_bazel_version("Closure Rules", "0.4.5")

    File "/Users/ci/workspace/Global/rules_closure-node=darwin-x86_64/closure/repositories.bzl", line 172, in _check_bazel_version

        fail(("%s requires Bazel >=%s but was...)))

Closure Rules requires Bazel >=0.4.5 but was 0.10.0rc1

ERROR: Error evaluating WORKSPACE file

ERROR: error loading package 'external': Package 'external' contains errors

INFO: Elapsed time: 0.522s

FAILED: Build did NOT complete successfully (0 packages loaded)

The console output is here.

This is blocking the 0.10.0 release (#3958).

[iirina]

This is also affecting Tensorflow (see pipeline).

[iirina]

... and for rules_webtesting: pipeline

and gerrit: pipeline

[iirina]

I wonder how these projects were built before, with the previous release candidates. @laszlocsomor do you have any experience with this?

[jin]

For rules_closure:

def _check_bazel_version(project, bazel_version):
  if "bazel_version" not in dir(native):
    fail("%s requires Bazel >=%s but was <0.2.1" % (project, bazel_version))
  elif not native.bazel_version:
    pass  # user probably compiled Bazel from scratch
  else:
    current_bazel_version = _parse_bazel_version(native.bazel_version)
    minimum_bazel_version = _parse_bazel_version(bazel_version)
    if minimum_bazel_version > current_bazel_version:
      fail("%s requires Bazel >=%s but was %s" % (
          project, bazel_version, native.bazel_version))

def _parse_bazel_version(bazel_version):
  # Remove commit from version.
  version = bazel_version.split(" ", 1)[0]
  # Split into (release, date) parts and only return the release
  # as a tuple of integers.
  parts = version.split("-", 1)
  # Turn "release" into a tuple of strings
  version_tuple = ()
  for number in parts[0].split("."):
    version_tuple += (str(number),)
  return version_tuple

>>> _parse_bazel_version("0.10.0-rc1 $commit") > _parse_bazel_version("0.4.5 $commit")
False

There's a bug in the version comparison algorithm.

[iirina]

cc: @jart could you take a look at parsing the bazel version in rules_closure?

[iirina]

cc: @frankchn @sanjoy could you take a look at parsing the bazel version in tensorflow/workspace.bzl?

[iirina]

cc: @DrMarcII could you take a look at parsing the bazel version in rules_webtesting?

[iirina]

cc: @davido could you take a look at parsing the bazel version for gerrit?

[DrMarcII]

This can be fixed by parsing bazel version numbers into a tuple of ints instead of a tuple of strings.

[sanjoy]

I think a quick fix for TensorFlow would also involve what @DrMarcII said above -- return ints instead of strings. However, before I make that change -- is there some canonical way of parsing and/or comparing bazel versions? For instance, changing the str(number) to int(number) works for "0.10.0-rc1 $commit" but not for "0.10.0rc1 $commit".

[jayconrod]

I propose the function below:

# Parse the bazel version string from `native.bazel_version`.
# For example, "0.10.0-rc1 0123abc"
def _parse_bazel_version(bazel_version):
  for i in range(len(bazel_version)):
    c = bazel_version[i]
    if not (c.isdigit() or c == "."):
      bazel_version = bazel_version[:i]
      break
  return tuple([int(n) for n in bazel_version.split(".")])

Find the first character in the version string which is not a digit or '.', and ignore that character and everything after. Split the remaining string on '.', and convert the pieces to integer.

[jin]

I think a quick fix for TensorFlow would also involve what @DrMarcII said above -- return ints instead of strings. However, before I make that change -- is there some canonical way of parsing and/or comparing bazel versions? For instance, changing the str(number) to int(number) works for "0.10.0-rc1 $commit" but not for "0.10.0rc1 $commit".

I think we should format the Bazel version for RCs to include the dash for easier downstream parsing.

[ixdy]

This is also going to break the kubernetes build, since we copy-pasted the same function. :(

We'll apply whatever fix is discovered here, but it'd be handy if there were a canonical version of this check function, rather than every single project copying their own...

[jin]

I can update https://github.com/bazelbuild/bazel-integration-testing/blob/master/bazel_version.bzl to use @jayconrod's function, and have that file bundled in bazelbuild/bazel-skylib as the canonical version. What does everyone think?

e.g. in WORKSPACE

load("@bazel_skylib//:lib.bzl", "check_bazel_version")
check_bazel_version("0.5.0")

[frankchn]

We are happy to include any fix discovered here too, and +1 on a canonical version of the version parsing function.

[davido]

@iirina I checked and it still works on Bazel@HEAD.

What we are doing is:

WORKSPACE:

load("//:version.bzl", "check_version")
check_version("0.5.3")

version.bzl:

def check_version(x):
    if native.bazel_version == "":
        # experimental / unreleased Bazel.
        return
    if native.bazel_version < x:
        fail("\nERROR: Current Bazel version is {}, expected at least {}\n".format(native.bazel_version, x))

Ah, my bad, i tested against unrelased version. Will have a look.

[jart]

I'm going to be exporting a change shortly for rules_closure where I just remove the version check.

[jin]

Once https://github.com/bazelbuild/bazel-skylib/pull/13 is in, rules can use this:

load("@bazel_skylib//lib:versions.bzl", "versions")
versions.check("1.0.0")

which results in

```ERROR: /.../WORKSPACE:4:1: Traceback (most recent call last):
File "/.../WORKSPACE", line 4
versions.check("1.0.0")
File "/.../61c1b9f6a4f1216adfc3c13c53288b50/external/bazel_skylib/lib/versions.bzl", line 60, in versions.check
fail("\nCurrent Bazel version is {}, ...))

Current Bazel version is 0.9.0, expected at least 1.0.0
```

[laszlocsomor]

I wonder how these projects were built before, with the previous release candidates. @laszlocsomor do you have any experience with this?

No I don't, but as others already noticed, the bug is with parsing the version number into a string instead of an int.

[meteorcloudy]

I wonder how these projects were built before, with the previous release candidates. @laszlocsomor do you have any experience with this?

@iirina We didn't embed bazel version in previous release candidates. When it's empty, the function ignores it in case users use a custom built Bazel.

[iirina]

@meteorcloudy Thank you, that explains it.

Thanks all for looking into this! Can someone confirm for these projects that the issue was fixed? Please tick the boxes, thanks!

• [x] rules_closure
• [x] Tensorflow
• [x] rules_webtesting
• [x] gerrit
• [x] rules_k8s

[ixdy]

Was rules_k8s directed at me? If so, I was referring to the kubernetes/kubernetes and kubernetes/test-infra projects. I was waiting on https://github.com/bazelbuild/bazel-skylib/pull/13 to be merged, and now that it has, I'll update these projects.

[jayconrod]

If you're updating bazel-skylib for this check, make sure to update to bazelbuild/bazel-skylib#14. There was a small bug in bazelbuild/bazel-skylib#13 that caused the version check to fail for source builds of Bazel. It shouldn't affect official releases or RCs though.

[davido]

Gerrit Code Review was fixed: https://gerrit-review.googlesource.com/#/c/gerrit/+/152290.

[iirina]

Thanks all.

Tensorflow_Serving also needs a fix (see log).

[iirina]

Gerrit still fails with this error because of the rules_closure repository. It probably needs to update the rules_closure version it is using.

Log

cc @davido

[jart]

rules_closure is updated at head. Still needs to trickle down into the other repos.

[davido]

Gerrit still fails with this error because of the rules_closure repository. It probably needs to update the rules_closure version it is using.

Done in: [1], the CL is pending for review and library compliance approval.

• [1] https://gerrit-review.googlesource.com/152297

[iirina]

Thank you @davido!

[meteorcloudy]

@jart @iirina I'm sending a PR to update rules_closure in TF repo.
https://github.com/tensorflow/tensorflow/pull/16157

[sanjoy]

I have checked in a fix to TensorFlow's workspace.bzl which should make it out to the open source repository in the next sync.

[jin]

@sanjoy I recommend following what Kubernetes is doing, instead of re-implementing the version check https://github.com/kubernetes/kubernetes/commit/d8f6febc7d2ea5c045dd93ff9ec7a06f11cdf2a5

[davido]

@jin ... and gerrit https://github.com/GerritCodeReview/gerrit/commit/2e01c36cecdf92a0209b998ed0f9bb63d33219b8.

[sanjoy]

@jin @gunan suggested not adding a new dependency for parsing.

[jart]

I agree. I just removed the version check entirely in rules_closure. Bazel is significantly more stable these days than when I added it back in 2016.

[davido]

I just removed the version check entirely in rules_closure. Bazel is significantly more stable these days than when I added it back in 2016

Not every Bazel version update is backwards compatible. Skylark will evolve, some methods will be declared deprecated or even discontinued, you will refactor your build tool chain. Especially with big community and user base (Gerrit Code Review) you will inevitably end up with obscure error messages during the build. Minimum supported Bazel version check is very helpful in this context.

I have better plans for my spare free time, then troubleshoot these error reports: [1], that was caused by replacing deprecated ctx.action() with new ctx.actions.run() / ctx.actions.run_shell() in this CL: [2].

$ bazel build polygerrit
    ERROR: /gerrit/lib/js/BUILD:27:1: in js_component rule //lib/js:highlightjs: 
    Traceback (most recent call last):
            File "/gerrit/lib/js/BUILD", line 27
                    js_component(name = 'highlightjs')
            File "/code/gerrit/tools/bzl/js.bzl", line 170, in _js_component
                    ctx.actions
    object of type 'ctx' has no field 'actions'.
    ERROR: Analysis of target '//:polygerrit' failed; build aborted.
    INFO: Elapsed time: 4.245s

[1] https://bugs.chromium.org/p/gerrit/issues/detail?id=7123
[2] https://gerrit-review.googlesource.com/123193

[jart]

Not every Bazel version update is backwards compatible. Skylark will evolve, some methods will be declared deprecated or even discontinued, you will refactor your build tool chain.

Only until Bazel drops a 1.x release.

Internally at Google, if I rewind the repository to a few years ago, it auto bootstraps whatever version of Blaze was being used at the time. That ensures history doesn't go down the memory hole when stuff evolves, e.g. depset.

Bazel can deliver that same level of historical integrity to your codebase, if and only if you pin and mirror dependencies. The problem is the only thing Bazel can't pin right now is itself.

So I hope 1.x comes soon and the team makes a commitment to backwards compatibility forevermore.

[davido]

So I hope 1.x comes soon and the team makes a commitment to backwards compatibility forevermore.

While I see your point about backwards compatibility and 1.x releases, what about incompatible changes in your build tool chain? Even such innocent PR "Consume java_import_external.bzl from bazel core", like my attempt de-duplicate java_import_external rule from rules_closure and consume it from Bazel core (that you added to Bazel core recently) would be problematic to merge, as it would certainly break users without minimal supported Bazel version check.

Unfortunately the error message that Bazel issued here is not really helpful if you try to use my PR with outdated Bazel version, as it was the case with Travis CI in rules_closure repository, that tried to verify my PR.

[laszlocsomor]

Version check fails on rules_nodejs too: https://ci.bazel.build/blue/rest/organizations/jenkins/pipelines/Global/pipelines/rules_nodejs/runs/179/nodes/19/log/?start=0

I'm investigating if it's the same problem as this bug. From the way it looks, i guess it is.

[jart]

@davido That would only be a breaking change. It wouldn't be a memory-hole change.

Assuming rules_closure users are doing what I've always encouraged them to do (pin CR in WORKSPACE w/ sha + redundant mirrors) then me merging your PR would only be a minor inconvenience for users, when they choose to upgrade to the latest rules_closure.

That doesn't concern me so much. Old revisions of the user repo will still build. The changes that terrify me are the "memory-hole changes." Just one memory-hole change is all it takes to break the build at all historical revisions. For example, since @bazel_tools can't be pinned, a part of me almost wonders if I shouldn't have upstreamed java_import_external. Because if that file ever got renamed, it would be a memory-hole change.

[laszlocsomor]

Version check fails on rules_nodejs too: https://ci.bazel.build/blue/rest/organizations/jenkins/pipelines/Global/pipelines/rules_nodejs/runs/179/nodes/19/log/?start=0

I'm investigating if it's the same problem as this bug. From the way it looks, i guess it is.

No, this failure is unrelated. (It's about a Python 2 vs. 3 difference.)

[davido]

Done in: [1], the CL is pending for review and library compliance approval.

The CL above was merged, so that Gerrit job on Bazel-CI should be green again.

[iirina]

@kirilg Can you update Tensorflow_serving to use the latest method of checking the bazel version in tensorflow:workspace.bzl? Note that it was renamed from check_version to check_bazel_version_at_least. It is used in tensorflow/serving/WORKSPACE

[kirilg]

Done :)

TensorFlow Serving was passing even before with the version of TensorFlow that we have pinned as a submodule. Note that if your test manually updates the TF submodule to point to head (what I assume happened to trigger this error), the test will often be broken. Instead please rely on the commit that we use in our repo which is guaranteed to be compatible with TensorFlow Serving at that time.

[iirina]

Thank you all for contributing in cleaning this up!

[jimbojw]

What should projects that use bazel do in the face of this bug? We're seeing this behavior now that 0.10.0 is out in the wild: https://github.com/PAIR-code/facets/issues/107

[davido]

@jimbojw You have three options, obviously:

• Remove the check entirely, something that rules_closure project is doing
• Replace it with the check from bazel_skylib, something that gerrit code review project is doing https://github.com/GerritCodeReview/gerrit/blob/master/WORKSPACE#L31-L33
• Reimplement the check on your own, something that TensorFlow project is doing: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/workspace.bzl#L15-L60

[fahhem]

@davido, there is another option for version checks:

https://github.com/fahhem/protobuf/blob/35119e39a07f426f3c48a1bfb41d862994223742/protobuf.bzl#L405

The documentation is in the java implementation (not in the skylark docs yet): https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/apple/DottedVersion.java

[davido]

@fahhem Thanks for pointing this out. I didn't know that. Still strange to use apple_common on Linux system ;-)

def check_protobuf_required_bazel_version():
  """For WORKSPACE files, to check the installed version of bazel.

  This ensures bazel supports our approach to proto_library() depending on a
  copied filegroup. (Fixed in bazel 0.5.4)
  """
  expected = apple_common.dotted_version("0.5.4")
  current = apple_common.dotted_version(native.bazel_version)
  if current.compare_to(expected) < 0:
    fail("Bazel must be newer than 0.5.4")

[kevinashaw]

So how should we proceed? We can't compile with the latest Bazel (0.11.0).
We need to compile Tensorflow to a .so for Ubuntu.
Should we downgrade to pre-0.10.0 builds and compile there?

[meteorcloudy]

@kevinashaw, if you build TensorFlow@HEAD with Bazel 0.11.0, it should work.
For previous TF version, you can cherry-pick https://github.com/tensorflow/tensorflow/commit/3f57956725b553d196974c9ad31badeb3eabf8bb and https://github.com/tensorflow/tensorflow/commit/6fcfab770c2672e2250e0f5686b9545d99eb7b2b before compiling.