fix bazel incompatibilities in proto #454
Conversation
| #inline this if after 0.12.0 is the oldest supported version | ||
| if hasattr(target.proto, 'transitive_proto_path'): | ||
| transitive_proto_paths += target.proto.transitive_proto_path | ||
| else: | ||
| jvm_deps.append(target) | ||
|
|
||
| acc_imports = depset(transitive = acc_imports) |
There was a problem hiding this comment.
Can we move changing acc_imports to a depset after 44?
In it you’re changing it back to a list which seems redundant
There was a problem hiding this comment.
so, note that acc_imports as the arg to transitive is a List[Depset] not a List[File] or something, so when we do depset(transitive = acc_imports).to_list() converts a List[Depset] to a List[File].
There was a problem hiding this comment.
😱 that's so implicit JavaConversions feels explicit 😉
| transitive_proto_paths = depset() | ||
|
|
||
| proto_deps, jvm_deps = [], [] | ||
| for target in ctx.attr.deps: | ||
| if hasattr(target, 'proto'): | ||
| proto_deps.append(target) | ||
| acc_imports += target.proto.transitive_sources | ||
| acc_imports.append(target.proto.transitive_sources) |
There was a problem hiding this comment.
Isn’t transitive_sources a depset? If so isn’t it better to accumulate them in depsets?
There was a problem hiding this comment.
there is no way to accumulate depsets that is not deprecated. You accumulate a list and then make a depset(transitive = your_list_here)
There was a problem hiding this comment.
@laurentlb is this correct? Why can’t we accumulate depsets? Turning depsets to lists so we accumulate them as depsets sounds counter intuitive
There was a problem hiding this comment.
Yes, it's correct (https://docs.bazel.build/versions/master/skylark/depsets.html).
- depset is immutable, so the
+=operator was misleading - we can create only one
depset, with the correct structure, and it's much better for performance (adding depset inside a loop could lead toO(n^2)complexity - it's more explicit: you see that the depset has n transitive depsets at the same level (which corresponds to the internal representation)
Depsets are not turned into lists. We just collect all the depsets first, and then merge them all at the same time.
There was a problem hiding this comment.
Can we pass a depset to the “transitive” argument of the constructor?
What Oscar did is convert them to lists for this ctor which sounds wrong
There was a problem hiding this comment.
I didn’t convert them to lists. I added them to a list. The argument to transitive is a list of Depsets. I constructed that list so I could pass the list of depsets to transitive.
There was a problem hiding this comment.
I see, my bad.
Thanks for the clarification
There was a problem hiding this comment.
I'm approving though the code seems strange (with respect to depset accumulation).
Let's hope @laurentlb can shed some light
just the proto part of #452 to address #430