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

Bump protobuf to 3.10.0 and apply fixes for --incompatible_load_{java|proto|python}_rules_from_bzl #413

Merged
merged 1 commit into from
Oct 7, 2019

Conversation

davido
Copy link
Contributor

@davido davido commented Sep 7, 2019

The JS tests are failing:

  $ bazel test closure/...
ERROR: /home/davido/projects/davido_rules_closure/closure/templates/test/BUILD:105:1: Compiling 94 JavaScript files to closure/templates/test/greeter_proto_test_bin.js failed (Exit 1)
external/com_google_protobuf_js/binary/writer.js:239: ERROR - Bad type annotation. Unknown type goog.crypt.base64.Alphabet
 * @param {!goog.crypt.base64.Alphabet=} alphabet Which flavor of base64 to use.
            ^
  ProTip: "JSC_UNRECOGNIZED_TYPE_ERROR" or "checkTypes" or "unrecognizedTypeError" can be added to the `suppress` attribute of:
  //closure/protobuf:jspb
  Alternatively /** @suppress {checkTypes} */ can be added to the source file.

1 error(s), 0 warning(s), 97.2% typed
	
INFO: Elapsed time: 91.160s, Critical Path: 44.83s
INFO: 1074 processes: 164 linux-sandbox, 910 worker.
FAILED: Build did NOT complete successfully

That is defined here: [1] and that was added here: [2]. It seems that closure compiler has to be bumped as well.

[1] https://github.com/google/closure-library/blame/master/closure/goog/crypt/base64.js#L71
[2] https://github.com/google/closure-library/releases/tag/v20190729

@davido
Copy link
Contributor Author

davido commented Sep 7, 2019

//CC @Yannic @gkdn

Copy link
Contributor

@Yannic Yannic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Why did you add java/io/bazel/rules/closure/Webpath.class, closure/v3.8.0.tar.gz and closure/v3.8.0.tar.gz.1? Was it an accident, or am I missing something?

@@ -1,11 +1,12 @@
# DO NOT EDIT -- bazel run //closure/library:regenerate -- "$PWD"

load("@rules_python//python:defs.bzl", "py_binary")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add this load here [1] as well? Otherwise, it'll be reverted the next time closure-library is pumped.

[1]

builds[build].insert(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

strip_prefix = "rules_python-4b84ad270387a7c439ebdccfd530e2339601ef27",
urls = ["https://github.com/bazelbuild/rules_python/archive/4b84ad270387a7c439ebdccfd530e2339601ef27.tar.gz"],
)

