Skip to content

Commit

Permalink
addressed review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
LeFrosch committed Oct 18, 2024
1 parent 2611aef commit d8c2ad4
Show file tree
Hide file tree
Showing 13 changed files with 70 additions and 106 deletions.
3 changes: 0 additions & 3 deletions base/src/META-INF/blaze-base.xml
Original file line number Diff line number Diff line change
Expand Up @@ -408,9 +408,6 @@
<registryKey defaultValue="true"
description="Causes the plugin to read external workspace data, enabling features such as code completion for external targets. This may incur the cost of an additional 'bazel mod' call, which could cause issues with older versions or setups."
key="bazel.read.external.workspace.data"/>
<registryKey defaultValue="true"
description="Enables the new sync view"
key="bazel.new.sync.view"/>
<editorNotificationProvider implementation="com.google.idea.blaze.base.wizard2.BazelNotificationProvider"/>
</extensions>

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@
import com.google.common.util.concurrent.MoreExecutors;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.intellij.openapi.application.ApplicationManager;
import com.intellij.openapi.application.ModalityState;
import com.intellij.openapi.progress.PerformInBackgroundOption;
import com.intellij.openapi.progress.ProgressManager;
import com.intellij.openapi.progress.Progressive;
import com.intellij.openapi.progress.EmptyProgressIndicator;
import com.intellij.openapi.progress.impl.BackgroundableProcessIndicator;
import com.intellij.openapi.progress.util.AbstractProgressIndicatorExBase;
import com.intellij.openapi.progress.util.ProgressWindow;
Expand Down Expand Up @@ -106,7 +108,7 @@ public void submitTaskLater(Progressive progressive) {
*/
public <T> ListenableFuture<T> submitTaskWithResult(ProgressiveWithResult<T> progressive) {
if (modality == Modality.BUILD_VIEW) {
return executor.submit(() -> progressive.compute(ProgressIndicatorStub.INSTANCE));
return executor.submit(() -> progressive.compute(new EmptyProgressIndicator(ModalityState.NON_MODAL)));
}

// The progress indicator must be created on the UI thread.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ import java.io.BufferedInputStream
import java.io.FileInputStream
import kotlin.io.path.pathString

private val LOG: Logger = Logger.getInstance(BazelService::class.java)
private val LOG: Logger = Logger.getInstance(BazelExecService::class.java)

@Service(Service.Level.PROJECT)
class BazelService(private val project: Project) : Disposable {
class BazelExecService(private val project: Project) : Disposable {
companion object {
@JvmStatic
fun instance(project: Project): BazelService = project.service()
fun instance(project: Project): BazelExecService = project.service()
}

// #api223 use the injected scope
Expand Down Expand Up @@ -67,27 +67,34 @@ class BazelService(private val project: Project) : Disposable {
private suspend fun execute(ctx: BlazeContext, cmd: BlazeCommand): Int {
val root = cmd.effectiveWorkspaceRoot.orElseGet { WorkspaceRoot.fromProject(project).path() }

val handler = GeneralCommandLine()
val cmdLine = GeneralCommandLine()
.withExePath(cmd.binaryPath)
.withParameters(cmd.toArgumentList())
.apply { setWorkDirectory(root.pathString) } // required for backwards compatability
.withRedirectErrorStream(true)
.let(::OSProcessHandler)

handler.addProcessListener(object : ProcessListener {
override fun onTextAvailable(event: ProcessEvent, outputType: Key<*>) {
ctx.println(event.text.trimEnd())
var handler: OSProcessHandler? = null
val exitCode = try {
handler = withContext(Dispatchers.IO) {
OSProcessHandler(cmdLine)
}
})
handler.startNotify()

try {
while (!handler.isProcessTerminated) delay(100)
handler.addProcessListener(object : ProcessListener {
override fun onTextAvailable(event: ProcessEvent, outputType: Key<*>) {
ctx.println(event.text.trimEnd())
}
})
handler.startNotify()

while (!handler.isProcessTerminated) {
delay(100)
}

handler.exitCode ?: 1
} finally {
handler.destroyProcess()
handler?.destroyProcess()
}

val exitCode = handler.exitCode ?: 1
if (exitCode != 0) {
ctx.setHasError()
}
Expand All @@ -97,12 +104,17 @@ class BazelService(private val project: Project) : Disposable {

private suspend fun parseEvent(ctx: BlazeContext, stream: BufferedInputStream) {
// make sure that there are at least four bytes already available
while (stream.available() < 4) delay(10)
while (stream.available() < 4) {
delay(10)
}

// protobuf messages are delimited by size (encoded as varint32),
// read size manually to ensure the entire message is already available
val size = CodedInputStream.readRawVarint32(stream.read(), stream)
while (stream.available() < size) delay(10)

while (stream.available() < size) {
delay(10)
}

val eventStream = LimitedInputStream(stream, size)
val event = try {
Expand All @@ -118,18 +130,16 @@ class BazelService(private val project: Project) : Disposable {
return
}

if (event == null) {
delay(10)
} else {
BuildEventParser.parse(event)?.let(ctx::output)
}
BuildEventParser.parse(event)?.let(ctx::output)
}

private fun CoroutineScope.parseEvents(ctx: BlazeContext, helper: BuildResultHelper): Job {
return launch(CoroutineName("EventParser")) {
return launch(Dispatchers.IO + CoroutineName("EventParser")) {
try {
// wait for bazel to create the output file
while (!helper.outputFile.exists()) delay(10)
while (!helper.outputFile.exists()) {
delay(10)
}

FileInputStream(helper.outputFile).buffered().use { stream ->
// keep reading events while the coroutine is active, i.e. bazel is still running,
Expand All @@ -138,11 +148,9 @@ class BazelService(private val project: Project) : Disposable {
parseEvent(ctx, stream)
}
}
}
catch (e: CancellationException) {
} catch (e: CancellationException) {
throw e
}
catch (e: Exception) {
} catch (e: Exception) {
LOG.error("error in event parser", e)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ package com.google.idea.blaze.base.buildview

import com.google.idea.blaze.base.async.executor.ProgressiveTaskWithProgressIndicator
import com.google.idea.blaze.base.scope.BlazeContext
import com.intellij.openapi.util.registry.Registry
import com.google.idea.blaze.base.settings.BlazeUserSettings

object BuildViewMigration {
@JvmStatic
val enabled get(): Boolean = Registry.`is`("bazel.new.sync.view")
val enabled
get(): Boolean = BlazeUserSettings.getInstance().useNewSyncView

@JvmStatic
fun present(ctx: BlazeContext): Boolean {
Expand Down
3 changes: 1 addition & 2 deletions base/src/com/google/idea/blaze/base/buildview/ContextExt.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import com.google.idea.blaze.common.PrintOutput
import com.intellij.openapi.progress.runBlockingMaybeCancellable
import kotlinx.coroutines.CoroutineName
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.async

fun BlazeContext.println(msg: String) {
Expand All @@ -17,7 +16,7 @@ fun <T> BlazeContext.pushJob(
name: String = "BazelContext",
block: suspend CoroutineScope.() -> T,
): T {
val deferred = scope.async(Dispatchers.IO + CoroutineName(name)) { block() }
val deferred = scope.async(CoroutineName(name)) { block() }

addCancellationHandler { deferred.cancel() }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ package com.google.idea.blaze.base.buildview.events

import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEvent
import com.google.idea.blaze.base.scope.output.IssueOutput
import com.intellij.openapi.diagnostic.Logger
import com.intellij.openapi.diagnostic.logger
import com.intellij.openapi.extensions.ExtensionPointName

internal val LOG = Logger.getInstance(BuildEventParser::class.java)
internal val LOG = logger<BuildEventParser>()

interface BuildEventParser {
companion object {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ public String toString() {
private boolean expandSyncToWorkingSet = true;
private boolean showPerformanceWarnings = false;
private boolean collapseProjectView = true;
private boolean useNewSyncView = false;

private boolean javascriptTestrunnersEnabled = false;

Expand Down Expand Up @@ -207,6 +208,14 @@ public void setCollapseProjectView(boolean collapseProjectView) {
this.collapseProjectView = collapseProjectView;
}

public boolean getUseNewSyncView() {
return useNewSyncView;
}

public void setUseNewSyncView(boolean useNewSyncView) {
this.useNewSyncView = useNewSyncView;
}

public boolean isJavascriptTestrunnersEnabled() {
return javascriptTestrunnersEnabled;
}
Expand Down Expand Up @@ -269,6 +278,7 @@ public ImmutableMap<String, String> getApplicationSettings() {
builder.put("blazeBinaryPath", settings.blazeBinaryPath);
builder.put("bazelBinaryPath", settings.bazelBinaryPath);
builder.put("buildifierBinaryPath", settings.buildifierBinaryPath);
builder.put("useNewSyncView", Boolean.toString(settings.useNewSyncView));
return builder.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,12 @@ public ImmutableCollection<SearchableText> getSearchableText() {
.setter(BlazeUserSettings::setCollapseProjectView)
.componentFactory(SimpleComponent::createCheckBox);

private static final ConfigurableSetting<?, ?> USE_NEW_SYNC_VIEW =
setting("Use the new sync view")
.getter(BlazeUserSettings::getUseNewSyncView)
.setter(BlazeUserSettings::setUseNewSyncView)
.componentFactory(SimpleComponent::createCheckBox);

private static final ConfigurableSetting<?, ?> ALWAYS_SELECT_NEWEST_CHILD_TASK =
setting("Always select the newest child task in Blaze view")
.getter(BlazeUserSettings::getSelectNewestChildTask)
Expand Down Expand Up @@ -170,6 +176,7 @@ public ImmutableCollection<SearchableText> getSearchableText() {
SHOW_PROBLEMS_VIEW_ON_RUN,
ALLOW_JAVASCRIPT_TESTS,
COLLAPSE_PROJECT_VIEW,
USE_NEW_SYNC_VIEW,
FORMAT_BUILD_FILES_ON_SAVE,
SHOW_ADD_FILE_TO_PROJECT,
ALWAYS_SELECT_NEWEST_CHILD_TASK,
Expand Down Expand Up @@ -199,6 +206,7 @@ public JComponent createComponent() {
return SwingHelper.newLeftAlignedVerticalPanel(
getFocusBehaviorSettingsUi(),
createVerticalPanel(
USE_NEW_SYNC_VIEW,
COLLAPSE_PROJECT_VIEW,
ALLOW_JAVASCRIPT_TESTS,
FORMAT_BUILD_FILES_ON_SAVE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@
import com.intellij.openapi.project.DumbService;
import com.intellij.openapi.project.Project;
import com.intellij.openapi.startup.StartupManager;
import com.intellij.openapi.util.registry.Registry;
import com.intellij.openapi.util.text.StringUtil;

import java.util.Collection;
Expand Down Expand Up @@ -123,7 +122,7 @@ public void requestProjectSync(BlazeSyncParams syncParams) {
indicator ->
Scope.root(
context -> {
if (Registry.is("bazel.new.sync.view")) {
if (BuildViewMigration.getEnabled()) {
context.push(new BuildViewScope(project,
getRootInvocationTitle(syncParams)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.google.idea.blaze.base.sync;

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.idea.blaze.base.issueparser.BlazeIssueParser.targetDetectionQueryParsers;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -398,6 +399,7 @@ private ImmutableList<TargetExpression> findTargetsBuildingSourceFiles(
var newScope = new ToolWindowScope.Builder(project, task)
.setProgressIndicator(indicator)
.setPopupBehavior(BlazeUserSettings.FocusBehavior.ON_ERROR)
.setIssueParsers(targetDetectionQueryParsers(project, WorkspaceRoot.fromProject(project)))
.build();
childContext.push(newScope);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,12 @@
import com.google.idea.blaze.base.async.FutureUtil;
import com.google.idea.blaze.base.async.executor.BlazeExecutor;
import com.google.idea.blaze.base.bazel.BuildSystem.BuildInvoker;
import com.google.idea.blaze.base.buildview.BazelService;
import com.google.idea.blaze.base.buildview.BazelExecService;
import com.google.idea.blaze.base.command.BlazeCommand;
import com.google.idea.blaze.base.command.BlazeCommandName;
import com.google.idea.blaze.base.command.BlazeFlags;
import com.google.idea.blaze.base.command.BlazeInvocationContext;
import com.google.idea.blaze.base.command.BlazeInvocationContext.ContextType;
import com.google.idea.blaze.base.command.buildresult.BuildResultHelper;
import com.google.idea.blaze.base.command.buildresult.LocalFileArtifact;
import com.google.idea.blaze.base.command.buildresult.RemoteOutputArtifact;
import com.google.idea.blaze.base.command.info.BlazeConfigurationHandler;
Expand Down Expand Up @@ -772,7 +771,7 @@ private static BlazeBuildOutputs runBuildForTargets(
aspectStrategy.addAspectAndOutputGroups(
builder, outputGroups, activeLanguages, onlyDirectDeps);

return BazelService.instance(project).build(context, builder);
return BazelExecService.instance(project).build(context, builder);
} finally {
if (!Registry.is("bazel.sync.keep.target.files")) {
try {
Expand Down
Loading

0 comments on commit d8c2ad4

Please sign in to comment.