Calling build targets from CMake Project Outline always builds default target if useTasks option is set #2780

piomis · 2022-10-07T18:26:29Z

This change fixes #2778

The following changes are proposed:

updated minimum vscode package version to 1.60.0
- fixed CleanRebuild command - Build phase hasn't used task even if buildTask setting was set to true
Clean Rebuild - build phase should use proper build task if buildTasks option is set
When build is requested following action is taken to select build task: if only one build task is present than it is used, if more tasks than one with default flag (if only one present), else user is asked to select build task which should be used from QuickPickMenu

piomis · 2022-10-07T18:27:30Z

@elahehrashedi here is my proposal. Please take a look if it makes sense. I haven't squashed commits to clearly present how this solution evolved :)

src/drivers/cmakeDriver.ts

…t insted of default target

- proc.ExectionOptions.useBuildTask is not needed anymore (driver is now detecting if task should be used - rename taskFinder function and remove its parameter

package.json

elahehrashedi · 2022-10-11T15:04:13Z

src/drivers/cmakeDriver.ts

@@ -1696,6 +1714,34 @@ export abstract class CMakeDriver implements vscode.Disposable {
        }
    }

+    private async _findBuildTask(): Promise<vscode.Task | undefined> {


It looks like that _findBuildTask is an implementation of vscode.commands.executeCommand("workbench.action.tasks.build"); , it fetches all of the tasks, choose the default is existed, otherwise shows a quickpick to choose from.
The only difference is that vscode.commands.executeCommand("workbench.action.tasks.build") will show the template tasks besides the tasks defined in tasks.json.

I think that I also list only tasks which are 'cmake' tasks. I supose workbench.action.tasks.build will find also others.

That's right. However, it is still not delivering the results we want. Please read the later comments.

src/cmakeTaskProvider.ts

elahehrashedi · 2022-10-11T15:19:07Z

src/cmakeTaskProvider.ts

        const cmakeDriver: CMakeDriver | undefined = (await cmakeProject?.getCMakeDriverInstance()) || undefined;
        let cmakePath: string;
+        if(cmakeDriver)


This implementation doesn't seem to be a good practice. The task with a specific target is created, but the code is altering the targets during the execution time, without any warning. If the targets needs to be changed, it should be before the task is being executed.

Besides that, I don't think it is good idea to choose a defined task and then alter the targets. The _findBuildTask could work like this:

fetch all of the tasks in tasks.json

find the task that has the targets matching the targets chosen from the project outline

if there is no such task, show a quickpick of the template tasks to choose from.
- alter the targets if the template task to match the project outline target.

The question is what do you call 'template' task. I supose you mean these which are added by default by cmake Task Provider.

In my case I add task to tasks.json which does not have targets defined as it is task dedicated for all executable targets ( I add custom problem matcher I also seen cases when one was using tasks to get output in terminal which na n contrary to Output log support colours). I supose in your definitione it is not template task.

Thus first condition to find tasks will never be met, second either as my taask is in tasks.json so wont appear on Quick pick list.

I agree that probably I could also eliminate in initial fetch all tasks which have target defined and it does not match the one requested.

PS. Please also note that target list is altered only when task is truggered as a result of 'BuildCommand'. When triggered by RunTask it will not get altered as target list in driver wont be set.

The question is what do you call 'template' task. I supose you mean these which are added by default by cmake Task Provider.

correct.

In my case I add task to tasks.json which does not have targets defined as it is task dedicated for all executable targets ( I add custom problem matcher I also seen cases when one was using tasks to get output in terminal which na n contrary to Output log support colours). I supose in your definitione it is not template task.

The user need to know what are the specifications of the task that is running. So it needs to be a custom task they defined in the tasks.json or the template task we provide.

Thus first condition to find tasks will never be met, second either as my taask is in tasks.json so wont appear on Quick pick list.

Would you please elaborate on this?

I agree that probably I could also eliminate in initial fetch all tasks which have target defined and it does not match the one requested.

Correct, quickpick should show the tasks with the same target, or template task for that target. And importantly, don't alter a selected task when running it.

In template tasks, we only change the target, other characteristics stay the same.

@elahehrashedi I reimplemented approach a little bit.

I list all cmake tasks.
Then I search for ones which target matches to requested one. If more than one found - I search for one with default flag. Else I let user to pick from list.
If no tasks with matching targets, then I do similar query among ones with no targets defined.

In task processing I use same "correctTargets" method which was already present. So all already implemented guards are in power. I added only a capability to query driver for "requested target".

By the way - I noticed that method runTestsTask and runConfigTasks also calls correctTargets but they ignore returned value. Does it make then sense to call them?

…eview) Task search algo is implemented: - among all cmake build tasks find ones with target matching requested target - if only one found use it, if more then try to find one with default flag, else show quick picker - if no target dedicated tasks find filter among one with no targets configured (same approach) -if above actions haven't provided tasks show all cmake tasks to user to pick one

