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.

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

Plugin fails to get metadata if java/scala compilation fails #1167

Closed
or-shachar opened this issue Sep 13, 2019 · 34 comments
Closed

Plugin fails to get metadata if java/scala compilation fails #1167

or-shachar opened this issue Sep 13, 2019 · 34 comments
Labels
lang: java Java rules integration lang: scala Scala rules integration product: IntelliJ IntelliJ plugin stale Issues or PRs that are stale (no activity for 30 days) topic: sync Issues related to the sync operation type: bug

Comments

@or-shachar
Copy link

Using bazel scala project we noticed that if compilation fails the plugin fails to get the metadata.

When we try to sync currently the plugin runs 2 bazel commands:

- bazel info ...
- bazel build ... --output_groups=intellij-info-generic,intellij-info-java,intellij-resolve-java

We noticed that if we remove the intellij-resolve-java the second operation will succeed.

We should probably break the second bazel invocation to two phases so we will still get the metadata even if the compilation fails.

cc: @jin @chaoren @ittaiz

@jin jin self-assigned this Sep 13, 2019
@jin jin added product: IntelliJ IntelliJ plugin lang: java Java rules integration lang: scala Scala rules integration labels Sep 13, 2019
@brendandouglas
Copy link
Contributor

There are a few reasons for the current situation:

  • running bazel build invocations has a very large overhead internally (7-8 seconds when run remotely, before any bazel work is actually done), so we want to minimize the number of builds.

  • there's no clear distinction between the 'metadata' (target graph, target details) and the output of the resolve step (compilation output, in the case of java and scala). They're co-dependent, so updating the metadata without resolve output can lead to bad behavior.

Instead, we assume that the 'resolve' build will succeed initially (for instance, because the user is checking out code without local modifications). Any subsequent builds for which some targets fail to compile will result in that target's 'metadata' and compilation output remaining unchanged, which is generally the desired outcome. We'd need a very good reason to consider changing this approach.

@ittaiz
Copy link
Member

ittaiz commented Sep 16, 2019

Thanks for responding!
Can you elaborate when/how updating the metadata without resolve output can lead to bad behavior? Not sure I understand why that is and for a very very long time the two commands were indeed separate.

I think part of the problem in the above assumption is that it's a bit too clean. Sometimes someone will rebase onto master they'll have a non compiling file. If this file is in the "middle" of the graph then all dependent targets will be messed up even if they are new from master. This is something that's not trivial to track down without deeply investigating the tool or really understanding Bazel.
We'd love to give users a simpler experience then what is now expected from people.

@brendandouglas
Copy link
Contributor

I did combine the two build steps recently (a few months ago) for performance reasons, but I can't think of a situation where that would actually cause a change in behavior like you're seeing.

We request all the build outputs in the same build invocation, but we still handle the 'info' and 'resolve' outputs separately -- that hasn't changed. If individual targets fail to build, we should still be updating their metadata in the project data.

It might help to have a small reproducer here (ideally in java if possible).

@ittaiz
Copy link
Member

ittaiz commented Sep 20, 2019

I'll add a repro no problem. Main issue is where A->B->C and B doesn't compile. Then you don't get C's info outputs (of course it could be a very long and complicated sub tree of targets)

@ittaiz
Copy link
Member

ittaiz commented Sep 20, 2019

repro repo: https://github.com/ittaiz/bazel-ij-info-with-resolve
After sync B and A are unsynced:
Screen Shot 2019-09-20 at 18 04 40
Because of a compilation issue in B:
Screen Shot 2019-09-20 at 18 04 54
As you can see the external dependencies of B and of A aren't resolved:
Screen Shot 2019-09-20 at 18 05 10

@jin
Copy link
Member

jin commented Sep 20, 2019

The current behavior seems to fit my intuition again. It all boils down to this:

@guava is a dependency of B, and only B in the entire project. If B fails to provide the intellij-resolve-java output group requested by the aspect, the aspect will still continue traversing the dep edge up to @guava (because of --keep_going). However, B will not be synced, and it seems like @guava's resolve info isn't added to the index because of that.

However, if you add @guava as a dep of C, and because C compiles, the aspect can travel to @guava and complete the intellij-resolve-java output group. Now, even if B doesn't compile, its import com.google.common.cache.AbstractCache import statement is no longer red because C synced successfully and allowed @guava to be added to the target index.

This has nothing to do with the difference between intellij-info-java and intellij-resolve-java. intellij-info-java doesn't provide the information required to fix the red code issue.

Interestingly, even though B is the only rdep of @guava and B's compilation failed, the ijar and intellij-info.txt are created:

