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

Fix thrift bazel incompatibilities #456

Merged
merged 1 commit into from
Mar 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 13 additions & 13 deletions thrift/thrift.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def _thrift_library_impl(ctx):

if len(src_paths) > 0:
zipper_arg_path = ctx.actions.declare_file("%s_zipper_args" % ctx.outputs.libarchive.path)
ctx.file_action(zipper_arg_path, zipper_args)
ctx.actions.write(zipper_arg_path, zipper_args)
_valid_thrift_deps(ctx.attr.deps)
# We move the files and touch them so that the output file is a purely deterministic
# product of the _content_ of the inputs
Expand All @@ -59,15 +59,15 @@ rm -f {out}
cmd = cmd.format(out=ctx.outputs.libarchive.path,
path=zipper_arg_path.path,
zipper=ctx.executable._zipper.path)
ctx.action(
ctx.actions.run_shell(
inputs = ctx.files.srcs + [ctx.executable._zipper, zipper_arg_path],
outputs = [ctx.outputs.libarchive],
command = cmd,
progress_message = "making thrift archive %s (%s files)" % (ctx.label, len(src_paths)),
)
else:
# we still have to create the output we declared
ctx.action(
ctx.actions.run_shell(
inputs = [ctx.executable._zipper],
outputs = [ctx.outputs.libarchive],
command = """
Expand All @@ -81,11 +81,11 @@ rm {out}.contents
)


transitive_srcs = _collect_thrift_srcs(ctx.attr.deps)
transitive_srcs += [ctx.outputs.libarchive]
transitive_external_jars = _collect_thrift_external_jars(ctx.attr.deps)
transitive_srcs = depset([ctx.outputs.libarchive], transitive = _collect_thrift_srcs(ctx.attr.deps))
jarfiles = _collect_thrift_external_jars(ctx.attr.deps)
for jar in ctx.attr.external_jars:
transitive_external_jars += jar.files
jarfiles.append(depset(jar.files))
transitive_external_jars = depset(transitive = jarfiles)

return struct(
thrift = struct(
Expand All @@ -96,17 +96,17 @@ rm {out}.contents
),
)

def _collect_thrift_attr(targets, attr):
s = depset()
def _collect_thrift_attr_depsets(targets, attr):
ds = []
for target in targets:
s += getattr(target.thrift, attr)
return s
ds.append(getattr(target.thrift, attr))
return ds

def _collect_thrift_srcs(targets):
return _collect_thrift_attr(targets, "transitive_srcs")
return _collect_thrift_attr_depsets(targets, "transitive_srcs")

def _collect_thrift_external_jars(targets):
return _collect_thrift_attr(targets, "transitive_external_jars")
return _collect_thrift_attr_depsets(targets, "transitive_external_jars")

def _valid_thrift_deps(targets):
for target in targets:
Expand Down
87 changes: 48 additions & 39 deletions twitter_scrooge/twitter_scrooge.bzl
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
_jar_filetype = FileType([".jar"])

load("//scala:scala.bzl",
"scala_mvn_artifact",
"scala_library",
"write_manifest",
"collect_srcjars",
"collect_jars")

_jar_filetype = FileType([".jar"])

def twitter_scrooge():
native.maven_server(
name = "twitter_scrooge_maven_server",
Expand Down Expand Up @@ -55,71 +55,80 @@ def twitter_scrooge():
native.bind(name = 'io_bazel_rules_scala/dependency/thrift/util_logging', actual = '@util_logging//jar')

def _collect_transitive_srcs(targets):
r = depset()
r = []
for target in targets:
if hasattr(target, "thrift"):
r += target.thrift.transitive_srcs
return r
r.append(target.thrift.transitive_srcs)
return depset(transitive = r)

def _collect_owned_srcs(targets):
r = depset()
r = []
for _target in targets:
if hasattr(_target, "extra_information"):
for target in _target.extra_information:
if hasattr(target, "scrooge_srcjar"):
r += target.scrooge_srcjar.transitive_owned_srcs
return r
r.append(target.scrooge_srcjar.transitive_owned_srcs)
return depset(transitive = r)

def _collect_external_jars(targets):
r = depset()
r = []
for target in targets:
if hasattr(target, "thrift"):
thrift = target.thrift
if hasattr(thrift, "external_jars"):
for jar in thrift.external_jars:
r += _jar_filetype.filter(jar.files)
r += _jar_filetype.filter(thrift.transitive_external_jars)
return r
r.append(depset(_jar_filetype.filter(jar.files)))
r.append(depset(_jar_filetype.filter(thrift.transitive_external_jars)))
return depset(transitive = r)

def collect_extra_srcjars(targets):
srcjars = depset()
srcjar = []
srcjars = []
for target in targets:
if hasattr(target, "extra_information"):
for _target in target.extra_information:
srcjars += [_target.srcjars.srcjar]
srcjars += _target.srcjars.transitive_srcjars
return srcjars
srcjar.append(_target.srcjars.srcjar)
srcjars.append(_target.srcjars.transitive_srcjars)
return depset(srcjar, transitive = srcjars)

def _collect_immediate_srcs(targets):
r = depset()
r = []
for target in targets:
if hasattr(target, "thrift"):
r += [target.thrift.srcs]
return r
r.append(depset([target.thrift.srcs]))
return depset(transitive = r)

def _assert_set_is_subset(want, have):
missing = depset()
missing = []
for e in want:
if e not in have:
missing += [e]
missing.append(e)
if len(missing) > 0:
fail('scrooge_srcjar target must depend on scrooge_srcjar targets sufficient to ' +
'cover the transitive graph of thrift files. Uncovered sources: ' + str(missing))

def _colon_paths(data):
return ':'.join([f.path for f in data])

def _list_to_map(items):
map_result = {}
for item in items:
map_result[item] = None
return map_result

def _gen_scrooge_srcjar_impl(ctx):
remote_jars = depset()
remote_jars = []
for target in ctx.attr.remote_jars:
remote_jars += _jar_filetype.filter(target.files)
remote_jars.append(_jar_filetype.filter(target.files))
# deduplicate these
remote_jars = depset(transitive = remote_jars).to_list()

# These are JARs that are declared externally and only have Thrift files
# in them.
external_jars = _collect_external_jars(ctx.attr.deps)
external_jars = _collect_external_jars(ctx.attr.deps).to_list()

# These are the thrift sources whose generated code we will "own" as a target
immediate_thrift_srcs = _collect_immediate_srcs(ctx.attr.deps)
immediate_thrift_srcs = _collect_immediate_srcs(ctx.attr.deps).to_list()

# This is the set of sources which is covered by any scala_library
# or scala_scrooge_gen targets that are depended on by this. This is
Expand All @@ -130,16 +139,16 @@ def _gen_scrooge_srcjar_impl(ctx):
# These are the thrift sources in the dependency graph. They are necessary
# to generate the code, but are not "owned" by this target and will not
# be in the resultant source jar
transitive_thrift_srcs = transitive_owned_srcs + _collect_transitive_srcs(ctx.attr.deps)
transitive_thrift_srcs = depset(transitive = [transitive_owned_srcs, _collect_transitive_srcs(ctx.attr.deps)]).to_list()
Copy link
Member

Choose a reason for hiding this comment

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

does this (depset and then to_list) make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to walk the depset eventually, but we do need it as a list. So, I don't know another way.

Note this is semantically a union of two depsets, but union is deprecated now...

I really don't know why they deprecated union when you can implement with depset(transitive = [a, b])... They should be optimizing depset to handle that case...

I'm concerned the bazel folks have not looked at the FP literature on fast immutable types, but maybe they have. I fear they may just be doing immutability with copying.


only_transitive_thrift_srcs = depset()
only_transitive_thrift_srcs = []
for src in transitive_thrift_srcs:
if src not in immediate_thrift_srcs:
only_transitive_thrift_srcs += [src]
if src not in _list_to_map(immediate_thrift_srcs):
only_transitive_thrift_srcs.append(src)

# We want to ensure that the thrift sources which we do not own (but need
# in order to generate code) have targets which will compile them.
_assert_set_is_subset(only_transitive_thrift_srcs, transitive_owned_srcs)
_assert_set_is_subset(_list_to_map(only_transitive_thrift_srcs), _list_to_map(transitive_owned_srcs.to_list()))

# bazel worker arguments cannot be empty so we pad to ensure non-empty
# and drop it off on the other side
Expand All @@ -153,14 +162,14 @@ def _gen_scrooge_srcjar_impl(ctx):
'--with-finagle' if ctx.attr.with_finagle else '',
]))

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._pluck_scrooge_scala,
inputs = list(remote_jars) +
list(only_transitive_thrift_srcs) +
list(external_jars) +
list(immediate_thrift_srcs) +
inputs = remote_jars +
only_transitive_thrift_srcs +
external_jars +
immediate_thrift_srcs +
[argfile],
outputs = [ctx.outputs.srcjar],
mnemonic="ScroogeRule",
Expand All @@ -186,7 +195,7 @@ def _gen_scrooge_srcjar_impl(ctx):
transitive_runtime_jars = deps_jars.transitive_runtime_jars,
)

transitive_srcjars = collect_srcjars(ctx.attr.deps) + collect_extra_srcjars(ctx.attr.deps)
transitive_srcjars = depset(transitive = [collect_srcjars(ctx.attr.deps), collect_extra_srcjars(ctx.attr.deps)])

srcjarsattr = struct(
srcjar = ctx.outputs.srcjar,
Expand All @@ -198,7 +207,7 @@ def _gen_scrooge_srcjar_impl(ctx):
srcjars=srcjarsattr,
extra_information=[struct(
srcjars=srcjarsattr,
scrooge_srcjar=struct(transitive_owned_srcs = transitive_owned_srcs + immediate_thrift_srcs),
scrooge_srcjar=struct(transitive_owned_srcs = depset(immediate_thrift_srcs, transitive = [transitive_owned_srcs])),
)],
)

Expand Down