intellij integration is broken #308

ittaiz · 2017-10-14T19:57:09Z

The failure with rules_scala master (relevant to 7b9b89b) looks related to scala_macro_library and ijars expectations.
@johnynek did we ever generate an ijar for scala_macro_library? I don't remember and it doesn't make a lot of sense
@chaoren These tests passed at some point right? Any idea how?

@greggdonovan all other tests pass?

The text was updated successfully, but these errors were encountered:

greggdonovan · 2017-10-15T01:26:58Z

@ittaiz Four of five tests fail with master. The testScalaBinary result actually looks unrelated to macros at first glance.

Here's a quick diff for adding intellij aspect testing to test_run.sh that you can use to reproduce it.

The approach of cloning or updating the intellij repo is ugly enough that I'm tempted to recommend a git submodule that tracks intellij master.

What sort of approach should we take to managing the dependency on intellij? Using a WORKSPACE dependency seems like the most Bazel-friendly way. Could we then bind io_bazel_rules_scala to ourself rather than using --override_repository?

ittaiz · 2017-10-15T03:14:36Z

Why do we need to manage the dependency on IntelliJ?

…

On Sun, 15 Oct 2017 at 4:26 Gregg Donovan ***@***.***> wrote: @ittaiz <https://github.com/ittaiz> Four of five tests fail with master. The testScalaBinary result actually looks unrelated to macros at first glance. Here's a quick diff <https://gist.github.com/greggdonovan/cd9596ac2bd74b6d6d597991a6f7d437> for adding intellij aspect testing to test_run.sh that you can use to reproduce it. The approach of cloning or updating the intellij repo is ugly enough that I'm tempted to recommend a git submodule that tracks intellij master <https://github.com/git/git/blob/master/Documentation/RelNotes/1.8.2.txt#L186:L189> . What sort of approach should we take to managing the dependency on intellij? Using a WORKSPACE dependency seems like the most Bazel-friendly way. Could we then bind io_bazel_rules_scala to ourself rather than using --override_repository? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#308 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFxrUdEmpzscL8U7UEk5b7FAQdoJqks5ssV9igaJpZM4P5ghB> .

greggdonovan · 2017-10-15T12:58:46Z

I was hoping we could keep the local CI times reasonable. I'm not sure about travis runtimes, as I don't know what the caching options are.

But, locally, doing a clean checkout of intellij.git and running the scala aspect integration tests takes five minutes.[1] Most of it is building protobuf and downloading large (300mb+) IntelliJ IDE artifacts for the tests.

If you're fine the approach in my diff above -- manually maintaining the intellij clone locally and creating it fresh every time in CI -- that seems like it could be fine.

[1] time { git clone [email protected]:bazelbuild/intellij.git; cd intellij; bazel test --profile ~/intellij_test_profile.out --test_output=errors --incompatible_disallow_set_constructor=false --override_repository io_bazel_rules_scala=/Users/gdonovan/development/rules_scala //aspect/testing/tests/src/com/google/idea/blaze/aspect/scala/...; }

Here's the result of bazel analyze-profile ~/intellij_test_profile.out


=== PHASE SUMMARY INFORMATION ===

Total launch phase time         63.0 ms    0.02%
Total init phase time            201 ms    0.07%
Total loading phase time        3.893 s    1.32%
Total analysis phase time     135.086 s   45.77%
Total preparation phase time    43.2 ms    0.01%
Total execution phase time    155.863 s   52.80%
Total finish phase time         23.2 ms    0.01%
Total run time                295.173 s  100.00%

=== INIT PHASE INFORMATION ===

Total init phase time                     201 ms

Total time (across all threads) spent on:
              Type    Total    Count     Average

=== LOADING PHASE INFORMATION ===

Total loading phase time                 3.893 s

Total time (across all threads) spent on:
              Type    Total    Count     Average
    CREATE_PACKAGE    0.66%       16     5.36 ms
          VFS_STAT    0.37%      683     0.07 ms
           VFS_DIR    3.87%      150     3.34 ms
           VFS_MD5    0.02%       15     0.16 ms
          VFS_OPEN    0.02%       26     0.11 ms
          VFS_READ    0.00%       23     0.03 ms
         VFS_WRITE    0.00%       11     0.02 ms
          VFS_GLOB    5.69%      827     0.89 ms
     SKYFRAME_EVAL   29.34%        4      948 ms
       SKYFUNCTION   32.78%     5050     0.84 ms
     SKYLARK_LEXER    0.08%      173     0.06 ms
    SKYLARK_PARSER    0.27%      173     0.20 ms
   SKYLARK_USER_FN   24.66%      392     8.13 ms
SKYLARK_BUILTIN_FN    1.45%     1253     0.15 ms

=== ANALYSIS PHASE INFORMATION ===

Total analysis phase time              135.086 s

Total time (across all threads) spent on:
              Type    Total    Count     Average
    CREATE_PACKAGE    0.03%       37     2.51 ms
          VFS_STAT    0.10%      256     1.23 ms
           VFS_DIR    0.01%      124     0.23 ms
          VFS_OPEN    0.00%       33     0.05 ms
          VFS_READ    0.00%       33     0.14 ms
         VFS_WRITE    7.42%      371     62.8 ms
          VFS_GLOB    0.04%      700     0.17 ms
     SKYFRAME_EVAL   43.00%        1   134.993 s
       SKYFUNCTION   49.11%     7767     19.9 ms
     SKYLARK_LEXER    0.00%      109     0.05 ms
    SKYLARK_PARSER    0.00%      109     0.07 ms
   SKYLARK_USER_FN    0.09%     1317     0.22 ms
SKYLARK_BUILTIN_FN    0.16%     3250     0.16 ms

=== EXECUTION PHASE INFORMATION ===

Total preparation time                   43.2 ms
Total execution phase time             155.863 s
Total time finalizing build              23.2 ms

Action dependency map creation           0.00 ms
Actual execution time                  155.863 s

Total time (across all threads) spent on:
              Type    Total    Count     Average
            ACTION    0.01%      513     0.28 ms
      ACTION_CHECK    0.00%        5     0.06 ms
    ACTION_EXECUTE   43.80%      513     1.440 s
     ACTION_UPDATE    0.00%      352     0.01 ms
   ACTION_COMPLETE    0.01%      513     0.45 ms
              INFO    0.00%        1     0.00 ms
          VFS_STAT    0.27%   265298     0.02 ms
           VFS_DIR    0.13%    16068     0.14 ms
           VFS_MD5    0.13%      942     2.31 ms
        VFS_DELETE    0.77%   115008     0.11 ms
          VFS_OPEN    0.01%      871     0.16 ms
          VFS_READ    0.00%     1092     0.01 ms
         VFS_WRITE    0.00%      571     0.12 ms
              WAIT    0.18%      812     3.84 ms
     SKYFRAME_EVAL    9.23%        1   155.694 s
       SKYFUNCTION   45.45%     9691     79.1 ms

Critical path (40.547 s):
    Id        Time Percentage   Description
 33901     1.458 s    3.60%   action 'Testing //aspect/testing/tests/src/com/google/idea/blaze/aspect/scala/scalalibrary ScalaLibraryTest'
 33900     0.06 ms    0.00%   runfiles for //aspect/testing/tests/src/com/google/idea/blaze/aspect/scala/scalalibrary ScalaLibraryTest
 33899     0.42 ms    0.00%   runfiles_artifacts for //aspect/testing/tests/src/com/google/idea/blaze/aspect/scala/scalalibrary ScalaLibraryTest
 33898      949 ms    2.34%   action 'Building Intellij Aspect Test Fixture'
 33897     0.12 ms    0.00%   runfiles for //aspect/testing/rules IntellijAspectTestFixtureBuilder
 33896     0.62 ms    0.00%   runfiles_artifacts for //aspect/testing/rules IntellijAspectTestFixtureBuilder
 33895    27.025 s   66.65%   action 'Building external/com_google_protobuf_java/libprotobuf_java.jar (77 source files, 1 source jar) [for host]'
 33894      203 ms    0.50%   action 'Executing genrule @com_google_protobuf_java// gen_well_known_protos_java [for host]'
 33893     0.12 ms    0.00%   runfiles for @com_google_protobuf_java// protoc
 33892      368 ms    0.91%   action 'Linking external/com_google_protobuf_java/protoc [for host]'
 33891      395 ms    0.97%   action 'Linking external/com_google_protobuf_java/libprotobuf.a [for host]'
 33890    10.148 s   25.03%   action 'Compiling external/com_google_protobuf_java/src/google/protobuf/descriptor.cc [for host]'
 33889     0.04 ms    0.00%   null for @com_google_protobuf_java// protobuf
 33888     0.06 ms    0.00%   null for @com_google_protobuf_java// protobuf_lite

ittaiz · 2017-10-15T19:09:59Z

+1 to reasonable CI times.
Haven't had too much experience with git submodules other than the common horror but this might actually be the appropriate use-case since we just want to "symlink" ourselves to the intellij plugin.
Do you want to take a stab at it and see how this effects the local work (mainly with bazel)?

greggdonovan · 2017-10-16T00:43:13Z

Haven't had too much experience with git submodules other than the common horror

:) My experience has been similar. I'll give this a try tomorrow. If the submodule horrors appear, we can use another approach... Related thought:

Should we use travis's directory caching to persist the Bazel output_base (perhaps an overriden output_base?) between CI runs?

Whether we use a submodule or manual cloning we are likely to add ~5 minutes to our Travis time without persisting the cache. Looking around, it appears Dagger and tensorflow/rust tried adding Bazel caching to Travis but reverted it after issues. But @jart got tensorboard caching working with some careful cache pruning and bazel command args. You think this is worth a shot?

ittaiz · 2017-10-16T07:15:44Z

@damienmg can we set up another ci.bazel.io job to do:

bazel test --test_output=errors --incompatible_disallow_set_constructor=false --override_repository io_bazel_rules_scala=${rules_scala_dir} //aspect/testing/tests/src/com/google/idea/blaze/aspect/scala/...

Where we clone rules_scala before? The trigger should be per PR. The reason is that we can't have multiple travis checks and sequentially this will add 5 minutes.

damienmg · 2017-10-16T07:17:15Z

Empty code section?

damienmg · 2017-10-16T07:18:27Z

Oops sorry it appeared now, maybe a glitch in GitHub. It cannot be done at the moment but the abseil people are interested in something similar.

damienmg · 2017-10-16T07:31:56Z

Humm there might be a workaround actually: use the configure step of the CI to do the git clone. I would prefer to do that correctly in the end but that should work for now.

ittaiz · 2017-10-16T09:51:05Z

great!
Can we (@greggdonovan or myself) help in any way?
Just so we're on the same page- The idea is that given a PR on rules_scala an additional job will be triggered with the commit of that PR against intellij-plugin HEAD where we run the above command on the intellij plugin workspace

damienmg · 2017-10-16T09:56:14Z

We cannot add an additional job (haven't investigated why but Jenkins kill one of the PR vetting when the other finish).

I was thinking of adding just a config to the existing job that does this in the configuration step: rm -fr bazel-rules_scala; git clone --depth 1 https://github.com/bazelbuild/bazel-rules_scala And then add --override_repository io_bazel_rules_scala=bazel-rules_scala.

But IIUC you want to run intellij test on rules_scala PR? We need to fix the bug that prevent us from triggering 2 jobs from one PR (I don't know what is the problem exactly, haven't investigated further, need to prioritize it).

ittaiz · 2017-10-16T10:03:55Z

yes you understand me correctly.
The problem is that we have an integration between rules_scala and the intellij plugin which can be broken by both sides (in fact it's currently broken).
Intellij plugin have tests to cover this but they aren't run on any of rules_scala's CI events (PR push/PR merge).
It sounds to me like we should continue with @greggdonovan's current plan which adds 5 minutes to the build and open a new issue to focus on the addition of the check in a separate job.
@johnynek please raise an objection if you have any since currently we'll continue with this path although it has its drawbacks.
@damienmg should we open the issue in https://github.com/bazelbuild/continuous-integration?

@greggdonovan I suggest you open a PR with the above outline, if Oscar won't object until tomorrow we'll continue with it. Not sure we'll be able to merge it since it will break the build but we'll at least have visibility.

damienmg · 2017-10-16T10:13:36Z

Filed bazelbuild/continuous-integration#142 for the Jenkins issue and bazelbuild/continuous-integration#143 for the nicer feature. The latest is not prioritized but I think we should work now on the former.

ittaiz · 2017-10-16T10:15:55Z

thanks!
Do you think we should wait with this integration for bazelbuild/continuous-integration#142?
If it's a matter of a few days I'd rather wait for it

damienmg · 2017-10-16T10:20:36Z

Unfortunately, there is nobody working on CI anymore, Klaus and I barely maintains it. So until we figure out what is happening with that, I cannot guarantee any fix soon.

…

On Mon, Oct 16, 2017 at 12:15 PM Ittai Zeidman ***@***.***> wrote: thanks! Do you think we should wait with this integration for bazelbuild/continuous-integration#142 <bazelbuild/continuous-integration#142>? If it's a matter of a days I'd rather wait for it — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#308 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADjHf1_2TJw_KqkfxQO9hF8mGwLm0BmFks5ssyzcgaJpZM4P5ghB> .

ittaiz · 2017-10-16T11:14:28Z

Ok. Gregg, Back to original plan On Mon, 16 Oct 2017 at 13:20 Damien Martin-Guillerez < [email protected]> wrote:

…

Unfortunately, there is nobody working on CI anymore, Klaus and I barely maintains it. So until we figure out what is happening with that, I cannot guarantee any fix soon. On Mon, Oct 16, 2017 at 12:15 PM Ittai Zeidman ***@***.***> wrote: > thanks! > Do you think we should wait with this integration for > bazelbuild/continuous-integration#142 > <bazelbuild/continuous-integration#142>? > If it's a matter of a days I'd rather wait for it > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > < #308 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/ADjHf1_2TJw_KqkfxQO9hF8mGwLm0BmFks5ssyzcgaJpZM4P5ghB > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#308 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF5SqG9Jxb2RsA_L187fBFmLXeUcSks5ssy33gaJpZM4P5ghB> .

greggdonovan · 2017-10-16T13:50:33Z

@ittaiz OK, sounds good. Three other thoughts:

Travis caching will not help much for intellij fetching large IDE files, as the large cache artifacts will need to be downloaded from s3. So, that's ~135s of addition to the CI build that will be hard to avoid.
We could hope to save time if we cached the build of protobuf that intellij does (155s in my test). Is there a way to bind our already build protobuf to something that we can pass to bazel test in the intellij repo?
It seems like we should be testing intellij master and a git tag/version representing the currently released version of the intellij plugin.

Breakage in intellij-master tells us our rules_scala change will break the plugin when master is released. Breakage in intellij-release tells us it will break our intellij users right now.

What do you think?

ittaiz · 2017-10-16T14:12:46Z

I might be wrong but my instincts are hinting against using too many caching mechanisms, overrides and manual deletions.
My main concern is we'll hurt correctness by not deleting enough.
WDYT?

Re the test matrix- they don't have tags representing the versions since they often don't correlate. I agree with what you're saying but I'm not sure we can control it given @brendandouglas doesn't have the capacity to auto-sync their code.

greggdonovan · 2017-10-16T14:32:14Z

I might be wrong but my instincts are hinting against using too many caching mechanisms, overrides and manual deletions... My main concern is we'll hurt correctness by not deleting enough.

I agree. After going through the pros/cons of travis caching or overriding protobuf I'm leaning against it. We may save 2-3 minutes in CI here or there but we may spend quite a bit of time debugging any issues.

I think you're approach of having multiple jobs ultimately makes the most sense for keeping CI times reasonable. It's also be nice to separate out the intellij failures from rules_scala as a whole, as you noted.

Re the test matrix- they don't have tags representing the versions since they often don't correlate. I agree with what you're saying but I'm not sure we can control it given @brendandouglas doesn't have the capacity to auto-sync their code.

Here's a version of the diff above testing against both master and what I manually inspected to be the commit representing the live version of the plugin. Clearly this is not ideal, but ultimately it would be nice to have that test matrix. I'll make a ticket for @brendandouglas to see if that can add a git tag when releasing the plugin.

they don't have tags representing the versions since they often don't correlate.

Do you mean the released version of the plugin doesn't match the source code in the public github?

ittaiz · 2017-10-16T15:09:35Z

LGTM.

Do you mean the released version of the plugin doesn't match the source code in the public github?

yes. Brendan said they're not always able to keep them up to date. I also saw that there was a release without a code drop so you couldn't fix any problems of the plugin for yourself if you encountered any.

johnynek · 2017-10-17T01:26:56Z

we did used to make an ijar which was just a copy of the regular jar for scala_macro_library.

Maybe with scala_import we can just always use the full jar along with the java_provider and things would just work?

ittaiz · 2017-10-17T03:22:06Z

Hm, ok. Then this was changed. Not sure it's a problem though. I'll try to take a look tomorrow at the rest of the failures and see if there is a common denominator. scala_import won't help here since it's for external code unless I'm missing your point. scala_macro_library is its counterpart for internal code

…

On Tue, 17 Oct 2017 at 4:26 P. Oscar Boykin ***@***.***> wrote: we did used to make an ijar which was just a copy of the regular jar for scala_macro_library. Maybe with scala_import we can just always use the full jar along with the java_provider and things would just work? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#308 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFySy4_oxjtPqD5T-02XI3ekI5MiVks5stAJigaJpZM4P5ghB> .

ittaiz mentioned this issue Oct 14, 2017

object of type 'File' has no field 'class_jar'. error from intellij plugin when syncing scala targets #277

Closed

damienmg mentioned this issue Oct 16, 2017

Allow injecting dependencies through --override_repository bazelbuild/continuous-integration#143

Closed

greggdonovan mentioned this issue Oct 16, 2017

Create a git tag representing the currently released version of the plugin bazelbuild/intellij#146

Closed

greggdonovan mentioned this issue Oct 17, 2017

Integrate intellij scala aspect integration testing into rules_scala #309

Merged

ittaiz closed this as completed Jul 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

intellij integration is broken #308

intellij integration is broken #308

ittaiz commented Oct 14, 2017

greggdonovan commented Oct 15, 2017

ittaiz commented Oct 15, 2017 via email

greggdonovan commented Oct 15, 2017

ittaiz commented Oct 15, 2017

greggdonovan commented Oct 16, 2017

ittaiz commented Oct 16, 2017 •

edited

Loading

damienmg commented Oct 16, 2017

damienmg commented Oct 16, 2017

damienmg commented Oct 16, 2017

ittaiz commented Oct 16, 2017

damienmg commented Oct 16, 2017

ittaiz commented Oct 16, 2017

damienmg commented Oct 16, 2017

ittaiz commented Oct 16, 2017 •

edited

Loading

damienmg commented Oct 16, 2017 via email

ittaiz commented Oct 16, 2017 via email

greggdonovan commented Oct 16, 2017

ittaiz commented Oct 16, 2017

greggdonovan commented Oct 16, 2017

ittaiz commented Oct 16, 2017 •

edited

Loading

johnynek commented Oct 17, 2017

ittaiz commented Oct 17, 2017 via email

intellij integration is broken #308

intellij integration is broken #308

Comments

ittaiz commented Oct 14, 2017

greggdonovan commented Oct 15, 2017

ittaiz commented Oct 15, 2017 via email

greggdonovan commented Oct 15, 2017

ittaiz commented Oct 15, 2017

greggdonovan commented Oct 16, 2017

ittaiz commented Oct 16, 2017 • edited Loading

damienmg commented Oct 16, 2017

damienmg commented Oct 16, 2017

damienmg commented Oct 16, 2017

ittaiz commented Oct 16, 2017

damienmg commented Oct 16, 2017

ittaiz commented Oct 16, 2017

damienmg commented Oct 16, 2017

ittaiz commented Oct 16, 2017 • edited Loading

damienmg commented Oct 16, 2017 via email

ittaiz commented Oct 16, 2017 via email

greggdonovan commented Oct 16, 2017

ittaiz commented Oct 16, 2017

greggdonovan commented Oct 16, 2017

ittaiz commented Oct 16, 2017 • edited Loading

johnynek commented Oct 17, 2017

ittaiz commented Oct 17, 2017 via email

ittaiz commented Oct 16, 2017 •

edited

Loading

ittaiz commented Oct 16, 2017 •

edited

Loading

ittaiz commented Oct 16, 2017 •

edited

Loading