$ tree bazel-bin/external/com_google_guava_guava/
bazel-bin/external/com_google_guava_guava/
├── com_google_guava_guava-46378293.intellij-info.txt
└── _ijar
    └── com_google_guava_guava
        └── external
            └── com_google_guava_guava
                └── guava-20.0-ijar.jar

So the fix here should be adding @guava into the target index even if all its callers did not compile and sync successfully.

@brendandouglas
Copy link
Contributor

Thanks for the repro project, I see the problem.

The part I was missing is that the intellij-info.txt output artifact is produced (expected), but isn't declared in the BEP output (unexpected).

This feels like a bug in BEP. Prior to BEP we relied on parsing stdout/stderr, with the --experimental_show_artifacts flag set. The intellij-info outputs for B are declared just fine there, regardless of whether the jars are successfully compiled.

@jin
Copy link
Member

jin commented Sep 20, 2019

@brendandouglas are you saying the root cause is that these two lists aren't consistent with each other?

$ tree -fi bazel-bin/ | grep intellij-info.txt
bazel-bin/A-65.intellij-info.txt
bazel-bin/B-66.intellij-info.txt
bazel-bin/C-67.intellij-info.txt
bazel-bin/external/bazel_tools/tools/jdk/current_java_toolchain--1286701614.intellij-info.txt
bazel-bin/external/com_google_code_findbugs_jsr305/com_google_code_findbugs_jsr305-1219275564.intellij-info.txt
bazel-bin/external/com_google_errorprone_error_prone_annotations/com_google_errorprone_error_prone_annotations--1327292041.intellij-info.txt
bazel-bin/external/com_google_guava_guava/com_google_guava_guava-46378293.intellij-info.txt
bazel-bin/external/google_java_format/google_java_format--2076037202.intellij-info.txt
bazel-bin/external/remote_java_tools_linux/toolchain--412636119.intellij-info.txt
$ cat /tmp/bep.txt | grep intellij-info.txt
    name: "external/remote_java_tools_linux/toolchain--412636119.intellij-info.txt"
    uri: "file:///usr/local/google/home/jingwen/.cache/bazel/_bazel_jingwen/af0112346abd86e45930298bb524e045/execroot/main/bazel-out/k8-fastbuild/bin/external/remote_java_tools_linux/toolchain--412636119.intellij-info.txt"
    name: "external/bazel_tools/tools/jdk/current_java_toolchain--1286701614.intellij-info.txt"
    uri: "file:///usr/local/google/home/jingwen/.cache/bazel/_bazel_jingwen/af0112346abd86e45930298bb524e045/execroot/main/bazel-out/k8-fastbuild/bin/external/bazel_tools/tools/jdk/current_java_toolchain--1286701614.intellij-info.txt"
    name: "C-67.intellij-info.txt"
    uri: "file:///usr/local/google/home/jingwen/.cache/bazel/_bazel_jingwen/af0112346abd86e45930298bb524e045/execroot/main/bazel-out/k8-fastbuild/bin/C-67.intellij-info.txt"

If we omit intellij-resolve-java from the list of requested output groups, the BEP shows the correct set of files:

