Select source roots for library jars that have interim folders like `… #5117

dkashyn-sfdc · 2023-07-17T22:08:14Z

…/src/...`. IJ UI logic is reused and suggested candidate selected if and only if there is one candidate exists.

Checklist

I have filed an issue about this change and discussed potential changes with the maintainers.
I have received the approval from the maintainers to make this change.
This is not a stylistic, refactoring, or cleanup change.

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.

Discussion thread for this change

Issue number: <please reference the issue number or url here>

Description of this change

…/src/...`. IJ UI logic is reused and suggested candidate selected if and only if there is one candidate exists.

google-cla · 2023-07-17T22:08:17Z

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).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

…m a single source jar.

tpasternak · 2023-07-19T14:48:10Z

So finally I managed to reproduce this issue, here's the minimal repro

SCENARIO:

Import the project below with x.projectview
Open Main.java
Try to go to Library definition from there

Instead of jumping to //lib:lib's src jar, IntelliJ jumps to decompiled code, because the liblib-src.jar has been attached in the wrong way

──────────────────────────────────────────────────────────────────────────────────────
File: lib/BUILD
──────────────────────────────────────────────────────────────────────────────────────
java_library(
    name = "lib",
    srcs = glob(["java/**/*.java"]),
    visibility = ["//visibility:public"],
)
──────────────────────────────────────────────────────────────────────────────────────
──────────────────────────────────────────────────────────────────────────────────────
File: lib/java/src/org/example/Library.java
──────────────────────────────────────────────────────────────────────────────────────
package org.example;

public class Library {

}
──────────────────────────────────────────────────────────────────────────────────────
──────────────────────────────────────────────────────────────────────────────────────
File: src/BUILD
──────────────────────────────────────────────────────────────────────────────────────
java_binary(
    name = "binary",
    srcs = ["Main.java"],
    main_class = "Main",
    deps = [
        "//lib",
    ],
)
──────────────────────────────────────────────────────────────────────────────────────
──────────────────────────────────────────────────────────────────────────────────────
File: src/Main.java
──────────────────────────────────────────────────────────────────────────────────────
import org.example.Library;

class Main {

    Library l;
    public static void main(String[] args) {
        System.out.println("Hello world");
    }
}
──────────────────────────────────────────────────────────────────────────────────────
──────────────────────────────────────────────────────────────────────────────────────
File: x.bazelproject
──────────────────────────────────────────────────────────────────────────────────────
directories:
  src

derive_targets_from_directories: false

targets:
  //src/...
──────────────────────────────────────────────────────────────────────────────────────

tpasternak · 2023-07-19T14:57:01Z

This could be related to these long-outstanding issues
#1206
#1207
bazelbuild/bazel#8277

tpasternak · 2023-07-19T15:27:06Z

base/src/com/google/idea/blaze/base/sync/libraries/LibraryModifier.java

@@ -73,7 +79,20 @@ private void addRoot(File file, OrderRootType orderRootType) {
      logger.warn("No local file found for " + file);
      return;
    }
-    modifiableModel.addRoot(pathToUrl(file), orderRootType);
+    if (Registry.is("bazel.sync.detect.source.roots") && orderRootType == OrderRootType.SOURCES) {


I'm not sure, but maybe Instead of doing it in LibraryModifier.addRoots, which is called in a lot of contexts, we could create a new method here? This method could be then called by BlazeAttachSourceProvider.attachSources instead of LibraryEditor.updateLibrary.

In this way, we could ensure that the scanning is not performed anywhere else except on-demand source attachment task.

the problem is that, the JavaVfsSourceRootDetectionUtil.suggestRoots call can be quite havy - it recursively scans the whole jar. https://github.com/JetBrains/intellij-community/blob/d0750485a3e0fd0123b8388478a41010c9c329ce/java/idea-ui/src/com/intellij/openapi/roots/ui/configuration/JavaVfsSourceRootDetectionUtil.java#L44

On the other hand it should not be much heavier than the indexing task that is going to be done right after attachment.

I saw issues in "Find Usages" with sources detection as well. I do think that proper and complete IJ project model is required since we are attaching sources there and they are incorrect. As a result it can cause some other source navigation/usage issues.

I do agree that IO cost might be too high and open to any discussions here.

This is one of the reasons to introduce custom registry key to enable this only if this is the only way to have proper sources.

Yeah, but the sources are attached on-demand, so the whole change does not affect find usages unless you try to jump to sources for all the libraries. So please let's do it this way. The behavior will be exactly the same, but we will guarantee that the heavy detection won't be called accidentally.

Apart from this, if you change the code in BlazeAttachSourceProvider.attachSources and put the new code in java module, you will fix the clion compatiblity issue for free.

tpasternak

So let's merge it (with the feature flag turned off by default), but apart from in-line comments, please adjust the examples/java/greeting_project in the way it can be used to show the issue.

base/src/META-INF/blaze-base.xml

tpasternak · 2023-07-21T09:11:00Z

base/src/com/google/idea/blaze/base/sync/libraries/LibraryModifier.java

+      VirtualFile jarFile = VirtualFileManager.getInstance().findFileByUrl(pathToUrl(file));
+      List<VirtualFile> candidates = Collections.emptyList();
+      if (jarFile != null) {
+        candidates = JavaVfsSourceRootDetectionUtil.suggestRoots(jarFile, new EmptyProgressIndicator());


Please use ProgressManager instead of EmptyProgressIndicator (ideally with cancellation handled correctly in case this was really slow)

…roots-detection

1. Moved some jar class/source related logic to BazelJarLibrary class from `base` module LibraryModifier. 2. Added proper progress indicator. 3. Applied registry key description edits suggestions.

dkashyn-sfdc · 2023-07-30T16:56:01Z

please adjust the examples/java/greeting_project in the way it can be used to show the issue.

Not sure what kind of changes I can make in those sample projects... should I create new package with glob and custom sources placement? How to make sure that registry key is used for that package?

…t there

dkashyn-sfdc · 2023-07-30T17:35:09Z

@tpasternak could you please check again? Addressed all but 1 review comments.

tpasternak · 2023-08-01T11:36:00Z

base/src/com/google/idea/blaze/base/sync/libraries/LibraryModifier.java

-  private String pathToUrl(File path) {
-    String name = path.getName();
-    boolean isJarFile =
-        FileUtilRt.extensionEquals(name, "jar")
-            || FileUtilRt.extensionEquals(name, "srcjar")
-            || FileUtilRt.extensionEquals(name, "zip");
-    // .jar files require an URL with "jar" protocol.
-    String protocol =
-        isJarFile
-            ? StandardFileSystems.JAR_PROTOCOL
-            : VirtualFileSystemProvider.getInstance().getSystem().getProtocol();
-    String filePath = FileUtil.toSystemIndependentName(path.getPath());
-    String url = VirtualFileManager.constructUrl(protocol, filePath);
-    if (isJarFile) {
-      url += URLUtil.JAR_SEPARATOR;
-    }
-    return url;


Please only move this code to the original place (and change access modifier to static). We regularly cherry pick cahnges from the google branch, so we want to keep the changes as small as possible

base/src/com/google/idea/blaze/base/sync/libraries/LibraryModifier.java

…roots-detection

dkashyn-sfdc · 2023-08-02T12:05:47Z

For some reason, dkashyn-sfdc@f7d0e7c is not reflected in the PR.

Select source roots for library jars that have interim folders like `…

