Skip to content

fix bazel incompatibilities in proto #454

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 27, 2018
Merged

fix bazel incompatibilities in proto #454

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 8 additions & 7 deletions scala_proto/scala_proto.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,38 +319,39 @@ def _colon_paths(data):
return ':'.join(["{root},{path}".format(root=_root_path(f), path=f.path) for f in data])

def _gen_proto_srcjar_impl(ctx):
acc_imports = depset()
acc_imports = []
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn’t transitive_sources a depset? If so isn’t it better to accumulate them in depsets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no way to accumulate depsets that is not deprecated. You accumulate a list and then make a depset(transitive = your_list_here)

Copy link
Contributor

Choose a reason for hiding this comment

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

@laurentlb is this correct? Why can’t we accumulate depsets? Turning depsets to lists so we accumulate them as depsets sounds counter intuitive

Copy link
Contributor

Choose a reason for hiding this comment

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

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 to O(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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, my bad.
Thanks for the clarification

#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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move changing acc_imports to a depset after 44?
In it you’re changing it back to a list which seems redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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].

Copy link
Contributor

Choose a reason for hiding this comment

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

😱 that's so implicit JavaConversions feels explicit 😉

if "java_conversions" in ctx.attr.flags and len(jvm_deps) == 0:
fail("must have at least one jvm dependency if with_java is True (java_conversions is turned on)")

deps_jars = collect_jars(jvm_deps)

worker_content = "{output}\n{paths}\n{flags_arg}\n{packages}".format(
output = ctx.outputs.srcjar.path,
paths = _colon_paths(acc_imports),
paths = _colon_paths(acc_imports.to_list()),
# Command line args to worker cannot be empty so using padding
flags_arg = "-" + ",".join(ctx.attr.flags),
# Command line args to worker cannot be empty so using padding
packages = "-" + ":".join(transitive_proto_paths.to_list())
)
argfile = ctx.new_file(ctx.outputs.srcjar, "%s_worker_input" % ctx.label.name)
ctx.file_action(output=argfile, content=worker_content)
ctx.action(
argfile = ctx.actions.declare_file("%s_worker_input" % ctx.label.name, sibling = ctx.outputs.srcjar)
ctx.actions.write(output=argfile, content=worker_content)
ctx.actions.run(
executable = ctx.executable.generator,
inputs = list(acc_imports) + [argfile],
inputs = depset([argfile], transitive = [acc_imports]),
outputs = [ctx.outputs.srcjar],
mnemonic="ProtoScalaPBRule",
progress_message = "creating scalapb files %s" % ctx.label,
Expand Down