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.

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

Jump to bottom

Add a ThriftInfo provider #464

Merged
merged 3 commits into from
Apr 2, 2018
Merged

Add a ThriftInfo provider #464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e11592a
Add a ThriftInfo provider
johnynek Mar 31, 2018
2644081
minor cleanup
johnynek Mar 31, 2018
ba16278
address review comments
johnynek Mar 31, 2018
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
37 changes: 22 additions & 15 deletions thrift/thrift.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

_thrift_filetype = FileType([".thrift"])

ThriftInfo = provider(
fields=[
"srcs", # The source files in this rule
"transitive_srcs", # the transitive version of the above
"external_jars", # external jars of thrift files
"transitive_external_jars" # transitive version of the above
])

def _common_prefix(strings):
pref = None
for s in strings:
Expand Down Expand Up @@ -87,30 +95,29 @@ rm {out}.contents
jarfiles.append(depset(jar.files))
transitive_external_jars = depset(transitive = jarfiles)

return struct(
thrift = struct(
srcs = ctx.outputs.libarchive,
transitive_srcs = transitive_srcs,
external_jars = ctx.attr.external_jars,
transitive_external_jars = transitive_external_jars,
),
)
return [
ThriftInfo(
srcs = ctx.outputs.libarchive,
transitive_srcs = transitive_srcs,
external_jars = ctx.attr.external_jars,
transitive_external_jars = transitive_external_jars,
)]

def _collect_thrift_attr_depsets(targets, attr):
def _collect_thrift_srcs(targets):
ds = []
for target in targets:
ds.append(getattr(target.thrift, attr))
ds.append(target[ThriftInfo].transitive_srcs)
return ds

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

def _collect_thrift_external_jars(targets):
return _collect_thrift_attr_depsets(targets, "transitive_external_jars")
ds = []
for target in targets:
ds.append(target[ThriftInfo].transitive_external_jars)
return ds

def _valid_thrift_deps(targets):
for target in targets:
if not hasattr(target, "thrift"):
if not ThriftInfo in target:
fail("thrift_library can only depend on thrift_library", target)

# Some notes on the raison d'etre of thrift_library vs. code gen specific
Expand Down
27 changes: 14 additions & 13 deletions twitter_scrooge/twitter_scrooge.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ load("//scala:scala.bzl",
"collect_srcjars",
"collect_jars")

load("//thrift:thrift.bzl", "ThriftInfo")

_jar_filetype = FileType([".jar"])

def twitter_scrooge():
Expand Down Expand Up @@ -57,8 +59,8 @@ def twitter_scrooge():
def _collect_transitive_srcs(targets):
r = []
for target in targets:
if hasattr(target, "thrift"):
r.append(target.thrift.transitive_srcs)
if ThriftInfo in target:
r.append(target[ThriftInfo].transitive_srcs)
return depset(transitive = r)

def _collect_owned_srcs(targets):
Expand All @@ -73,13 +75,12 @@ def _collect_owned_srcs(targets):
def _collect_external_jars(targets):
r = []
for target in targets:
if hasattr(target, "thrift"):
thrift = target.thrift
if hasattr(thrift, "external_jars"):
for jar in thrift.external_jars:
r.append(depset(_jar_filetype.filter(jar.files)))
r.append(depset(_jar_filetype.filter(thrift.transitive_external_jars)))
return depset(transitive = r)
if ThriftInfo in target:
thrift = target[ThriftInfo]
for jar in thrift.external_jars:
r.extend(_jar_filetype.filter(jar.files))
r.extend(_jar_filetype.filter(thrift.transitive_external_jars))
return depset(r)
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using the transitive parameter on the depset constructor. Something like this might work:

depset(transitive =
  [_jar_filetype.filter(jar.files) for jar in thrift.external_jars] +
  [_jar_filetype.filter(thrift.transitive_external_jars)]
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also note that FileType is deprecated and we should move away from it (maybe in another PR).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that works because transitive is a list of depsets while the arg to depset is a list of items (non-depset). I could make singleton depsets, but I assume that is slower than just putting it in the first arg since it is adding more nesting.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 makes sense. I'm not terribly familiar with the thrift code so it wasn't immediately apparent what's a depset and what's not.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, me neither. Especially due to the lack of types... :(


def collect_extra_srcjars(targets):
srcjar = []
Expand All @@ -92,11 +93,11 @@ def collect_extra_srcjars(targets):
return depset(srcjar, transitive = srcjars)

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

def _assert_set_is_subset(want, have):
missing = []
Expand Down