Add a ThriftInfo provider #464
Conversation
thrift/thrift.bzl
Outdated
| @@ -2,6 +2,8 @@ | |||
|
|
|||
| _thrift_filetype = FileType([".thrift"]) | |||
|
|
|||
| ThriftInfo = provider(fields=["srcs", "transitive_srcs", "external_jars", "transitive_external_jars"]) | |||
There was a problem hiding this comment.
nit: format this with each field on a new line
also consider adding a one line comment to each provider field to document what it's for
| 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) |
There was a problem hiding this comment.
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)]
)
There was a problem hiding this comment.
Also note that FileType is deprecated and we should move away from it (maybe in another PR).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
👍 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.
There was a problem hiding this comment.
yeah, me neither. Especially due to the lack of types... :(
|
LGTM |
* Add a ThriftInfo provider * minor cleanup * address review comments
This updates thrift to use a modern provider instead of an anonymous struct.
This is part of #450
ptal @andyscott @ittaiz