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

Allow to have non-existent import entries in projectview that pointing to … #5689

Merged
merged 8 commits into from
Dec 11, 2023
Merged

Allow to have non-existent import entries in projectview that pointing to … #5689

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a23d29a
Allow to have non-existent entries in `projectview` that pointing to …
dkashyn-sfdc Nov 14, 2023
54a1ff2
Reordering of conditions for readability
dkashyn-sfdc Nov 14, 2023
9215b8a
Merge branch 'master' into optional-imports-projectview
dkashyn-sfdc Nov 27, 2023
1fabf5d
Merge remote-tracking branch 'origin/master' into optional-imports-pr…
dkashyn-sfdc Nov 29, 2023
ae15030
TryImport POC
dkashyn-sfdc Nov 29, 2023
841a9aa
`try_import` instead of `import`
dkashyn-sfdc Nov 29, 2023
f1377b5
Some tests for try_import
dkashyn-sfdc Nov 29, 2023
b174bc6
test fix
dkashyn-sfdc Nov 29, 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
1 change: 1 addition & 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 @@ -360,6 +360,7 @@
<notificationGroup displayType="STICKY_BALLOON" id="QuerySyncPromo" />
<notificationGroup displayType="BALLOON" id="QuerySyncBuild" />
<registryKey defaultValue="false" description="Disable auto import of bazel projects" key="bazel.auto.import.disabled"/>
<registryKey defaultValue="false" description="Allow to have optional imports in project view" key="bazel.projectview.optional.imports"/>
<registryKey
key="bazel.sync.resolve.virtual.includes"
description="Apply heuristics to detect correct location of headers which are accessed from _virtual_includes during build"
Expand Down
8 changes: 5 additions & 3 deletions base/src/com/google/idea/blaze/base/projectview/ProjectViewSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,12 @@ public <T> List<T> listItems(SectionKey<T, ListSection<T>> key) {
}