…th_build_task

bobbrow · 2022-10-28T19:07:31Z

I just published #2828 which should address the build from outline issues. It goes a little deeper than this PR does on that. If you're ok with that approach, we could repurpose this PR to cover the new feature being added to select the build task.

Please also run yarn lint on your branch. There are a number of errors that will need to be addressed.

bobbrow · 2022-12-02T17:42:34Z

src/cmakeBuildRunner.ts

+
+export class CMakeBuildRunner {
+
+    private taskExecutor: vscode.TaskExecution | null = null;


Please use undefined instead of null (applies to the whole file). We will be trying to switch all null usages to undefined in the future. #Closed

Can do.
@elahehrashedi please let me know if I should do it or you prefer to take over.

I think @elahehrashedi was planning to address some of your comments. She can fix this one too.

elahehrashedi · 2023-01-10T16:20:51Z

piomis this change will consider the generic tasks with no target as well as the tasks defined for the specific target.

addressed

piomis · 2023-01-10T17:08:11Z

src/cmakeTaskProvider.ts

+                return matchingTargetTasks[0];
+            } else {
+                // Search for the matching default task.
+                const defaultTask: CMakeTask[] = matchingTargetTasks.filter(task => task.group?.isDefault);


You should change task => task.group?.isDefault into task => task.isDefault
This is because in temporary task created in line 210 you do not add full group but only this one field

piomis · 2023-01-10T17:09:47Z

src/cmakeTaskProvider.ts

+                    return defaultTask[0];
+                } else {
+                    // Search for the matching existing task.
+                    const existingTask: CMakeTask[] = matchingTargetTasks.filter(task => !task.isTemplate);


From what I see isTemplate property is not set for any task retuned in buildTasks array. I guess you need to set this property in similar way like you set isDefault

By the way - I haven't found any place in code where isTemplate is set to true ?

piomis · 2023-01-10T17:10:50Z

@elahehrashedi thank you.
Note that my comment: #2780 (comment) is still valid.
If task group in task.json contains "isDefault" then your filtering lamba won't work (as group direct comparison is not working).

I also added two comments - IMHO currently neither searching for default task nor filtering out template tasks will work.
See #2780 (comment) and #2780 (comment)

I am happy to push changes if you want.
I can also share very small "test" project for testing (I confirmed my observation from three comments empirically).

piomis · 2023-01-10T17:29:47Z

@elahehrashedi
It looks that PR is automatically merged but like I wrote in above comment - IMHO it still has some important problems.

piomis · 2023-01-10T18:20:08Z

@elahehrashedi @bobbrow
Additional thing which is not working (it worked before) - stop ongoing build (via "X Stop" button on status bar). Currently it does not stop build however new button appeared on status bar "Building: xxxx" after clicking it "Cancel" button appears but it is not connected to same "stop()" handler as previous one. What leads to situation that BuildRunner status is desynchronized.

Should new Issues be reported and PRs created for these findings?

elahehrashedi · 2023-01-10T18:50:31Z

piomis thank you for testing the PR. Would you please create a new report for the new issues?

piomis · 2023-01-10T20:58:56Z

@elahehrashedi Done.

For #2935 and for #2937 I've created pull-requests.
In regards to #2936 I am not sure when you intended isTemplate to be set - thus I am not sure about correct solution.

piomis commented Oct 7, 2022

View reviewed changes

src/drivers/cmakeDriver.ts Outdated Show resolved Hide resolved

serfreeman1337 mentioned this pull request Oct 9, 2022

Colorized CMake/Build output #478

Open

Mis, Piotr added 8 commits October 10, 2022 07:24

feat: Switch types/vscode package version 1.60.0

a2a269c

fix: cleanRebuild should use task for build phase (if configured)

f250fe1

fix: When targets is started from Project Outline task should build i…

d006e16

…t insted of default target

chore: Update CHANGELOG

740fcc6

fix: When only one build task is available then it should be used

4c996de

fix: Cleanups in cmakeDriver and proc

d0d7326

- proc.ExectionOptions.useBuildTask is not needed anymore (driver is now detecting if task should be used - rename taskFinder function and remove its parameter

fix: Only default build tasks were presented in QuickPicker

fdadb91

fix: Incorrect message used when presenting task quick picker

e9c2d54

piomis force-pushed the bugfix/2778_cmake_project_outline_targets_with_build_task branch from 428e90e to e9c2d54 Compare October 10, 2022 05:30

elahehrashedi reviewed Oct 11, 2022

View reviewed changes

package.json Show resolved Hide resolved

elahehrashedi reviewed Oct 11, 2022

View reviewed changes

src/cmakeTaskProvider.ts Outdated Show resolved Hide resolved

elahehrashedi reviewed Oct 11, 2022

View reviewed changes

Mis, Piotr and others added 2 commits October 19, 2022 12:47

Merge branch 'main' into bugfix/2778_cmake_project_outline_targets_wi…

25b19e5

…th_build_task

bobbrow added this to the On Deck milestone Oct 20, 2022

piomis added 3 commits October 24, 2022 08:22

Merge branch 'main' into bugfix/2778_cmake_project_outline_targets_wi…

9f80e22

…th_build_task

Merge branch 'main' into bugfix/2778_cmake_project_outline_targets_wi…

b7a28e1

…th_build_task

Merge branch 'main' into bugfix/2778_cmake_project_outline_targets_wi…

c410793

…th_build_task

piomis requested review from elahehrashedi and bobbrow and removed request for elahehrashedi October 28, 2022 15:54

Merge branch 'main' into bugfix/2778_cmake_project_outline_targets_wi…

adcf353

…th_build_task

piomis requested review from elahehrashedi and removed request for bobbrow October 28, 2022 15:55

bobbrow reviewed Dec 2, 2022

View reviewed changes

bobbrow linked an issue Jan 9, 2023 that may be closed by this pull request

Calling builds/clean/install/test from command palette always builds default target if useTasks option is set #2768

Closed

remove Null

884edd2

elahehrashedi force-pushed the bugfix/2778_cmake_project_outline_targets_with_build_task branch from d47d27d to 884edd2 Compare January 10, 2023 15:42

elahehrashedi added 2 commits January 10, 2023 08:03

make changes to task provider

a3c1635

consider generic tasks

ccaf704

elahehrashedi added 3 commits January 10, 2023 08:38

bring null back

bdb11c2

small fix

4e711ca

lint

8b52846

bobbrow approved these changes Jan 10, 2023

View reviewed changes

elahehrashedi enabled auto-merge (squash) January 10, 2023 16:40

bobbrow modified the milestones: On Deck, 1.13 Jan 10, 2023

piomis commented Jan 10, 2023

View reviewed changes

elahehrashedi merged commit 95a8ada into microsoft:main Jan 10, 2023

piomis mentioned this pull request Feb 13, 2023

Expand targets names while searching for matching build task #3009

Merged

github-actions bot locked and limited conversation to collaborators Feb 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calling build targets from CMake Project Outline always builds default target if useTasks option is set #2780

Calling build targets from CMake Project Outline always builds default target if useTasks option is set #2780

piomis commented Oct 7, 2022 •

edited

Loading

piomis commented Oct 7, 2022

elahehrashedi Oct 11, 2022

piomis Oct 11, 2022 •

edited

Loading

elahehrashedi Oct 14, 2022

elahehrashedi Oct 11, 2022

piomis Oct 11, 2022 •

edited

Loading

elahehrashedi Oct 14, 2022

elahehrashedi Oct 14, 2022

piomis Oct 19, 2022 •

edited

Loading

bobbrow commented Oct 28, 2022

bobbrow Dec 2, 2022 •

edited

Loading

piomis Dec 2, 2022

bobbrow Dec 2, 2022

elahehrashedi commented Jan 10, 2023

piomis Jan 10, 2023

piomis Jan 10, 2023 •

edited

Loading

piomis commented Jan 10, 2023 •

edited

Loading

piomis commented Jan 10, 2023

piomis commented Jan 10, 2023

elahehrashedi commented Jan 10, 2023

piomis commented Jan 10, 2023


		export class CMakeBuildRunner {

		private taskExecutor: vscode.TaskExecution \| null = null;

Calling build targets from CMake Project Outline always builds default target if useTasks option is set #2780

Calling build targets from CMake Project Outline always builds default target if useTasks option is set #2780

Conversation

piomis commented Oct 7, 2022 • edited Loading

This change fixes #2778

piomis commented Oct 7, 2022

Choose a reason for hiding this comment

piomis Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piomis Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piomis Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

bobbrow commented Oct 28, 2022

bobbrow Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elahehrashedi commented Jan 10, 2023

Choose a reason for hiding this comment

piomis Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

piomis commented Jan 10, 2023 • edited Loading

piomis commented Jan 10, 2023

piomis commented Jan 10, 2023

elahehrashedi commented Jan 10, 2023

piomis commented Jan 10, 2023

piomis commented Oct 7, 2022 •

edited

Loading

piomis Oct 11, 2022 •

edited

Loading

piomis Oct 11, 2022 •

edited

Loading

piomis Oct 19, 2022 •

edited

Loading

bobbrow Dec 2, 2022 •

edited

Loading

piomis Jan 10, 2023 •

edited

Loading

piomis commented Jan 10, 2023 •

edited

Loading