5fbd5cf

…/src/...`. IJ UI logic is reused and suggested candidate selected if and only if there is one candidate exists.

dkashyn-sfdc requested review from mai93, jastice and tpasternak as code owners July 17, 2023 22:08

Add registry value to conditionally import all of the source dirs fro…

563474f

…m a single source jar.

Pavank1992 added the awaiting-review Awaiting review from Bazel team on PRs label Jul 18, 2023

dkashyn-sfdc added 2 commits July 18, 2023 09:48

Registry key definition

75b79a2

Merge branch 'master' into non-default-source-roots-detection

c101401

tpasternak reviewed Jul 19, 2023

View reviewed changes

sgowroji added the product: IntelliJ IntelliJ plugin label Jul 20, 2023

tpasternak requested changes Jul 21, 2023

View reviewed changes

base/src/META-INF/blaze-base.xml Outdated Show resolved Hide resolved

tpasternak reviewed Jul 21, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into non-default-source-…

d011260

…roots-detection

sgowroji assigned tpasternak Jul 28, 2023

dkashyn-sfdc added 4 commits July 28, 2023 09:28

Merge branch 'master' into non-default-source-roots-detection

587d7e2

Merge branch 'master' into non-default-source-roots-detection

ca43fce

Addressed PR comments:

af4d35b

1. Moved some jar class/source related logic to BazelJarLibrary class from `base` module LibraryModifier. 2. Added proper progress indicator. 3. Applied registry key description edits suggestions.

Cleanup unused imports

f2eb19a

dkashyn-sfdc added 2 commits July 30, 2023 13:15

Created default impls to not upset existing LibrarySourceProviders ou…

fa4684e

…t there

Proper message for progress task

b846629

dkashyn-sfdc requested a review from tpasternak July 30, 2023 17:34

tpasternak reviewed Aug 1, 2023

View reviewed changes

base/src/com/google/idea/blaze/base/sync/libraries/LibraryModifier.java Show resolved Hide resolved

dkashyn-sfdc added 2 commits August 2, 2023 04:31

Merge remote-tracking branch 'origin/master' into non-default-source-…

47c7432

…roots-detection

Review comments

c50a400

Reverted excessive formatting changes and empty lines.

f7d0e7c

tpasternak approved these changes Aug 2, 2023

View reviewed changes

tpasternak merged commit 53cf068 into bazelbuild:master Aug 2, 2023
1 check passed

github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Aug 2, 2023

mai93 mentioned this pull request Aug 15, 2023

Update changelog for release v2023.08.15 #5235

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select source roots for library jars that have interim folders like `… #5117

