Add scrooge support #29
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
note stripe has signed the CLA. |
| # TODO the FileType.filter also seemed to get rid of it...?? | ||
| for srcjar in srcjars: | ||
| srcjar_cmd += """ | ||
| mkdir -p {{out}}_tmp/expand_srcjars/{srcjar} |
There was a problem hiding this comment.
we should delete this first to make sure we don't keep old ones around. edit, I should read more before I comment. I see it is a subdir, so it should be clean.
There was a problem hiding this comment.
yeah, we rm -rf {out}_tmp so it should be removed as part of that
| implementation = _scrooge_library_impl, | ||
| attrs = { | ||
| "srcs": attr.label_list(allow_files=_thrift_filetype), | ||
| "deps": attr.label_list(allow_files=_scroogelib_tar_filetype), |
There was a problem hiding this comment.
I think we only want other scrooge libraries here, not the tars.
There was a problem hiding this comment.
yeah, you are right. this is 100% cargo culted from: https://github.com/wt/bazel_thrift/blob/master/tools/thrift.bzl#L95
Though that raises the question: what are they trying to express?
|
This is super great! I would like two things:
|
|
Ok, CI passes, though I need to add real tests. Definitely agree with 2), 1) I agree with but I want to do after I have a couple of passes to answer some of the issues. Without the scrooge stuff, there is no use case that will properly ensure that the srcjars are being used as intended, and that transitive information is passed along properly. Though I have a feeling that if I answer some of your comments above, the decoupling of the 2 PRs will be a lot cleaner. |
| fail('scrooge_srcjar target must depend on scrooge_srcjar targets sufficient to ' + | ||
| 'cover the transitive graph of thrift files. Uncovered sources: ' + missing) | ||
|
|
||
| def _content_newline(data): |
There was a problem hiding this comment.
can we say _path_newline or something to reflect that we are calling .path?
There was a problem hiding this comment.
#namingthingsishard
|
Lots of cleanup just pushed |
|
So it sounds like you're ok with absolute_prefix, and we should at least get a POC that new_git_repository can eliminate the need for remote_jars |
| "_pluck_scrooge": attr.label( | ||
| executable=True, | ||
| default=Label("//src/python/scripts:twitter_scrooge_pluck_cmd"), | ||
| #single_file=True, | ||
| allow_files=True), | ||
| } + implicit_deps(), |
There was a problem hiding this comment.
I don't think you need the scala implicit deps, do you?
There was a problem hiding this comment.
good catch...man, using the executable is a really nice way to simplify it!
|
One minor comment, then ready to go. Thanks for pushing this through! |
|
So what do we want to do about the remote_jars? Leave them for now, but comment that they may be removed soon? Or get the new_git_repository thing working? |
|
Oh, and re: _jdk, I'm gonna put it back... I totally forgot about #37 ! |
|
so I guess _jdk is a dependency of _jar? hm, I guess we will see :) |
|
crap, I merged your other one and now this won't merge cleanly |
|
I'll fix it now |
|
Fixing it was pretty easy actually... I kept the commits, though it was a rebase, so they'll get new sha's. But hopefully you still get the incremental view? |
|
Awesome! |
|
For your information this has broken ci.bazel.io build (missing dependency). See http://ci.bazel.io/job/rules_scala/105/ |
Ok, there is a LOT to be desired here... there are a ton of TODOS
BUT IT WORKS
I currently have a project which has two thrift files which depend on each other, BUILDS for them, and then a scala_library which depends on that and the thrifts...it builds. My guess is right now transitive stuff will not work.
So my immediate first goal is to make a test directory with some thrifts and some stuff that depends on them, to make sure that transitive stuff works properly.
After that is just...making it less crazy. I really, really need the guidance of people who have done this... I will put a microscope to everything I did, but I'm finding the "throw structs around with attributes! oh some attributes are missing!" to really resist abstraction and promote hacks...
But it works :) so I will do the above, hopefully refactor with guidance and time, and then will try it on one of our internal repositories.
One thing that I would like to put off, though, is moving it into different files etc. Right now I want to just make sure that we get the flow of everything down properly.