/** Returns all values from all scalar sections in the project views, in order */
public <T> List<T> listScalarItems(SectionKey<T, ScalarSection<T>> key) {
public <T> List<T> listScalarItems(SectionKey<T, ScalarSection<T>>... keys) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure whether this is a good idea so can redo if someone have strong opinions against this way

List<T> result = Lists.newArrayList();
for (ScalarSection<T> section : getSections(key)) {
result.add(section.getValue());
for (SectionKey<T, ScalarSection<T>> key: keys) {
for (ScalarSection<T> section : getSections(key)) {
result.add(section.getValue());
}
}
return result;
}
Expand Down
25 changes: 22 additions & 3 deletions base/src/com/google/idea/blaze/base/projectview/section/sections/ImportSection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
import com.google.idea.blaze.base.projectview.section.ScalarSectionParser;
import com.google.idea.blaze.base.projectview.section.SectionKey;
import com.google.idea.blaze.base.projectview.section.SectionParser;
import com.google.idea.blaze.base.scope.output.IssueOutput;
import com.intellij.openapi.util.registry.Registry;

import java.io.File;
import javax.annotation.Nullable;

Expand All @@ -31,15 +34,25 @@ public class ImportSection {
SectionKey.of("import");
public static final SectionParser PARSER = new ImportSectionParser();

private static class ImportSectionParser extends ScalarSectionParser<WorkspacePath> {
protected static class ImportSectionParser extends ScalarSectionParser<WorkspacePath> {
public ImportSectionParser() {
super(KEY, ' ');
this(KEY, ' ');
}

protected ImportSectionParser(SectionKey<WorkspacePath, ScalarSection<WorkspacePath>> key, char divider) {
super(key, divider);
}

@Nullable
@Override
protected WorkspacePath parseItem(
ProjectViewParser parser, ParseContext parseContext, String text) {
boolean projectViewImportsMandatory = !Registry.is("bazel.projectview.optional.imports");
return parseItem(parser, parseContext, text, projectViewImportsMandatory);
}

@Nullable
protected static WorkspacePath parseItem(ProjectViewParser parser, ParseContext parseContext, String text, boolean projectViewImportsMandatory) {
String error = WorkspacePath.validate(text);
if (error != null) {
parseContext.addError(error);
Expand All @@ -50,7 +63,13 @@ protected WorkspacePath parseItem(
if (parser.isRecursive()) {
File projectViewFile = parseContext.getWorkspacePathResolver().resolveToFile(workspacePath);
if (projectViewFile != null) {
parser.parseProjectView(projectViewFile);
if (projectViewImportsMandatory || projectViewFile.exists()) {
parser.parseProjectView(projectViewFile);
} else {
IssueOutput.warn(
String.format("Could not load project view file: '%s'", projectViewFile.getPath()))
.submit(parseContext.getContext());
}
} else {
parseContext.addError("Could not resolve import: " + workspacePath);
}
Expand Down
1 change: 1 addition & 0 deletions base/src/com/google/idea/blaze/base/projectview/section/sections/Sections.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public class Sections {
Lists.newArrayList(
TextBlockSection.PARSER,
ImportSection.PARSER,
TryImportSection.PARSER,
DirectorySection.PARSER,
AutomaticallyDeriveTargetsSection.PARSER,
SyncManualTargetsSection.PARSER,
Expand Down
32 changes: 32 additions & 0 deletions base/src/com/google/idea/blaze/base/projectview/section/sections/TryImportSection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package com.google.idea.blaze.base.projectview.section.sections;

import com.google.idea.blaze.base.model.primitives.WorkspacePath;
import com.google.idea.blaze.base.projectview.parser.ParseContext;
import com.google.idea.blaze.base.projectview.parser.ProjectViewParser;
import com.google.idea.blaze.base.projectview.section.ScalarSection;
import com.google.idea.blaze.base.projectview.section.SectionKey;
import com.google.idea.blaze.base.projectview.section.SectionParser;

import javax.annotation.Nullable;

public class TryImportSection extends ImportSection {

public static final SectionKey<WorkspacePath, ScalarSection<WorkspacePath>> KEY =
SectionKey.of("try-import");

public static final SectionParser PARSER = new TryImportSectionParser();

private static class TryImportSectionParser extends ImportSectionParser {

public TryImportSectionParser() {
super(KEY, ' ');
}

@Nullable
@Override
protected WorkspacePath parseItem(
ProjectViewParser parser, ParseContext parseContext, String text) {
return parseItem(parser, parseContext, text, false);
}
}
}
3 changes: 2 additions & 1 deletion base/src/com/google/idea/blaze/base/qsync/ProjectStatsLogger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.idea.blaze.base.model.primitives.WorkspacePath;
import com.google.idea.blaze.base.projectview.ProjectViewSet;
import com.google.idea.blaze.base.projectview.section.sections.ImportSection;
import com.google.idea.blaze.base.projectview.section.sections.TryImportSection;
import com.google.idea.blaze.common.Context;
import com.google.idea.blaze.qsync.BlazeProjectListener;
import com.google.idea.blaze.qsync.project.BlazeProjectSnapshot;
Expand Down Expand Up @@ -55,7 +56,7 @@ public void onNewProjectSnapshot(Context<?> context, BlazeProjectSnapshot instan
.getProjectInfoStatsBuilder()
.setLanguagesActive(instance.queryData().projectDefinition().languageClasses())
.setBlazeProjectFiles(
projectViewSet.listScalarItems(ImportSection.KEY).stream()
projectViewSet.listScalarItems(ImportSection.KEY, TryImportSection.KEY).stream()
.map(WorkspacePath::asPath)
.collect(toImmutableSet()));
scope
Expand Down
3 changes: 2 additions & 1 deletion base/src/com/google/idea/blaze/base/sync/SyncPhaseCoordinator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import com.google.idea.blaze.base.projectview.ProjectViewManager;
import com.google.idea.blaze.base.projectview.ProjectViewSet;
import com.google.idea.blaze.base.projectview.section.sections.ImportSection;
import com.google.idea.blaze.base.projectview.section.sections.TryImportSection;
import com.google.idea.blaze.base.scope.BlazeContext;
import com.google.idea.blaze.base.scope.Scope;
import com.google.idea.blaze.base.scope.output.IssueOutput;
Expand Down Expand Up @@ -722,7 +723,7 @@ private static void fillInBuildStats(
.setWorkspaceType(projectState.getLanguageSettings().getWorkspaceType())
.setLanguagesActive(projectState.getLanguageSettings().getActiveLanguages())
.setBlazeProjectFiles(
projectState.getProjectViewSet().listScalarItems(ImportSection.KEY));
projectState.getProjectViewSet().listScalarItems(ImportSection.KEY, TryImportSection.KEY));
}
if (buildResult != null) {
buildResult
Expand Down
4 changes: 4 additions & 0 deletions base/src/com/google/idea/blaze/base/wizard2/ui/BlazeEditProjectViewControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import com.google.idea.blaze.base.projectview.section.sections.ImportSection;
import com.google.idea.blaze.base.projectview.section.sections.Sections;
import com.google.idea.blaze.base.projectview.section.sections.TargetSection;
import com.google.idea.blaze.base.projectview.section.sections.TryImportSection;
import com.google.idea.blaze.base.scope.BlazeContext;
import com.google.idea.blaze.base.scope.OutputSink.Propagation;
import com.google.idea.blaze.base.scope.Scope;
Expand Down Expand Up @@ -599,6 +600,9 @@ public void updateBuilder(BlazeNewProjectBuilder builder) {
.add(
ScalarSection.builder(ImportSection.KEY)
.set(selectProjectViewOption.getSharedProjectView()))
.add(
Copy link
Collaborator

@mai93 mai93 Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

why is this needed? It results in having both import and try_import sections in the generated local projectview file even with the flag disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is ok to use both import and try_import. It might be not ok to have some duplication or excessive defaults...

Could you please share more context for your inquiry?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not know why it is needed to add a try_import in this function? If I understand correctly, this function creates a project view that imports the shared project view file selected in the import process. So import section should be enough and there is no need to try_import the same project view.

Copy link
Contributor Author

@dkashyn-sfdc dkashyn-sfdc Dec 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@mai93 there might be "mandatory" and "optional" imports in any project view. One example of optional would be

try_import:
  ~/.some.local.projectview

(let's assume ~ is resolved to user home dir ;) )

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, I'm still not sure what the effect of having something like:

import hello/hello.bazelproject
try_import hello/hello.bazelproject

will this provide a different result from only

import hello/hello.bazelproject

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is rather for

import hello/fail.if.not.present.bazelproject
try_import hello/ok.if.not.present.bazelproject

That can result in:

  1. both imported
  2. only hello/fail.if.not.present.bazelproject imported and accepting the fact that that try_import hello/ok.if.not.present.bazelproject is not there
  3. failing if hello/fail.if.not.present.bazelproject is missing

Optional imports will allow engineers to have pre-defined custom import files both present or missing.

Now it is possible for some of the users to benefit from custom contents while other users not affected even if they have no local customizations. For both groups of users the same .bazelproject can be stored in VCS now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

so there is no additional value for having:

import hello/hello.bazelproject
try_import hello/hello.bazelproject

right? If so, then the change in this line can be reverted because it adds try_import section for the same project view file: selectProjectViewOption.getSharedProjectView() that already was added with import in line 600.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverting here #5958 @mai93

ScalarSection.builder(TryImportSection.KEY)
.set(selectProjectViewOption.getSharedProjectView()))
.build();
projectViewSet =
ProjectViewSet.builder()
Expand Down
2 changes: 2 additions & 0 deletions base/tests/unittests/com/google/idea/blaze/base/projectview/ProjectViewSetTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import com.google.idea.blaze.base.projectview.section.sections.TestSourceSection;
import com.google.idea.blaze.base.projectview.section.sections.TextBlock;
import com.google.idea.blaze.base.projectview.section.sections.TextBlockSection;
import com.google.idea.blaze.base.projectview.section.sections.TryImportSection;
import com.google.idea.blaze.base.projectview.section.sections.WorkspaceTypeSection;
import com.google.idea.blaze.base.sync.BlazeSyncPlugin;
import com.google.idea.common.experiments.ExperimentService;
Expand Down Expand Up @@ -85,6 +86,7 @@ public void testProjectViewSetSerializable() {
ListSection.builder(TargetSection.KEY)
.add(TargetExpression.fromStringSafe("//test:all")))
.add(ScalarSection.builder(ImportSection.KEY).set(new WorkspacePath("test")))
.add(ScalarSection.builder(TryImportSection.KEY).set(new WorkspacePath("test-optional")))
.add(ListSection.builder(TestSourceSection.KEY).add(new Glob("javatests/*")))
.add(ListSection.builder(ExcludedSourceSection.KEY).add(new Glob("*.java")))
.add(ListSection.builder(BuildFlagsSection.KEY).add("--android_sdk=abcd"))
Expand Down
25 changes: 24 additions & 1 deletion .../tests/unittests/com/google/idea/blaze/base/projectview/parser/ProjectViewParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import com.google.idea.blaze.base.projectview.section.sections.TestSourceSection;
import com.google.idea.blaze.base.projectview.section.sections.TextBlock;
import com.google.idea.blaze.base.projectview.section.sections.TextBlockSection;
import com.google.idea.blaze.base.projectview.section.sections.TryImportSection;
import com.google.idea.blaze.base.projectview.section.sections.WorkspaceTypeSection;
import com.google.idea.blaze.base.scope.BlazeContext;
import com.google.idea.blaze.base.scope.ErrorCollector;
Expand All @@ -51,6 +52,9 @@
import java.util.Collection;
import java.util.Map;
import javax.annotation.Nullable;

import com.intellij.openapi.util.registry.Registry;
import com.intellij.openapi.util.registry.RegistryValue;
import org.jetbrains.annotations.NotNull;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -190,6 +194,9 @@ public void testPrint() {
.add(
ScalarSection.builder(ImportSection.KEY)
.set(new WorkspacePath("some/file.blazeproject")))
.add(
ScalarSection.builder(TryImportSection.KEY)
.set(new WorkspacePath("some/file.blazeproject.optional")))
.build();
String text = ProjectViewParser.projectViewToString(projectView);
assertThat(text)
Expand All @@ -202,7 +209,8 @@ public void testPrint() {
"targets:",
" //java/com/google:one",
" //java/com/google:two",
"import some/file.blazeproject"));
"import some/file.blazeproject",
"try-import some/file.blazeproject.optional"));
}

@Test
Expand Down Expand Up @@ -303,11 +311,26 @@ public void testCanParseWithMissingCarriageReturnAtEndOfSection() {

@Test
public void testImportMissingFileResultsInIssue() {
Registry.get("bazel.projectview.optional.imports").setValue(false);
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 was unable to make it work with Registry.is('bla', false) for tests.

projectViewStorageManager.add(".blazeproject", "import parent.blazeproject");
projectViewParser.parseProjectView(new File(".blazeproject"));
errorCollector.assertIssues("Could not load project view file: '/parent.blazeproject'");
}

@Test
public void testImportMissingFileResultsInWarning() {
RegistryValue importsTreatmentRegistryValue = Registry.get("bazel.projectview.optional.imports");
try {
importsTreatmentRegistryValue.setValue(true);
projectViewStorageManager.add(".blazeproject", "import parent.blazeproject");
projectViewParser.parseProjectView(new File(".blazeproject"));
errorCollector.assertIssues("Could not load project view file: '/parent.blazeproject'");
errorCollector.assertHasNoErrors();
} finally {
importsTreatmentRegistryValue.setValue(false);
}
}

@Test
public void testMissingSectionResultsInIssue() {
projectViewStorageManager.add(".blazeproject", "nosuchsection:", " java/com/google");
Expand Down
4 changes: 4 additions & 0 deletions base/tests/utils/unit/com/google/idea/blaze/base/scope/ErrorCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ public void assertHasErrors() {
assertThat(issues.stream().anyMatch(i -> i.getCategory() == Category.ERROR)).isTrue();
}

public void assertHasNoErrors() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in some cases there are issues but those are not errors.

assertThat(issues.stream().anyMatch(i -> i.getCategory() == Category.ERROR)).isFalse();
}

public void assertIssues(String... requiredMessages) {
List<String> messages = Lists.newArrayList();
for (IssueOutput issue : issues) {
Expand Down