Select source roots for library jars that have interim folders like `… #5117

dkashyn-sfdc commented Jul 17, 2023 •

edited

Loading

google-cla bot commented Jul 17, 2023

tpasternak commented Jul 19, 2023 •

edited

Loading

tpasternak commented Jul 19, 2023

tpasternak Jul 19, 2023

tpasternak Jul 19, 2023

dkashyn-sfdc Jul 19, 2023

tpasternak Jul 21, 2023

tpasternak left a comment

tpasternak Jul 21, 2023

dkashyn-sfdc commented Jul 30, 2023

dkashyn-sfdc commented Jul 30, 2023

tpasternak Aug 1, 2023

dkashyn-sfdc commented Aug 2, 2023

Select source roots for library jars that have interim folders like `… #5117

Select source roots for library jars that have interim folders like `… #5117

Conversation

dkashyn-sfdc commented Jul 17, 2023 • edited Loading

Checklist

Discussion thread for this change

Description of this change

google-cla bot commented Jul 17, 2023

tpasternak commented Jul 19, 2023 • edited Loading

tpasternak commented Jul 19, 2023

tpasternak Jul 19, 2023

Choose a reason for hiding this comment

tpasternak Jul 19, 2023

Choose a reason for hiding this comment

dkashyn-sfdc Jul 19, 2023

Choose a reason for hiding this comment

tpasternak Jul 21, 2023

Choose a reason for hiding this comment

tpasternak left a comment

Choose a reason for hiding this comment

tpasternak Jul 21, 2023

Choose a reason for hiding this comment

dkashyn-sfdc commented Jul 30, 2023

dkashyn-sfdc commented Jul 30, 2023

tpasternak Aug 1, 2023

Choose a reason for hiding this comment

dkashyn-sfdc commented Aug 2, 2023

dkashyn-sfdc commented Jul 17, 2023 •

edited

Loading

tpasternak commented Jul 19, 2023 •

edited

Loading