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 scala_protobuf aspect based #700

Merged
merged 34 commits into from
Mar 6, 2019
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
f1ae21f
Run a test with custom javabin
ianoc-stripe Feb 20, 2019
aca15a5
Respect java bin overrides
ianoc-stripe Feb 20, 2019
abf490b
Try set a different variable
ianoc-stripe Feb 20, 2019
bd3e18b
The action_should_fail tests have been broken :( , without assigning …
ianoc-stripe Feb 20, 2019
83f621c
Empty commit to trigger travis
ianoc-stripe Feb 20, 2019
73d7fbb
Make the scala_protobuf aspect based
ianoc-stripe Feb 25, 2019
444fc79
Use default toolchain?
ianoc-stripe Feb 25, 2019
a66e78b
Remove print
ianoc-stripe Feb 25, 2019
537d227
Remove prints
ianoc-stripe Feb 25, 2019
2a76e57
Remove println
ianoc-stripe Feb 25, 2019
8ecb252
Import files
ianoc-stripe Feb 25, 2019
82a46a3
Default produce services code
ianoc-stripe Feb 25, 2019
db9bd47
Support dependencies on scala
ianoc-stripe Feb 25, 2019
016a403
Add support for blacklisted protos
ianoc-stripe Mar 2, 2019
08c2261
Remove references to with_java
ianoc-stripe Mar 2, 2019
683d724
Provider
ianoc-stripe Mar 2, 2019
2a2c386
Code generator to toolchain
ianoc-stripe Mar 2, 2019
4e09ac9
Move the configuration options to the toolchain and add a comment
ianoc-stripe Mar 3, 2019
1b20ad5
Remove unused code
ianoc-stripe Mar 3, 2019
5ddb70c
Remove unused code, and get the classpath for compiling to not be the…
ianoc-stripe Mar 3, 2019
fbe0a10
Remove unused extra generator code for now
ianoc-stripe Mar 4, 2019
9495931
Using new bazel this works
ianoc-stripe Mar 5, 2019
53edf1c
Merge branch 'master' of github.com:ianoc-stripe/rules_scala
ianoc-stripe Mar 5, 2019
119ecbe
Merge branch 'master' into ianoc/shouldntCompileProto
ianoc-stripe Mar 5, 2019
b70967c
Merge branch 'master' of https://github.com/bazelbuild/rules_scala in…
ianoc-stripe Mar 5, 2019
5d3b4cb
Use the google protoc
ianoc-stripe Mar 5, 2019
86dd7b5
Add the override generator piping
ianoc-stripe Mar 5, 2019
85b9d4d
Show target overriding with a custom code generator
ianoc-stripe Mar 5, 2019
a47135d
Add 0.23 travis builds
ianoc-stripe Mar 6, 2019
ddf87f9
Some cleanup
ianoc-stripe Mar 6, 2019
215d89d
remove duplicated lines
ianoc-stripe Mar 6, 2019
31d70ad
Remove older targets from travis since they fail, update readme with …
ianoc-stripe Mar 6, 2019
e8574fe
Review comments
ianoc-stripe Mar 6, 2019
83914f1
Remove override generator paths
ianoc-stripe Mar 6, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ env:
#- V=0.16.1 TEST_SCRIPT=test_lint.sh
- V=0.17.1 TEST_SCRIPT=test_rules_scala.sh
- V=0.22.0 TEST_SCRIPT=test_rules_scala.sh
- V=0.23.1 TEST_SCRIPT=test_rules_scala.sh
#- V=0.14.1 TEST_SCRIPT=test_intellij_aspect.sh
- V=0.17.1 TEST_SCRIPT=test_reproducibility.sh
- V=0.22.0 TEST_SCRIPT=test_reproducibility.sh
- V=0.23.1 TEST_SCRIPT=test_reproducibility.sh

before_install:
- |
Expand Down
4 changes: 4 additions & 0 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ load("@io_bazel_rules_scala//scala:toolchains.bzl", "scala_register_unused_deps_

scala_register_unused_deps_toolchains()


register_toolchains("@io_bazel_rules_scala//test/proto:scalapb_toolchain")


load("//scala:scala_maven_import_external.bzl", "scala_maven_import_external", "java_import_external")

scala_maven_import_external(
Expand Down
38 changes: 38 additions & 0 deletions scala_proto/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
load("//scala_proto:scala_proto_toolchain.bzl", "scala_proto_toolchain")

toolchain_type(
name = "toolchain_type",
Copy link
Member

Choose a reason for hiding this comment

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

I freely admit to not understanding this stuff. It seems really weird to me the whole toolchain_type, toolchain, impl of the toolchain.... but I guess maybe they are mapping a typed system onto dynamic values in skylark.

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 don't really know about how much of this is required/needed tbh. I cargo culted this from the scala tool chain

visibility = ["//visibility:public"],
)

scala_proto_toolchain(
name = "default_toolchain_impl",
with_grpc = True,
with_flat_package=False,
with_single_line_to_string=False,
visibility = ["//visibility:public"],
)

toolchain(
name = "default_toolchain",
toolchain = ":default_toolchain_impl",
toolchain_type = "@io_bazel_rules_scala//scala_proto:toolchain_type",
visibility = ["//visibility:public"],
)


scala_proto_toolchain(
name = "enable_all_options_toolchain_impl",
with_grpc = True,
with_flat_package=True,
# with_java=True,
with_single_line_to_string=True,
visibility = ["//visibility:public"],
)

toolchain(
name = "enable_all_options_toolchain",
toolchain = ":enable_all_options_toolchain_impl",
toolchain_type = "@io_bazel_rules_scala//scala_proto:toolchain_type",
visibility = ["//visibility:public"],
)
Empty file added scala_proto/private/BUILD
Empty file.
31 changes: 31 additions & 0 deletions scala_proto/private/dep_sets.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@

SCALAPB_DEPS = [
"//external:io_bazel_rules_scala/dependency/proto/scalapb_runtime",
"//external:io_bazel_rules_scala/dependency/com_google_protobuf/protobuf_java",
"//external:io_bazel_rules_scala/dependency/proto/scalapb_lenses",
"//external:io_bazel_rules_scala/dependency/proto/scalapb_fastparse",
]

GRPC_DEPS = [
"//external:io_bazel_rules_scala/dependency/proto/scalapb_runtime_grpc",
"//external:io_bazel_rules_scala/dependency/proto/grpc_core",
"//external:io_bazel_rules_scala/dependency/proto/grpc_stub",
"//external:io_bazel_rules_scala/dependency/proto/grpc_protobuf",
"//external:io_bazel_rules_scala/dependency/proto/grpc_netty",
"//external:io_bazel_rules_scala/dependency/proto/grpc_context",
"//external:io_bazel_rules_scala/dependency/proto/guava",
"//external:io_bazel_rules_scala/dependency/proto/opencensus_api",
"//external:io_bazel_rules_scala/dependency/proto/opencensus_contrib_grpc_metrics",
"//external:io_bazel_rules_scala/dependency/proto/google_instrumentation",
"//external:io_bazel_rules_scala/dependency/proto/netty_codec",
"//external:io_bazel_rules_scala/dependency/proto/netty_codec_http",
"//external:io_bazel_rules_scala/dependency/proto/netty_codec_http2",
"//external:io_bazel_rules_scala/dependency/proto/netty_codec_socks",
"//external:io_bazel_rules_scala/dependency/proto/netty_handler",
"//external:io_bazel_rules_scala/dependency/proto/netty_buffer",
"//external:io_bazel_rules_scala/dependency/proto/netty_transport",
"//external:io_bazel_rules_scala/dependency/proto/netty_resolver",
"//external:io_bazel_rules_scala/dependency/proto/netty_common",
"//external:io_bazel_rules_scala/dependency/proto/netty_handler_proxy",
]

49 changes: 49 additions & 0 deletions scala_proto/private/proto_to_scala_src.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
load(
"//scala/private:common.bzl",
"write_manifest_file",
)
load("//scala/private:rule_impls.bzl", "compile_scala")
load("//scala_proto/private:dep_sets.bzl", "SCALAPB_DEPS", "GRPC_DEPS")


def _root_path(f):
if f.is_source:
return f.owner.workspace_root
return "/".join([f.root.path, f.owner.workspace_root])


def _colon_paths(data):
return ":".join([
f.path
Copy link
Member

Choose a reason for hiding this comment

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

you seem to have reverted (maybe by mistake) this change:
0bac7fe
Is this intentional? If so what's different?
I think this relates to the change in included_proto but I'm not sure I fully understand it

Copy link
Contributor

Choose a reason for hiding this comment

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

I've just handled this differently, the external locations all work right in the tests i've done. Instead of a big chain of imports in the rule i just move things to where they are expected. For me it removed a bunch of warnings about external/com_google_.. not existing. Seemed like it made it simpler though i'm not wed to it.

for f in sorted(data)
])


def proto_to_scala_src(ctx, label, code_generator, compile_proto, include_proto, transitive_proto_paths, flags, jar_output):
worker_content = "{output}\n{included_proto}\n{flags_arg}\n{transitive_proto_paths}\n{inputs}\n{protoc}".format(
output = jar_output.path,
included_proto = "-" + ":".join(sorted(["%s,%s" % (f.root.path, f.path) for f in include_proto])),
# Command line args to worker cannot be empty so using padding
flags_arg = "-" + ",".join(flags),
transitive_proto_paths = "-" + ":".join(sorted(transitive_proto_paths)),
# Command line args to worker cannot be empty so using padding
# Pass inputs seprately because they doesn't always match to imports (ie blacklisted protos are excluded)
inputs = _colon_paths(compile_proto),
protoc = ctx.executable._protoc.path
)
argfile = ctx.actions.declare_file(
"%s_worker_input" % label.name,
sibling = jar_output,
)
ctx.actions.write(output = argfile, content = worker_content)
ctx.actions.run(
executable = code_generator.files_to_run,
Copy link

Choose a reason for hiding this comment

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

@johnynek even after registering the default toolchain in my WORKSPACE, I'm getting the following error:

ERROR: /private/var/tmp/_bazel_feri/27c97e79ab5b7e8bcdeb0d679a4bc60c/external/com_google_protobuf/BUILD:248:2: in @io_bazel_rules_scala//scala_proto/private:scalapb_aspect.bzl%scalapb_aspect aspect on proto_library rule @com_google_protobuf//:any_proto:
Traceback (most recent call last):
	File "/private/var/tmp/_bazel_feri/27c97e79ab5b7e8bcdeb0d679a4bc60c/external/com_google_protobuf/BUILD", line 248
		@io_bazel_rules_scala//scala_proto/private:scalapb_aspect.bzl%scalapb_aspect(...)
	File "/private/var/tmp/_bazel_feri/27c97e79ab5b7e8bcdeb0d679a4bc60c/external/io_bazel_rules_scala/scala_proto/private/scalapb_aspect.bzl", line 175, in _scalapb_aspect_impl
		proto_to_scala_src(ctx, target.label, code_generator, com..., <4 more arguments>)
	File "/private/var/tmp/_bazel_feri/27c97e79ab5b7e8bcdeb0d679a4bc60c/external/io_bazel_rules_scala/scala_proto/private/proto_to_scala_src.bzl", line 39, in proto_to_scala_src
		ctx.actions.run(executable = code_generator.file..., <6 more arguments>)
expected value of type 'File or string' for parameter 'executable', in method call run(FilesToRunProvider executable, list inputs, list outputs, string mnemonic, string progress_message, dict execution_requirements, list arguments) of 'actions'

Copy link

Choose a reason for hiding this comment

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

@gseitz FYI

Copy link
Contributor

Choose a reason for hiding this comment

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

@tferi-da This needs a fix from a bazel update -- the latest rules require bazel 0.23 or above now i'm afraid

Copy link

Choose a reason for hiding this comment

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

@ianoc thanks for the tip!

inputs = compile_proto + include_proto + [argfile, ctx.executable._protoc],
outputs = [jar_output],
mnemonic = "ProtoScalaPBRule",
progress_message = "creating scalapb files %s" % ctx.label,
execution_requirements = {"supports-workers": "1"},
arguments = ["@" + argfile.path],
)


Loading