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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from
Aug 2, 2023
+78 −9
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5fbd5cf
Select source roots for library jars that have interim folders like `…
dkashyn-sfdc Jul 17, 2023
563474f
Add registry value to conditionally import all of the source dirs fro…
dkashyn-sfdc Jul 18, 2023
75b79a2
Registry key definition
dkashyn-sfdc Jul 18, 2023
c101401
Merge branch 'master' into non-default-source-roots-detection
dkashyn-sfdc Jul 18, 2023
d011260
Merge remote-tracking branch 'origin/master' into non-default-source-…
dkashyn-sfdc Jul 26, 2023
587d7e2
Merge branch 'master' into non-default-source-roots-detection
dkashyn-sfdc Jul 28, 2023
ca43fce
Merge branch 'master' into non-default-source-roots-detection
dkashyn-sfdc Jul 29, 2023
af4d35b
Addressed PR comments:
dkashyn-sfdc Jul 30, 2023
f2eb19a
Cleanup unused imports
dkashyn-sfdc Jul 30, 2023
fa4684e
Created default impls to not upset existing LibrarySourceProviders ou…
dkashyn-sfdc Jul 30, 2023
b846629
Proper message for progress task
dkashyn-sfdc Jul 30, 2023
47c7432
Merge remote-tracking branch 'origin/master' into non-default-source-…
dkashyn-sfdc Aug 2, 2023
c50a400
Review comments
dkashyn-sfdc Aug 2, 2023
f7d0e7c
Reverted excessive formatting changes and empty lines.
dkashyn-sfdc Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions base/src/META-INF/blaze-base.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,9 @@
<registryKey defaultValue="true"
description="Enables opening the project view file the first time the project is imported"
key="bazel.project.import.open_project_view"/>
<registryKey defaultValue="false"
description="Scan for non-root source folders in source jars."
dkashyn-sfdc marked this conversation as resolved.
Show resolved Hide resolved
key="bazel.sync.detect.source.roots"/>
</extensions>

<extensions defaultExtensionNs="com.intellij">
Expand Down
21 changes: 20 additions & 1 deletion base/src/com/google/idea/blaze/base/sync/libraries/LibraryModifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,21 @@
import com.google.idea.blaze.base.model.LibraryFilesProvider;
import com.intellij.openapi.diagnostic.Logger;
import com.intellij.openapi.externalSystem.service.project.IdeModifiableModelsProvider;
import com.intellij.openapi.progress.EmptyProgressIndicator;
import com.intellij.openapi.roots.OrderRootType;
import com.intellij.openapi.roots.libraries.Library;
import com.intellij.openapi.roots.libraries.Library.ModifiableModel;
import com.intellij.openapi.roots.ui.configuration.JavaVfsSourceRootDetectionUtil;
import com.intellij.openapi.util.io.FileUtil;
import com.intellij.openapi.util.io.FileUtilRt;
import com.intellij.openapi.util.registry.Registry;
import com.intellij.openapi.vfs.StandardFileSystems;
import com.intellij.openapi.vfs.VirtualFile;
import com.intellij.openapi.vfs.VirtualFileManager;
import com.intellij.util.io.URLUtil;
import java.io.File;
import java.util.Collections;
import java.util.List;

/** Modifies {@link Library} content in {@link Library.ModifiableModel}. */
public class LibraryModifier {
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
if (!candidates.isEmpty()) {
candidates.forEach(candidate -> modifiableModel.addRoot(candidate, orderRootType));
} else {
modifiableModel.addRoot(pathToUrl(file), orderRootType);
}
} else {
modifiableModel.addRoot(pathToUrl(file), orderRootType);
}
}

private String pathToUrl(File path) {
Expand Down