-
Notifications
You must be signed in to change notification settings - Fork 304
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
Added build folder action #6615
Merged
tpasternak
merged 17 commits into
bazelbuild:master
from
kitbuilder587:zajdelovm-feature-request-branch
Aug 16, 2024
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
90f0f5f
Added build folder action
kitbuilder587 4e79303
Merge branch 'master' into zajdelovm-feature-request-branch
kitbuilder587 c1cacd1
Update CompileCorrespondingBuildFilesAction.java
kitbuilder587 c4f6938
Merge branch 'zajdelovm-feature-request-branch' of https://github.com…
kitbuilder587 4bd7d29
Update CompileCorrespondingBuildFilesAction.java
kitbuilder587 f3e1697
button name change
kitbuilder587 8bab9b4
Update CompileCorrespondingBuildFilesAction.java
kitbuilder587 cf0bb22
Added build folder action
kitbuilder587 1ab1234
Update BlazeBuildService.java
kitbuilder587 86df84e
review remarks fix
kitbuilder587 9f87422
Update CompileCorrespondingBuildFilesAction.java
kitbuilder587 925f9c7
Merge branch 'zajdelovm-feature-request-branch' of https://github.com…
kitbuilder587 8562252
SERIAL_NOT_EXPAND deleted
kitbuilder587 a59c674
Update BlazeBuildTargetSharder.java
kitbuilder587 83d41eb
serial not expand
kitbuilder587 f6f9515
Merge branch 'zajdelovm-feature-request-branch' of https://github.com…
kitbuilder587 ec81777
revert
kitbuilder587 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
73 changes: 73 additions & 0 deletions
73
base/src/com/google/idea/blaze/base/actions/CompileFolderAction.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/* | ||
* Copyright 2024 The Bazel Authors. All rights reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package com.google.idea.blaze.base.actions; | ||
|
||
import com.google.idea.blaze.base.build.BlazeBuildService; | ||
import com.google.idea.blaze.base.model.primitives.*; | ||
import com.google.idea.blaze.base.lang.buildfile.search.BlazePackage; | ||
import com.google.idea.common.actions.ActionPresentationHelper; | ||
import com.intellij.openapi.actionSystem.*; | ||
import com.intellij.openapi.project.Project; | ||
import com.intellij.openapi.vfs.VirtualFile; | ||
|
||
import java.io.File; | ||
import java.util.List; | ||
|
||
/** Action to compile all build files in folder if folder picked or compile corresponding build file if file picked */ | ||
public class CompileFolderAction extends BlazeProjectAction { | ||
|
||
@Override | ||
protected void actionPerformedInBlazeProject(Project project, AnActionEvent e) { | ||
runBuild(project, e); | ||
} | ||
|
||
protected void runBuild(Project project, AnActionEvent e) { | ||
VirtualFile vf = e.getData(CommonDataKeys.VIRTUAL_FILE); | ||
WorkspaceRoot root = WorkspaceRoot.fromProject(project); | ||
WorkspacePath path = root.workspacePathForSafe(new File(vf.getPath())); | ||
try { | ||
List<TargetExpression> dt = List.of(TargetExpression.fromString("//" + path.toString() + "/...")); | ||
BlazeBuildService.getInstance(project).buildFolder(vf.getName(),dt); | ||
} catch (InvalidTargetException ex) { | ||
throw new RuntimeException(ex); | ||
} | ||
} | ||
|
||
@Override | ||
protected void updateForBlazeProject(Project project, AnActionEvent e) { | ||
Presentation presentation = e.getPresentation(); | ||
|
||
DataContext dataContext = e.getDataContext(); | ||
VirtualFile virtualFile = CommonDataKeys.VIRTUAL_FILE.getData(dataContext); | ||
BlazePackage blazePackage = BuildFileUtils.getBuildFile(project, virtualFile); | ||
|
||
boolean enabled = blazePackage != null; | ||
presentation.setVisible(enabled || !ActionPlaces.isPopupPlace(e.getPlace())); | ||
presentation.setEnabled(enabled); | ||
|
||
ActionPresentationHelper.of(e) | ||
.disableIf(virtualFile == null) | ||
.disableIf(!virtualFile.isDirectory()) | ||
.setText("Compile " + virtualFile.getName() + "/...:all") | ||
.hideInContextMenuIfDisabled() | ||
.commit(); | ||
} | ||
|
||
@Override | ||
protected QuerySyncStatus querySyncSupport() { | ||
return QuerySyncStatus.SUPPORTED; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this overrides the
shard_sync
setting in the PV, how about just keeping the old behavior here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue here is that compile folder with
shard_sync=true
takes sooo much more time. For example, one of our project's folder compiles in 17s withshard_sync=true
and in 1s withshard_sync=false
. The main reason why we added this button was that sync time was very slow, and we wanted to speed up compile errors check. Ordinary user won't setshard_sync
to false without any reason. We can add another setting to setshard_sync
for build folder action, but I think in cases when OOM happens, you can just use Partial Sync insteadThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's a good point. Btw what do you think about setting the default PV in
tools/intellij/.managed.bazelproject
withshard_sync: true
?.Example here https://github.com/bazelbuild/bazel/blob/master/tools/intellij/.managed.bazelproject
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think It's a good idea, but I don't have a lot of knowledge about how often people want to use
shard_sync=true
. In our project we use it quite rarely, but I can't expand my PV on all users of the plugin.Should I create another PR here or we can stay on current realization or may be another ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LeFrosch what do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, actually if I am not mistaken,
shard-sync=false
by default. We can delete this property in enumThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But anyway logically it would be better if shard-sync won't affect build. Often
shard-sync=true
is used for full sync of project if OOM happens.shard-sync=true
will affect the build folder speed. (17s and 1s is big diff) I don't think that it will be used for the whole project often. Withshard-sync=true
build folder action is useless, because the diff between build and sync time isn't big. I think SERIAL_NOT_EXPAND anyway will be betterThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but I can't guarantee it will be permanent. If it turns out to be misleading, we’ll adjust it to align with the shard_sync setting.