$ cat /tmp/bep.txt | grep intellij-info.txt
    name: "external/remote_java_tools_linux/toolchain--412636119.intellij-info.txt"
    uri: "file:///usr/local/google/home/jingwen/.cache/bazel/_bazel_jingwen/af0112346abd86e45930298bb524e045/execroot/main/bazel-out/k8-fastbuild/bin/external/remote_java_tools_linux/toolchain--412636119.intellij-info.txt"
    name: "external/bazel_tools/tools/jdk/current_java_toolchain--1286701614.intellij-info.txt"
    uri: "file:///usr/local/google/home/jingwen/.cache/bazel/_bazel_jingwen/af0112346abd86e45930298bb524e045/execroot/main/bazel-out/k8-fastbuild/bin/external/bazel_tools/tools/jdk/current_java_toolchain--1286701614.intellij-info.txt"
    name: "C-67.intellij-info.txt"
    uri: "file:///usr/local/google/home/jingwen/.cache/bazel/_bazel_jingwen/af0112346abd86e45930298bb524e045/execroot/main/bazel-out/k8-fastbuild/bin/C-67.intellij-info.txt"
    name: "external/com_google_code_findbugs_jsr305/com_google_code_findbugs_jsr305-1219275564.intellij-info.txt"
    uri: "file:///usr/local/google/home/jingwen/.cache/bazel/_bazel_jingwen/af0112346abd86e45930298bb524e045/execroot/main/bazel-out/k8-fastbuild/bin/external/com_google_code_findbugs_jsr305/com_google_code_findbugs_jsr305-1219275564.intellij-info.txt"
    name: "external/google_java_format/google_java_format--2076037202.intellij-info.txt"
    uri: "file:///usr/local/google/home/jingwen/.cache/bazel/_bazel_jingwen/af0112346abd86e45930298bb524e045/execroot/main/bazel-out/k8-fastbuild/bin/external/google_java_format/google_java_format--2076037202.intellij-info.txt"
    name: "external/com_google_errorprone_error_prone_annotations/com_google_errorprone_error_prone_annotations--1327292041.intellij-info.txt"
    uri: "file:///usr/local/google/home/jingwen/.cache/bazel/_bazel_jingwen/af0112346abd86e45930298bb524e045/execroot/main/bazel-out/k8-fastbuild/bin/external/com_google_errorprone_error_prone_annotations/com_google_errorprone_error_prone_annotations--1327292041.intellij-info.txt"
    name: "A-65.intellij-info.txt"
    uri: "file:///usr/local/google/home/jingwen/.cache/bazel/_bazel_jingwen/af0112346abd86e45930298bb524e045/execroot/main/bazel-out/k8-fastbuild/bin/A-65.intellij-info.txt"
    name: "external/com_google_guava_guava/com_google_guava_guava-46378293.intellij-info.txt"
    uri: "file:///usr/local/google/home/jingwen/.cache/bazel/_bazel_jingwen/af0112346abd86e45930298bb524e045/execroot/main/bazel-out/k8-fastbuild/bin/external/com_google_guava_guava/com_google_guava_guava-46378293.intellij-info.txt"
    name: "B-66.intellij-info.txt"
    uri: "file:///usr/local/google/home/jingwen/.cache/bazel/_bazel_jingwen/af0112346abd86e45930298bb524e045/execroot/main/bazel-out/k8-fastbuild/bin/B-66.intellij-info.txt"

@brendandouglas
Copy link
Contributor

brendandouglas commented Sep 20, 2019 via email

@jin
Copy link
Member

jin commented Sep 20, 2019

Thanks, filed bazelbuild/bazel#9413

@ittaiz
Copy link
Member

ittaiz commented Sep 21, 2019

Interesting, thanks!
Just to close the Bazel understanding gap- I expected A's intellij-info files not to be created because B failed. Are they created because these are in reality two different graphs (one for the resolve-java actions and one for the intellij/java-info actions)?

@ittaiz
Copy link
Member

ittaiz commented Sep 21, 2019

Also,
@brendandouglas thanks for iterating with us on this. It indeed seems like the BEP fix is better for everyone (blaze and bazel). Is there a cheap workaround you think we can do in the meanwhile (I know you're not a fan of temporary issues)?

@brendandouglas
Copy link
Contributor

Thanks for filing the Bazel-side bug @jin.

@ittaiz the intellij-info outputs are not dependent on the compilation outputs, so in theory they could be implemented with different graphs. In practice I doubt they are though.

There's no cheap workaround, so I'll wait to hear back from the Bazel team. If they're willing to support this use case, I'll leave as is and wait for a Bazel-side fix. If not, I'll conditionally separate the 'info' and 'resolve' steps.

@jin
Copy link
Member

jin commented Sep 23, 2019

the intellij-info outputs are not dependent on the compilation outputs, so in theory they could be implemented with different graphs. In practice I doubt they are though.

Technically they are different subgraphs in the Skyframe-sense, since the intellij-info outputs were requested by a different output group from the compilation output group.

I'll look into this issue. It's a particularly specific but important edge case for IDE use cases.

@jin
Copy link
Member

jin commented Sep 25, 2019

Digging into this a little deeper, this issue only seems to happen if you're doing an initial sync and that the Java code doesn't compile.

If you've successfully performed an initial sync, and then you sync again and a compilation error happens (e.g. B in the repro project), the metadata of the rest of the files don't get affected. This seems WAI to me.

@ittaiz @or-shachar could you please confirm about my diagnosis in your repro project?

@ittaiz
Copy link
Member

ittaiz commented Sep 26, 2019

@jin I'm not sure I understand. Is the scenario you're describing about whether or not this issue also discards existing metadata of previously resolved and working files (A)?
I think you're right. However if there are any changes (for example pulled from upstream) then those aren't synced and are broken. Is the use-case clear?

@ittaiz
Copy link
Member

ittaiz commented Oct 2, 2019

@brendandouglas @jin if Bazel won't support this use-case what do you think about running the info command only if the current command fails? This way most times people will still get the perf benefit. Additionally the change if they will support the use-case later on will just be dropping this extra call.

@jin
Copy link
Member

jin commented Oct 10, 2019

The info command always runs before the command that builds the aspect. Do you mean separating out intellij-info-java into a separate command if intellij-resolve-java fails?

@ittaiz
Copy link
Member

ittaiz commented Oct 10, 2019

Yes, exactly

@ittaiz
Copy link
Member

ittaiz commented Nov 3, 2019

@brendandouglas wdyt about adding a fallback bazel build invocation without intellij-resolve-java if the first fails? Sounds simple and without a perf hit in most cases.
Main motivation is that we have a feature where you can add classes from the monorepo which IntelliJ doesn't currently know (using IntelliJ's smart auto complete). This feature is severely broken since we currently:

  1. Add the symbol chosen by the user to the editor and also to the target.
  2. Tell the user to comment out the symbol and IJ.
  3. Trigger partial sync on the file.
  4. Comment back in.
  5. Use alt+enter to add an import.