def zlib():
http_archive(
name = "zlib",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you replace this with build_file = "@com_google_protobuf//:third_party/zlib.BUILD" and delete the local copy of this file? The one from Protobuf has the necessary load statements for rules_cc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -69,6 +69,8 @@ def closure_repositories(
omit_org_ow2_asm_tree = False,
omit_org_ow2_asm_util = False,
omit_phantomjs = False,
omit_rules_proto = False,
omit_rules_python = False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also include rules_{cc,java}. They are required by the native rules, so they are implicit dependencies, but I'm not sure how long they'll stay that way.

Copy link
Contributor Author

@davido davido Sep 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather not add them in this PR, but prefer to add the later. In Gerrit we have also consumed Java rules from Starlark, and not fetched java rules in WORKSPACE file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think they'll be available at least during 1.x, so that shouldn't be an issue for the next 3+ month.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think rules_{cc,java} is only available in Bazel 0.29+. Please add them in this PR.

With Bazel 0.28.1, I'm seeing the following error:

ERROR: error loading package 'closure/templates': Unable to find package for @rules_java//java:defs.bzl: The repository '@rules_java' could not be resolved.
INFO: Elapsed time: 0.211s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (3 packages loaded)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added rules_{cc,java}. Done.

@@ -0,0 +1,13 @@
java_library(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -0,0 +1,23 @@
// Copyright 2016 The Closure Rules Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this added (dito *.java~)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@Yannic Yannic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % test failures

@@ -69,6 +69,8 @@ def closure_repositories(
omit_org_ow2_asm_tree = False,
omit_org_ow2_asm_util = False,
omit_phantomjs = False,
omit_rules_proto = False,
omit_rules_python = False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think they'll be available at least during 1.x, so that shouldn't be an issue for the next 3+ month.

@davido
Copy link
Contributor Author

davido commented Sep 14, 2019

@Yannic @gkdn @dslomov @laurentlb

I rebased this PR on recent master and squashed all commits. PTAL.

@davido
Copy link
Contributor Author

davido commented Sep 14, 2019

OK, there is one complication. I conducted custom rules_closure release from this commit here: [1], and tried to consume this new rules_closure release in Gerrit project: [2]. The build is broken, and it seems that the same problem shows up that I already reported here (but that I was able to workaround in Gerrit build tool chain, by moving one java_import invocation from Starlark file to BUILD file): [3]:

  $ bazel build :release
INFO: Writing tracer profile to '/home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/command.profile.gz'
INFO: Invocation ID: 6cb4e17d-97db-4bff-b824-5342fe18a6b2
ERROR: Failed to load Starlark extension '@rules_java//java:defs.bzl'.
Cycle in the workspace file detected. This indicates that a repository is used prior to being defined.
The following chain of repository dependencies lead to the missing definition.
 - @rules_java
This could either mean you have to add the '@rules_java' repository with a statement like `http_archive` in your WORKSPACE file (note that transitive dependencies are not added automatically), or move an existing definition earlier in your WORKSPACE file.
ERROR: cycles detected during target parsing
INFO: Elapsed time: 0.489s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)

Bazel version is Bazel@HEAD, built from this commit (6470bb74047a76d3e0271273ac0864b697ab2aa9).

[1] https://github.com/davido/rules_closure/releases/tag/v0.25
[2] https://gerrit-review.googlesource.com/c/gerrit/+/237186
[3] bazelbuild/bazel#9291

Copy link
Contributor

@Yannic Yannic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Travis is going to need a bit of extra love because it's using Bazel 0.22.0.

Otherwise, LGTM

@Yannic
Copy link
Contributor

Yannic commented Sep 14, 2019

@davido Ok, I was able to reproduce that locally with Gerrit. I think it's caused by the fact that we tell users to use load("@io_bazel_rules_closure//closure:defs.bzl", "closure_repositories") which also exports our rules, hence has load statements for repos we haven't declared yet (and is against Bazel's rule guide).

The reason this doesn't fail on our CI is that we use load("@io_bazel_rules_closure//closure:repositories.bzl", "closure_repositories"). Switching Gerrit to that one also seems to fix Gerrit.

Fixing this is going to be a bit of a pain because it's an incompatible change.
I propose the following migration plan:

  • Create rules_closure_{dependencies,toolchains} in closure/repositories.bzl to conform with the rule guide (and update readme).
  • Make closure_repositories in closure/defs.bzl a macro that prints a warning to tell users to switch to rules_closure_{dependencies,toolchains}.
  • Cut a release.
  • Land this PR, Gerrit can consume rules_closure at head.
  • 2019-11-01: Remove closure_repositories from closure/defs.bzl and cut a new release.

@davido
Copy link
Contributor Author

davido commented Sep 15, 2019

@Yannic, I added this PR, where I refactored the consumption of rules_closure dependencies.

@davido
Copy link
Contributor Author

davido commented Sep 19, 2019

Can this now be merged, or do we waiting for something?

@Yannic
Copy link
Contributor

Yannic commented Sep 19, 2019

@davido Can you rebase to resolve the merge conflict and update Travis to 0.29.1? I think that's all that's blocking us from merging.

@gkdn @dslomov Please cut a new release before merging this. There is a chance that this breaks users if they don't explicitly declare rules_{cc,java} (but merging this is the way forward).

@davido davido force-pushed the bump_protobuf_to_3.10.0-rc1 branch 2 times, most recently from 9c4d754 to cda1697 Compare September 19, 2019 18:17
@davido
Copy link
Contributor Author

davido commented Sep 19, 2019

@davido Can you rebase to resolve the merge conflict and update Travis to 0.29.1?

Done.

@davido
Copy link
Contributor Author

davido commented Sep 19, 2019

@Yannic The build on Mac OS slave is failing:

ERROR: /Users/travis/build/bazelbuild/rules_closure/closure/compiler/test/closure_js_deps/BUILD:66:1: Calculating 39 JavaScript deps to closure/compiler/test/closure_js_deps/goblin_deps.js failed (Exit 1) depswriter failed: error executing command 
  (cd /Users/travis/.cache/bazel/sandbox/darwin-sandbox/289/execroot/io_bazel_rules_closure && \
  exec env - \
  bazel-out/host/bin/external/com_google_javascript_closure_library/depswriter '--output_file=bazel-out/darwin-fastbuild/bin/closure/compiler/test/closure_js_deps/goblin_deps.js' '--root_with_prefix=closure/compiler/test/closure_js_deps ../../../io_bazel_rules_closure/closure/compiler/test/closure_js_deps' '--root_with_prefix=external/com_google_javascript_closure_library/closure/goog .')
Execution platform: @bazel_tools//platforms:host_platform
Use --sandbox_debug to see verbose messages from the sandbox
[365 / 370] .../compiler/test/closure_js_deps:hyperion2_lib; 0s darwin-sandbox
pting to use the default Python toolchain (@rules_python//python:autodetecting_toolchain).
According to '/usr/local/bin/python -V', version is 'Python 2.7.12', but we need version 3. PATH is:
/usr/gnu/bin:/usr/local/bin:/bin:/usr/bin:.
Please ensure an interpreter with version 3 is available on this platform as 'python3' or 'python', or else register an appropriate Python toolchain as per the documentation for py_runtime_pair (https://github.com/bazelbuild/rules_python/blob/master/docs/python.md#py_runtime_pair).
Note that prior to Bazel 0.27, there was no check to ensure that the interpreter's version matched the version declared by the target (#4815). If your build worked prior to Bazel 0.27, and you're sure your targets do not require Python 3, you can opt out of this version check by using the non-strict autodetecting toolchain instead of the standard autodetecting toolchain. This can be done by passing the flag `--extra_toolchains=@rules_python//python:autodetecting_toolchain_nonstrict` on the command line or adding it to your bazelrc.
----------------
Note: The failure of target @com_google_javascript_closure_library//:depswriter (with exit code 1) may have been caused by the fact that it is running under Python 3 instead of Python 2. Examine the error to determine if that appears to be the problem. Since this target is built in the host configuration, the only way to change its version is to set --host_force_python=PY2, which affects the entire build.
If this error started occurring in Bazel 0.27 and later, it may be because the Python toolchain now enforces that targets analyzed as PY2 and PY3 run under a Python 2 and Python 3 interpreter, respectively. See https://github.com/bazelbuild/bazel/issues/7899 for more information.

I remember we had a similar issue in gerrit and solution was to add the following options in .bazelrc:

build --action_env=PATH
build --incompatible_strict_action_env

But note, that problem occurred on Gerrit@Bazel CI (Buildkite), so I am not sure about Travis CI, that is used here.

//CC @brandjon

@davido
Copy link
Contributor Author

davido commented Sep 19, 2019

@brandjon @Yannic

I added .bazelrc wiht the following content:

build --action_env=PATH
build --incompatible_strict_action_env

Let's see, what happens.

@Yannic
Copy link
Contributor

Yannic commented Sep 19, 2019

Yeah, Bazel 0.25 changed the default version of Python, which I think is why we're seeing this.

I'd rather not set any --incompatible_* flag. Feel free to try setting lang: python3.
https://github.com/bazelbuild/rules_closure/pull/422/files#diff-354f30a63fb0907d4ad57269548329e3R6

@davido
Copy link
Contributor Author

davido commented Sep 19, 2019

Yeah, Bazel 0.25 changed the default version of Python, which I think is why we're seeing this.

Ah, OK. I removed .bazelrc again and added Python 3 to Travis instead, as suggested.

@brandjon
Copy link
Member

@davido's error log indicates that your build is trying to run a py_binary (depswriter) in the host config. By default, the host config uses PY3 (regardless of python_version). This fails at execution time because your machine apparently does not have a python3 interpreter.

So presumably, you don't want to be using Python 3 in your build. You can change the default version used by the host config to PY2 by passing --host_force_python=PY2 on the command line.

If for some reason you can't do that -- for instance, if you have targets that depend on Bazel thinking that they're PY3 (in select()s) when they're really not -- you can opt into the old behavior of not enforcing the declared Python version at execution time by using the nonstrict toolchain, as mentioned in the error message. I'd avoid that.

@davido
Copy link
Contributor Author

davido commented Sep 19, 2019

@brandjon Thanks for your comment. Even after adding language: python3 to the .travis.yml file the Python 3 is still not detected on Mac OS; just check the very last log.

I remember one issue in Gerrit where build --action_env=PATH had to be added to the .bazelrc file to detect Python 3 on Mac OS on Bazel-CI. I just added .bazelrc with just that content.

@Yannic
Copy link
Contributor

Yannic commented Sep 19, 2019

Sorry, my bad. The correct way to declare Python 3 is language: python. It seems like 3.6 is the default version for Travis.
https://docs.travis-ci.com/user/languages/python/

If this also doesn't work, --host_force_python=PY2 is an alternative. There are no select()s that assume PY3.

@davido
Copy link
Contributor Author

davido commented Sep 19, 2019

The correct way to declare Python 3 is language: python

OK, changed.

@Yannic
Copy link
Contributor

Yannic commented Sep 19, 2019

Sorry for the repeated changes, fixing Travis really isn't fun. Please also remove dist: trusty (Ubuntu 14.04 is EOL, default is 16.04) and osx_image: xcode8 (macOS 10.11 doesn't have py3.6, default should have it).

@brandjon
Copy link
Member

Er, my previous answer was predicated on the assumption that this target is written for PY2 and will only run under PY2. If you actually want it to run under PY3, then presumably the CI config just needs to ensure Python 3 is available. Note that /usr/local/bin is already on PATH according to the error log, so if python3 is installed it should find it.

@davido
Copy link
Contributor Author

davido commented Sep 19, 2019

@brandjon @Yannic Thanks! I addressed all comments. Hopefully, the next build should succeed.

@Yannic
Copy link
Contributor

Yannic commented Sep 19, 2019

Ok, we're at the point where I'm out of ideas how to make Travis happy. If you want to remove osx, I won't complain. Buildkite is covering it.

Thank you for your patience!

@davido
Copy link
Contributor Author

davido commented Sep 19, 2019

If you want to remove osx, I won't complain. Buildkite is covering it.

Done.

@Yannic
Copy link
Contributor

Yannic commented Oct 2, 2019

@gkdn @dslomov What's the status on this? Can you cut a release and then merge this?

urls = [
"https://mirror.bazel.build/github.com/google/protobuf/archive/v3.9.0.tar.gz",
"https://github.com/protocolbuffers/protobuf/archive/v3.9.0.tar.gz",
# "https://mirror.bazel.build/github.com/google/protobuf/archive/v3.10.0-rc1.tar.gz",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commented out mirror URL can be improved here and in other place.

@meteorcloudy @laurentlb Can you please upload https://github.com/protocolbuffers/protobuf/archive/v3.10.0-rc1.tar.gz to https://mirror.bazel.build/github.com/google/protobuf/archive/v3.10.0-rc1.tar.gz

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like 3.10.0 was just released yesterday. maybe the -rc1 suffix can be dropped?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for working on this btw!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for letting me know, I will amend this series and bump right to 3.10.0.

@meteorcloudy @laurentlb Could you please upload https://github.com/protocolbuffers/protobuf/archive/v3.10.0.tar.gz to bazel mirror? Thank you in advance!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to everyone for help!

@gkdn @laurentlb @dslomov PTAL, it's ready now.

@davido davido force-pushed the bump_protobuf_to_3.10.0-rc1 branch from a6f9801 to 589fe8d Compare October 4, 2019 22:38
@davido davido changed the title Bump protobuf to 3.10.0 rc1 and apply fixes for --incompatible_load_{java|proto|python}_rules_from_bzl Bump protobuf to 3.10.0 and apply fixes for --incompatible_load_{java|proto|python}_rules_from_bzl Oct 4, 2019
package(default_visibility = ["//visibility:public"])

licenses(["notice"])

load("//closure:defs.bzl", "closure_css_library")
load("//closure:defs.bzl", "closure_js_library")
load("//closure:defs.bzl", "closure_css_library", "closure_js_library")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls move next to other load

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -12,6 +12,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.

load("@rules_proto//proto:defs.bzl", "ProtoInfo")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls keep loads together. and for others as well

Copy link
Contributor Author

@davido davido Oct 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Here and in all other places. To my defense, I should mention, that the changes were done by buildifier and I haven't edited/moved the new introduced load statements to group them together with the existing ones.


os:
- linux
- osx
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no more osx? I don't have the context.

Copy link
Contributor Author

@davido davido Oct 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these statements in the commit message not enough?

This change also does the following:
[...]
* Bump Bazel version to 0.29.1 in .travis.yml file
[...]
Travis changes:
    
    * Remove dist: trusty (Ubuntu 14.04 is EOL, default is 16.04)
    * Remove osx OS as it currently doesn't support Python: [1].

[1] https://github.com/travis-ci/travis-ci/issues/2312

As pointed out by @Yannic OSX is verified in Buildkite pipeline already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's verified by Buildkite (and Travis is super slow), so there's no harm in removing it.

See also #422 (comment)

@davido davido force-pushed the bump_protobuf_to_3.10.0-rc1 branch from 589fe8d to bd812a4 Compare October 5, 2019 05:04
This change also does the following:

* Add fixes for --incompatible_load_{java|proto|python}_rules_from_bzl
* Consume zlib.BUILD from protobuf repository
* regenerate.py: load py_binary from rules_python
* Add rules_cc and rules_java
* Bump Bazel version to 0.29.1 in .travis.yml file

Travis changes:

* Remove dist: trusty (Ubuntu 14.04 is EOL, default is 16.04)
* Remove osx OS as it currently doesn't support Python: [1].

[1] travis-ci/travis-ci#2312
@davido
Copy link
Contributor Author

davido commented Oct 7, 2019

@laurentlb Can you please approve this PR?

@gkdn gkdn merged commit d53c0d7 into bazelbuild:master Oct 7, 2019
@gkdn
Copy link
Collaborator

gkdn commented Oct 7, 2019

I merged the patch. I also cut a new release before merging.

@davido
Copy link
Contributor Author

davido commented Oct 7, 2019

Thanks!

@Yannic
Copy link
Contributor

Yannic commented Oct 7, 2019

Thanks! I'll send a follow-up of #423 to update the readme / getting started section tomorrow.

lucamilanesio pushed a commit to GerritCodeReview/gerrit that referenced this pull request Oct 8, 2019
The latest version updates protobuf version to 3.10.0: [1].

[1] bazelbuild/rules_closure#413

Change-Id: Ib0aecd23a95f8834f3d06edd66af76a475d0ebb5
sgammon pushed a commit to Bloombox/rules_closure that referenced this pull request Oct 17, 2019
This change also does the following:

* Add fixes for --incompatible_load_{java|proto|python}_rules_from_bzl
* Consume zlib.BUILD from protobuf repository
* regenerate.py: load py_binary from rules_python
* Add rules_cc and rules_java
* Bump Bazel version to 0.29.1 in .travis.yml file

Travis changes:

* Remove dist: trusty (Ubuntu 14.04 is EOL, default is 16.04)
* Remove osx OS as it currently doesn't support Python: [1].

[1] travis-ci/travis-ci#2312
ptmphuong pushed a commit to ptmphuong/rules_closure that referenced this pull request Dec 9, 2022
This change also does the following:

* Add fixes for --incompatible_load_{java|proto|python}_rules_from_bzl
* Consume zlib.BUILD from protobuf repository
* regenerate.py: load py_binary from rules_python
* Add rules_cc and rules_java
* Bump Bazel version to 0.29.1 in .travis.yml file

Travis changes:

* Remove dist: trusty (Ubuntu 14.04 is EOL, default is 16.04)
* Remove osx OS as it currently doesn't support Python: [1].

[1] travis-ci/travis-ci#2312
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants