Aspect-based scrooge implementation #524

johnynek · 2018-06-16T22:27:56Z

this fixes #510

It works, but there are a few issues currently:
.

it appears aspects can only have string attr arguments, we have previously been using booleans for with_finagle. See the string requirement here: https://docs.bazel.build/versions/master/skylark/aspects.html#aspect-definition-1
Public aspect attributes are of type string and are called parameters. Parameters must have avalues attribute specified on them. In this case we have a parameter called extension that is allowed to have '*', 'h', or 'cc' as a value.. I side stepped this by always enabling, but we can revisit by adding a flag on the thrift_library to enable or not if this becomes an issue.
there is a weird double wrapping of ScroogeInfo and ScroogeAspectInfo. This is because aspects and the rules themselves cannot return the same provider type. We want to support depending on scrooges in a scrooge because this is how it was implemented before, and it would be nice to minimize migration pains.
I can't handle the java flags for the same reason I can't handle with_finagle boolean: they were a list of strings, not a single string from a finite set. This one may be solvable if we use a toolchain to set the java flags, and it may be that the toolchain, which is yet another migration we have yet to finish.

I'd appreciate feedback at this point to raise any warning signs.

cc @ittaiz @ianoc

johnynek · 2018-06-16T22:38:48Z

related to #170 #402

johnynek · 2018-06-27T03:39:59Z

ptal @ianoc @ittaiz @andyscott

I will try a build of some internal stripe repos with this tomorrow.

It does change the semantics slightly, but it makes them more bazel-y I would argue. Also, I think all the current uses at Stripe will either work directly, or will error (but most will work).

johnynek · 2018-06-27T03:41:31Z

note this is almost a net 0 change in lines: it enables us to remove extra_information and once we get #534 green, we can convert to totally modern providers, which will be nice to finally get there.

andyscott · 2018-06-27T04:40:51Z

I read through and it looks good. What's the subtle semantic change? It's not apparent from a read through the diff.

ittaiz

I went over the non thrift.bzl and twitter_scrooge.bzl part (which is the small part) and it looks good. I'm sorry for not reviewing the whole of it but I'll defer to @ianoc since I'm pretty sure it will be much easier for him with the context. If you need me then I'll make the bandwitdh to thoroughly review

ittaiz · 2018-06-27T04:49:33Z

btw, the failures seem to be related to the fact that you're using syntax/constructs from 0.14 while we're running in travis with 0.13 (which explains why buildkite is green)

ianoc-stripe · 2018-06-27T19:22:38Z

test/src/main/scala/scalarules/test/twitter_scrooge/thrift/bare_jar_thrifts/BUILD

    visibility = ["//visibility:public"],
 )

 java_import(
    name = "barejar_java_import",
-    jars = ["bare-thrift.jar"],
+    jars = ["bare_jar_thrifts_scrooge.jar"],


what is thrifts_scrooge ?

it is the compiled scrooge code.

ianoc-stripe · 2018-06-27T19:26:32Z

thrift/thrift.bzl

@@ -160,6 +165,9 @@ thrift_library = rule(
        "absolute_prefix": attr.string(default = '', mandatory = False),
        "absolute_prefixes": attr.string_list(),
        # This is a list of JARs which only contain Thrift files
+        # these files will NOT  be compiled as part of the current target,
+        # but the current thrifts do include these. It should also include


These are external_thrift_jar's right? there isn't a requirement that both the compiled + thrift are in the one jar i think? (you can think of the scenario the thrift is only in a jar with the thrift classifier for instance)

so, I kept it segregated, but now we compile the jars. This allows you to do import style of remote items by using a target with only an external_jar. mixing them with the srcs ultimately seemed confusing to me since I don't think we want to rezip them into the output or apply the prefix rules to them. What do you think about this?

It has the side effect, I think, of making current uses just work, although slower if they include a giant jar over and over.

ianoc-stripe · 2018-06-27T19:26:50Z

tools/bazel.rc

@@ -1,2 +1,2 @@
-build --experimental_local_disk_cache
-build --experimental_local_disk_cache_path=.bazel_cache
+#build --experimental_local_disk_cache


Shouldn't be committed? though after rebase should be gone

ianoc-stripe · 2018-06-27T19:31:06Z

twitter_scrooge/twitter_scrooge.bzl

      paths = path_content,
      flags = worker_arg_pad + ':'.join([
-          '--with-finagle' if ctx.attr.with_finagle else '',
+          # '--with-finagle' if ctx.attr.with_finagle else '',


commented code?

ianoc-stripe · 2018-06-27T19:37:20Z

twitter_scrooge/twitter_scrooge.bzl

+      resource_jars = [],
+      labels = {},
+      in_scalacopts = [],
+      print_compile_time = False,


not for this PR, but with stats files and other things, we should probably just remove this maybe to clean up/fewer options soon

I agree. cc @andyscott do we have a ticket to remove this and make sure we can get the data from BEP logs?

Is this used for the stats file target? I believe originally this was just a println I added

I think this is your println. The statsfile is always emitted I believe.

ianoc-stripe · 2018-06-27T19:43:19Z

small questions and such, otherwise i think lgtm when green. Might be worth firing it through an internal build once you get it green?

johnynek · 2018-06-28T00:32:00Z

with minor expected changes, this builds our giant thrift repo.

johnynek · 2018-06-28T03:08:51Z

I am mostly building our largest scala repo, which has more complex usage of thrift is somewhat painful, but manageable (maybe 2 hours of modification).

The problem was:

it didn't really matter if you made the deps correct on thrift_library. It was a dummy, so you could fix them in the scrooge_scala_library deps. That is currently not possible because we invoke one scrooge run per thrift_library
we removed remote_jar and no wrapping I could see could put that behavior back. Basically, by the time you have a scrooge_scala_library it is too late to add deps. I could work around with scrooge_scala_import, but it was work.
we often had thrift targets private, but public scrooge, but the way the rule worked, public scrooge made the thrift public too. That sucks. You had no way to just expose the compiled code, but not allow people to make thrifts that depend on your code, we now have that ability.

ianoc-stripe · 2018-06-28T18:43:02Z

lgtm

WIP: aspect-based scrooge implementation

d47b079

googlebot added the cla: yes label Jun 16, 2018

johnynek added 2 commits June 26, 2018 09:02

merge with master

fcd3214

Get the aspect working

e1cf910

johnynek changed the title ~~WIP: aspect-based scrooge implementation~~ Aspect-based scrooge implementation Jun 27, 2018

fix a comment

dafcb3a

ittaiz reviewed Jun 27, 2018

View reviewed changes

johnynek mentioned this pull request Jun 27, 2018

Use java_common.run_ijar #538

Merged

ianoc-stripe reviewed Jun 27, 2018

View reviewed changes

johnynek added 4 commits June 27, 2018 11:40

merge master

d7dae2f

go ahead and be more strict

dcfda86

remove scrooge.jars, fix comments

0f693ab

setup DefaultInfo to declare outputs

4d7577c

add scrooge_scala_import

ddef410

johnynek merged commit 72d8cd2 into master Jun 28, 2018

johnynek mentioned this pull request Jun 28, 2018

aspect based proto rules #548

Closed

ittaiz deleted the oscar/scrooge_aspect branch July 1, 2018 12:35

beala-stripe mentioned this pull request Jul 17, 2018

Fix Intellij Bazel Plugin support for scrooge_scala_library rule #562

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aspect-based scrooge implementation #524

Aspect-based scrooge implementation #524

johnynek commented Jun 16, 2018 •

edited

Loading

johnynek commented Jun 16, 2018

johnynek commented Jun 27, 2018

johnynek commented Jun 27, 2018

andyscott commented Jun 27, 2018

ittaiz left a comment

ittaiz commented Jun 27, 2018

ianoc-stripe Jun 27, 2018

johnynek Jun 27, 2018

ianoc-stripe Jun 27, 2018

johnynek Jun 27, 2018

ianoc-stripe Jun 27, 2018

ianoc-stripe Jun 27, 2018

ianoc-stripe Jun 27, 2018

johnynek Jun 27, 2018

ianoc-stripe Jun 28, 2018

johnynek Jun 28, 2018

ianoc-stripe commented Jun 27, 2018

johnynek commented Jun 28, 2018

johnynek commented Jun 28, 2018 •

edited

Loading

ianoc-stripe commented Jun 28, 2018

Aspect-based scrooge implementation #524

Aspect-based scrooge implementation #524

Conversation

johnynek commented Jun 16, 2018 • edited Loading

johnynek commented Jun 16, 2018

johnynek commented Jun 27, 2018

johnynek commented Jun 27, 2018

andyscott commented Jun 27, 2018

ittaiz left a comment

Choose a reason for hiding this comment

ittaiz commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianoc-stripe commented Jun 27, 2018

johnynek commented Jun 28, 2018

johnynek commented Jun 28, 2018 • edited Loading

ianoc-stripe commented Jun 28, 2018

johnynek commented Jun 16, 2018 •

edited

Loading

johnynek commented Jun 28, 2018 •

edited

Loading