What we'd like is:

  1. Add the symbol chosen by the user to the editor and also to the target.
  2. Programmatically trigger partial sync on the file.
  3. Use alt+enter to add an import.

For the user it's to go from 5 operations to 2 where often people have a hard time because they forget to comment out the symbol and so the sync fails and they waste some time on this.

We'd be really interested in contributing this change if it's ok with you.

@brendandouglas
Copy link
Contributor

I'd prefer to push for a Bazel-side fix here, rather than adding more complexity to the sync process. I've pinged the bazel-side bug and filed an internal bug for this issue -- let's see what the response is.

In the meantime, there's one part of that situation I don't understand. If both the source file and target deps are updated in step 1, why does the build fail in step 3 (without step 2)?

@ittaiz
Copy link
Member

ittaiz commented Nov 13, 2019

Good question (and sorry for missing the notification)- what we add in step 1 is merely the symbol in the editor.
For example imagine someone adding a new variable Foo a and while they're typing Fo nothing is recognized so they use smart autocomplete and what we do is complete Fo to Foo.
We don't add the import above because we need a PsiElement to do that correctly without messing with imports and such and so compilation fails (Foo doesn't yet have an import).
Does that make sense?

@brendandouglas
Copy link
Contributor

Thanks, that workflow makes sense. Though I don't understand how the ide-info data helps.

I'd expect that the target is already synced prior to step 1, so adding 'Foo' to the source file then syncing wouldn't change anything (you'd still be missing both the import and dep and syncing wouldn't add them)

@ittaiz
Copy link
Member

ittaiz commented Nov 13, 2019

The difference would have been in IJ-Bazel plugin "discovering" about @foo and configuring it as an IntelliJ library. This in turn means that IntelliJ will index that jar and suggest the user to click alt+enter to add the import.
The idea is indeed that everything is synced prior to step 1 and then in it we add Foo to the editor and @foo to the deps of the relevant target.

If you think I'm missing something I'd love to know (I'm fairly certain this used to work for us before the unification to one invocation)

@ittaiz
Copy link
Member

ittaiz commented Nov 13, 2019

Also a separate use-case (and the one that caused us to look at this issue) is when you're working on prod code A and it doesn't compile for some reason and you're switching to the test target ATest to add some code that requires a new dependency you need to "blindly" code in the editor because "new-test-dep" won't be identified until you stash changes to A and probably to ATest as well.
I'm re-surfacing this because one might suggest to just add the import manually somehow in the autocomplete scenario above but that will only solve one part of our needs.
Thanks in advance!

WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Jan 13, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Jan 14, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Jan 15, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Jan 18, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Jan 19, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Jan 20, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Jan 21, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Jan 22, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Jan 25, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Jan 26, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Jan 27, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Jan 28, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Feb 1, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Feb 8, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Feb 9, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Feb 11, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Feb 12, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Feb 15, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Feb 16, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Feb 17, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Feb 18, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Feb 19, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Feb 23, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Mar 1, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
WixBuildServer pushed a commit to wix-playground/intellij that referenced this issue Mar 3, 2022
…ith info only

This is to make sure we have the needed metadata even if build broke
Relates to:
bazelbuild#1167
bazelbuild/bazel#9413

Fix test
@github-actions
Copy link

Thank you for contributing to the IntelliJ repository! This issue has been marked as stale since it has not had any activity in the last 2 years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-maintainer". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 28, 2023
@github-actions
Copy link

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang: java Java rules integration lang: scala Scala rules integration product: IntelliJ IntelliJ plugin stale Issues or PRs that are stale (no activity for 30 days) topic: sync Issues related to the sync operation type: bug
Projects
None yet
Development

No branches or pull requests

6 participants