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

Make the bazel rules work with current rules_closure. #368

Merged
merged 5 commits into from
Nov 13, 2018
Merged

Make the bazel rules work with current rules_closure. #368

merged 5 commits into from
Nov 13, 2018

Conversation

danfabi
Copy link
Contributor

@danfabi danfabi commented Nov 10, 2018

This commit introduced a helper function for creating closure_js_library.

@stanley-cheung
Copy link
Collaborator

@Yannic Please review, thanks!

@stanley-cheung
Copy link
Collaborator

There are test failures:

+ bazel test //javascript/net/grpc/web/... //net/grpc/gateway/examples/...
Starting local Bazel server and connecting to it...
...........................
Loading:
Loading: 0 packages loaded
Loading: 0 packages loaded
ERROR: /tmpfs/src/github/grpc-web/bazel/closure_grpc_web_library.bzl:5:1: file '@io_bazel_rules_closure//closure/compiler:closure_js_library.bzl' does not contain symbol 'create_closure_js_library'
ERROR: error loading package 'net/grpc/gateway/examples/echo': Extension file 'bazel/closure_grpc_web_library.bzl' has errors

@danfabi
Copy link
Contributor Author

danfabi commented Nov 11, 2018

Forgot to update rules_closure in WORKSPACE. Tests should pass now.

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 the fix, (mostly) LGTM.

Could we remove _ClosureWorker and _closure_library_base from attrs and replace it with CLOSURE_JS_TOOLCHAIN_ATTRS like it's done in closure_js_library?
https://github.com/bazelbuild/rules_closure/blob/master/closure/compiler/closure_js_library.bzl#L431

@danfabi
Copy link
Contributor Author

danfabi commented Nov 11, 2018

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

@stanley-cheung
Copy link
Collaborator

Sorry, still some test failures:

ERROR: /tmpfs/src/github/grpc-web/javascript/net/grpc/web/BUILD.bazel:148:1: in closure_js_binary rule //javascript/net/grpc/web:grpcwebstreamparser_test_bin:
Traceback (most recent call last):
	File "/tmpfs/src/github/grpc-web/javascript/net/grpc/web/BUILD.bazel", line 148
		closure_js_binary(name = 'grpcwebstreamparser_test_bin')
	File "/tmpfs/tmp/bazel/external/io_bazel_rules_closure/closure/compiler/closure_js_binary.bzl", line 212, in _impl
		all_args.add_all([src], map_each = get_jsfile_path, e...)
unexpected keyword 'expand_directories', in method call add_all(list, function map_each, bool expand_directories) of 'Args'
ERROR: Analysis of target '//javascript/net/grpc/web:grpcwebstreamparser_test' failed; build aborted: Analysis of target '//javascript/net/grpc/web:grpcwebstreamparser_test_bin' failed; build aborted

@Yannic
Copy link
Contributor

Yannic commented Nov 12, 2018

Cannot reproduce this with Bazel 0.19.0. That failure looks like it's caused by a slightly outdated and therefore incompatible version of Bazel.
@stanley-cheung Can you check which version is installed on the kokoro runner, and update it if necessary?

@stanley-cheung
Copy link
Collaborator

Looks like the kokoro machine is having bazel version 0.15.0.

@Yannic
Copy link
Contributor

Yannic commented Nov 12, 2018

Then it's clear why the tests fail. The expand_directories-argument was added to Bazel in 0.18.0.

There is more than one solution to resolve this:

  • You could update Bazel on kokoro,
  • We could modify the kokoro script to fetch the latest version of Bazel (maybe use bazelisk), or
  • We could ask some working on Bazel CI to add this repository to their system. This would bring the advantage that grpc-web will be tested with the latest version automatically (@buchgr, @damienmg, @philwo).

@stanley-cheung
Copy link
Collaborator

#373 should help.

@Yannic
Copy link
Contributor

Yannic commented Nov 13, 2018

@factuno-db We think that #373 will fix the failing tests. Please rebase when the other PR is merged so we can run the tests again, thanks!

@danfabi
Copy link
Contributor Author

danfabi commented Nov 13, 2018

Yeah, I saw it. Thanks for taking care of this!

@stanley-cheung
Copy link
Collaborator

@factuno-db #373 is merged. Please rebase so we can run the tests again. Thanks!

@danfabi
Copy link
Contributor Author

danfabi commented Nov 13, 2018

Done!

Copy link
Collaborator

@stanley-cheung stanley-cheung 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 the contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants