Make the scala_protobuf aspect based #700
Conversation
…the results of the stdout of calling bazel to a variable, bash will try execute it. So the response code here will always have been on the next line -1
|
@ianoc-stripe this is great! thanks. Did you see if this addresses some concerns from internal uses? Can you comment on them here so people on google can see some of our motivations? |
|
Thanks taking this on. I took a look, generally it looks like the right direction to me. The challenge here is that the options are set once for all targets, I think. That may be okay, or may not. Options to aspects didn't have a lot of features when I looked at this with thrift. @ittaiz can you and your team take a look at this? |
| load("//scala_proto:scala_proto_toolchain.bzl", "scala_proto_toolchain") | ||
|
|
||
| toolchain_type( | ||
| name = "toolchain_type", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't really know about how much of this is required/needed tbh. I cargo culted this from the scala tool chain
scala_proto/BUILD
Outdated
| name = "default_toolchain_impl", | ||
| with_grpc = False, | ||
| with_flat_package=False, | ||
| # with_java=False, |
There was a problem hiding this comment.
why the comment? to show the setting?
There was a problem hiding this comment.
Right now this is commented because i haven't figured out how it should be implemented dynamically -- we need to depend on the java aspect only when this is true. And depending on the java aspect itself since its defined in java i haven't found the right magic invokcation for yet
| # Pass inputs seprately because they doesn't always match to imports (ie blacklisted protos are excluded) | ||
| inputs = _colon_paths(compile_proto) | ||
| ) | ||
| print(worker_content) |
| compile_protos = sorted(target_ti.direct_sources) | ||
| transitive_protos = sorted(target_ti.transitive_sources) | ||
|
|
||
| print("transitive_descriptor_sets") |
There was a problem hiding this comment.
can we remove or make a flag for debugging?
| label.name + "_scalac.statsfile", | ||
| sibling = scalapb_jar, | ||
| ) | ||
| print("Compiling %s -> %s" %(label, scalapb_jar)) |
| implementation = _scalapb_aspect_impl, | ||
| attr_aspects = ["deps"], | ||
| attrs = { | ||
| "_pluck_scalapb_scala": attr.label( |
There was a problem hiding this comment.
I don't think this pluck name is relevant here.
| require(op.toFile.exists, s"Path $fullPath does not exist, which it should as a dependency of this rule") | ||
| val relativePath = rp.relativize(op) | ||
|
|
||
| println(s"Root: rp , op: $op and relativePath: $relativePath") |
|
|
||
| println(s"Root: rp , op: $op and relativePath: $relativePath") | ||
| relativePath.toFile.getParentFile.mkdirs | ||
| java.nio.file.Files.copy(op, relativePath) |
test/proto/BUILD
Outdated
| deps = [ | ||
| ":test2", | ||
| ":test_proto_java_lib", | ||
| # ":test_proto_java_lib", |
test/proto/BUILD
Outdated
| name = "test1_proto_scala", | ||
| deps = ["//test/proto2:test"], | ||
| generator = "@io_bazel_rules_scala//src/scala/scripts:scalapb_generator") | ||
| # scala_proto_srcjar( |
There was a problem hiding this comment.
I guess we want to enable these before merging? somehow? I think passing the generator should not be an option, but building the test1 target.
|
What this is: Why/motivation:
Challenges/differences: Since the scala_proto's now are going to be generated via the aspect we need to share a bunch of the generating options. I've moved these to a toolchain to enable them for a repo. Similar to how this would mostly be used in sbt from scalapb i hope its not a negative impact for folks. One thing I haven't quite figured out yet is doing the Also still to be implemented: |
|
Indeed thanks a lot!
|
|
@ittaiz on (1) you mean you like having the scala_library just referring to the src jar of the scalapb code? or how do you mean decoupled? |
|
@ittaiz did you have a chance to take another look here? If you have a custom generator that needs to get passed to all targets we could do it via a toolchain config setting? |
|
@ianoc-stripe is this still a WIP? Can we merge master? |
|
@johnynek its still WIP just since there's two features not implemented blacklists and java converters, blacklists seem trivial, not entirely sure quite yet how to correctly handle blacklists. But also to understand for folks like @ittaiz that this is directionally usable for him or we need to possibly consider that this will need to be put in as a new set of rules. (Given aspect based stuff implies more global configuration from the tool chain and less local per-target config). |
|
@simuons is the one leading our internal proto scala work and I asked him to take a look since I think this will severely break us :( |
|
what do you think will break you @ittaiz ? Maybe we can focus on that. Ideally, the majority of old targets can be made to work with no change, and otherwise have a canonical simple change for the rest. We are really concerned with performance and this was MUCH faster for us (according to what @ianoc-stripe told me offline). |
|
In general I'm very much in favor of this approach and want to merge it in even if it breaks some APIs and we need to do some work for it but it has to be compatible. |
|
Its a little odd seemingly but it might be easy enough to maintain if we aimed for the ability for a global generator + blacklist for said generator. You could blacklist the inception point then? |
|
I don't remember the blacklist 100% unfortunately. |
ianoc-stripe
changed the title
|
So we will require bazel 0.23.0 for this, but now the tool can properly be configured on the toolchain. Needs bazelbuild/bazel@5a20a44 i've also made the changes to just use the google protoc now too. With this change we can look closer at what would be required to support a custom generator for some specific labels |
|
ok @ittaiz see what you think now maybe. With this you can specify a second code generator, and then a set of targets to give to that code generator. Would that work for Wix? |
| [ScalaPBImport], | ||
| ], | ||
| attrs = { | ||
| "_protoc": attr.label(executable = True, cfg = "host", default = "@com_google_protobuf//:protoc"), |
There was a problem hiding this comment.
@simuons this is the same change you have, do you agree?
There was a problem hiding this comment.
This looks really great to me. Thank you for such a thorough job and for designing a work around for Wix, even though we don't currently use this approach.
This looks like a major win to me, especially since our proto compilations are pretty slow now. This is exciting!
@ittaiz what do you think? It looks like your concerns were addressed. Would you agree?
| @@ -2,48 +2,38 @@ package scripts | |||
|
|
|||
| import java.nio.file.{Files, Path, Paths} | |||
|
|
|||
| class PBGenerateRequest(val jarOutput: String, val scalaPBOutput: Path, val scalaPBArgs: List[String], val protoc: Path) | |||
| class PBGenerateRequest(val jarOutput: String, val scalaPBOutput: Path, val scalaPBArgs: List[String], val includedProto: List[(String, String)], val protoc: Path) | |||
There was a problem hiding this comment.
wonder why this isn't a case class and we can remove all the vals...
There was a problem hiding this comment.
will try make that change to make it shorter
| val protoFiles = args.get(4).split(':') | ||
| val includedProto = args.get(1).drop(1).split(':').distinct.map { e => | ||
| val p = e.split(',') | ||
| (p(0), p(1)) |
There was a problem hiding this comment.
should we just use Path here so we don't have to do the conversion later?
| ) | ||
|
|
||
| scala_proto_srcjar( |
There was a problem hiding this comment.
can we no longer directly exercise srcjar?
There was a problem hiding this comment.
Nope, we could bring it back if it was useful, though since the aspect generates one internally they would be duplicates of some form
| code_generator = toolchain.override_code_generator | ||
|
|
||
|
|
||
| for lbl in toolchain.override_code_generator_targets: |
There was a problem hiding this comment.
This is identical to three lines above. I guess it should be removed.
|
@ianoc @johnynek, On a more abstract note- I have to say that I'm not sure how this should have turned out. One the one hand there is a significant advancement (written above) but on the other hand this is breaking existing features that work for us. Don't know... |
|
@ittaiz instead of an override generator if that doesn't quite fit the bill -- how about we have a I'm not entirely sure tbh still of your needs here, but i think then you could construct the core targets anyway you need, followed by adding those targets into the toolchains dependency set so all the toolchain generated targets depend on them? |
|
I hope @simuons will give a more thorough description of our needs but basically I under-represented the complexity. The core targets are a small issue. One core requirement is to allow the introduction of a custom generator without it needing to be blessed by the infra team. Another desired feature (not must) is to avoid generating all generators if a target doesn't want it. Not 100% sure about the latter. One question though- I seem to remember that the aspect can read the attributes of the target it's inspecting. Why not have custom flags attribute on the rule? |
|
@ittaiz there is a tree of targets generated that are shared by multiple calls to the scala proto library rule. So the rule we are building it for is the proto_library not the scala_proto_library. the scala_proto_library expresses then a dependency on one of the generated by aspect targets. So it becomes difficult to say how a shared target should be built based on different args passed for generators |
|
@ittaiz the aspect mechanism does not allow for custom flags per rule. I think you could attach custom flags to the proto and have it propogate, but the whole idea is that each proto rule gets mapped to a scala proto src jar. There is no place to hook custom args, that I know of, onto the proto. I don't know why you can't use one generator for everyone and control the behavior by adding annotations to the proto itself (or in some comment). At twitter, which was a pretty large org when I was there, we only used 1 generator for thrift. We did not have some concept that each rule would pass bespoke options. We tried to push any options into the thrift sources, which you could control. I'm a bit skeptical that this must be solved by passing custom generators to proto rules, and I'm concerned that design clashes with aspects for the reason I mention above, so is somewhat ill-suited to a bazel best practice of aspects for code generation. |
| # method bodies | ||
| compile_scala( | ||
| ctx, | ||
| Label("%s-fast" % (label)), |
There was a problem hiding this comment.
starlark really needs some kind of gensym thing that makes sure generated symbols are unique.
| default = Label("@io_bazel_rules_scala//src/scala/scripts:scalapb_generator"), | ||
| allow_files=True | ||
| ), | ||
| "override_code_generator_targets": attr.label_list( |
There was a problem hiding this comment.
should we remove this override code if @ittaiz says it won't be useful for them anyway? simplify the code?
| ) | ||
| ctx.actions.write(output = argfile, content = worker_content) | ||
| ctx.actions.run( | ||
| executable = code_generator.files_to_run, |
There was a problem hiding this comment.
@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'
There was a problem hiding this comment.
@tferi-da This needs a fix from a bazel update -- the latest rules require bazel 0.23 or above now i'm afraid
This reverts commit 74192b2.
This reverts commit 74192b2.
Working on a scala_protobuf aspect based rules.
These use the toolchain to configure the options instead of being on the rule. Allowing us to build out the shadow graph and share it.
The only part i haven't implemented and am not entirely sure how i should is the java conversions. This would also be a pretty large change for someone to implement. (Though when you'd want to mix/match these options usually in a repo i don't know, so it might not be much of a deal other than deleting some lines).