improvement(fluid-build): Fix group task dependencies and reduce message clutter #23523

curtisman · 2025-01-10T14:32:56Z

Fix group task dependencies: For script task that has "&&" in the command, a sequential group task is created. When building the dependency graph, the latter tasks need to depend on the previous task. However, an "iterator" of previous leaf task is passed to add as dependency for the latter tasks, and if the latter tasks have multiple subtasks, the iterator will only iterate once for the first subtask and miss the other subtask since it doesn't reset. The fix is to just pass the Set into addDependentLeafTasks

Reduce message clutter:

For set up without config file, avoid having a big warning message. Merge the indication with the "Build Root" message.
Don't show symlink status unless it not in the default mode.

Additionally, make getFluidBuildConfig correctly track which searchDir is using a default config in case it is used with multiple searchDir in the future.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

curtisman · 2025-01-10T14:33:51Z

@tylerbutler FYI.

build-tools/packages/build-tools/src/fluidBuild/fluidUtils.ts

build-tools/packages/build-tools/src/fluidBuild/fluidBuild.ts

Abe27342

LGTM minus docs requests, maybe Michael/TylerB have other thoughts.

I was going to cut a flub release to adopt some of Tyler's changes--I'll wait for this to go in before doing so.

tylerbutler · 2025-01-10T22:29:36Z

I was going to cut a flub release to adopt some of Tyler's changes--I'll wait for this to go in before doing so.

@Abe27342 There's a build-tools prerelease that has the changes to unblock r11s upgrade, if you're blocked.

tylerbutler

Additionally, make getFluidBuildConfig correctly track which searchDir is using a default config in case it is used with multiple searchDir in the future.

No objection to this change, but I am curious if you have a specific scenario in mind that needs this?

build-tools/packages/build-tools/src/fluidBuild/fluidUtils.ts

tylerbutler · 2025-01-10T22:37:03Z

build-tools/packages/build-tools/src/fluidBuild/tasks/groupTask.ts

@@ -33,7 +33,7 @@ export class GroupTask extends Task {
 				if (prevTask !== undefined) {
 					const leafTasks = new Set<LeafTask>();
 					prevTask.collectLeafTasks(leafTasks);
-					task.addDependentLeafTasks(leafTasks.values());
+					task.addDependentLeafTasks(leafTasks);


Is there something we could do to the function typing to prevent this sort of mistake? Maybe it's one of those things where both could be correct, and this was just a case where the wrong value was used?

I was surprised that worked before. but from the typescript iterable definition, the SetIterator has @@iterator field, which makes it an iterable :( Not sure what else to do here.

~~Having a hard time figuring out what the error is though. Passing a Set, or <Set>.values() seems like it should be the same thing in this case (where an Iterable is expected)?~~ Never mind, missed the reason in the PR description.

tylerbutler · 2025-01-10T22:39:28Z

/azp run Build - protocol-definitions,Build - test-tools,server-gitrest,server-gitssh,server-historian,server-routerlicious,Build - client packages,repo-policy-check

tylerbutler · 2025-01-10T22:39:33Z

/azp run Build - api-markdown-documenter,Build - benchmark-tool,Build - build-common,Build - build-tools,Build - common-utils,Build - eslint-config-fluid,Build - eslint-plugin-fluid

azure-pipelines · 2025-01-10T22:39:50Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2025-01-10T22:39:50Z

Azure Pipelines successfully started running 1 pipeline(s).

Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>

curtisman · 2025-01-10T23:59:51Z

Additionally, make getFluidBuildConfig correctly track which searchDir is using a default config in case it is used with multiple searchDir in the future.

No objection to this change, but I am curious if you have a specific scenario in mind that needs this?

Nope. It was just wrong behavior, and I fixed it.

curtisman · 2025-01-11T00:00:26Z

@Abe27342 @tylerbutler all the comment should be addressed.

tylerbutler · 2025-01-11T00:29:17Z

/azp run Build - protocol-definitions,Build - test-tools,server-gitrest,server-gitssh,server-historian,server-routerlicious,Build - client packages,repo-policy-check

azure-pipelines · 2025-01-11T00:29:32Z

Azure Pipelines successfully started running 1 pipeline(s).

tylerbutler · 2025-01-11T00:29:32Z

/azp run Build - api-markdown-documenter,Build - benchmark-tool,Build - build-common,Build - build-tools,Build - common-utils,Build - eslint-config-fluid,Build - eslint-plugin-fluid

azure-pipelines · 2025-01-11T00:29:48Z

Azure Pipelines successfully started running 1 pipeline(s).

build-tools/packages/build-tools/src/fluidBuild/fluidUtils.ts

Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com>

Abe27342 · 2025-01-13T18:31:18Z

/azp run Build - api-markdown-documenter,Build - benchmark-tool,Build - build-common,Build - build-tools,Build - common-utils,Build - eslint-config-fluid,Build - eslint-plugin-fluid

azure-pipelines · 2025-01-13T18:31:36Z

Azure Pipelines successfully started running 1 pipeline(s).

Abe27342 · 2025-01-13T18:31:45Z

/azp run Build - protocol-definitions,Build - test-tools,server-gitrest,server-gitssh,server-historian,server-routerlicious,Build - client packages,repo-policy-check

azure-pipelines · 2025-01-13T18:32:03Z

Azure Pipelines successfully started running 1 pipeline(s).

fix

b9cf43a

Copilot bot review requested due to automatic review settings January 10, 2025 14:32

github-actions bot added area: build Build related issues base: main PRs targeted against main branch labels Jan 10, 2025

Copilot AI reviewed Jan 10, 2025

View reviewed changes

tylerbutler requested review from zhenmichael and a team January 10, 2025 15:42

Abe27342 reviewed Jan 10, 2025

View reviewed changes

build-tools/packages/build-tools/src/fluidBuild/fluidUtils.ts Outdated Show resolved Hide resolved

Abe27342 reviewed Jan 10, 2025

View reviewed changes

build-tools/packages/build-tools/src/fluidBuild/fluidUtils.ts Show resolved Hide resolved

Abe27342 reviewed Jan 10, 2025

View reviewed changes

build-tools/packages/build-tools/src/fluidBuild/fluidBuild.ts Outdated Show resolved Hide resolved

Abe27342 approved these changes Jan 10, 2025

View reviewed changes

tylerbutler changed the title ~~[fluid-build]: Fix group task dependencies and reduce message clutter~~ improvement(fluid-build): Fix group task dependencies and reduce message clutter Jan 10, 2025

tylerbutler approved these changes Jan 10, 2025

View reviewed changes

build-tools/packages/build-tools/src/fluidBuild/fluidUtils.ts Outdated Show resolved Hide resolved

tylerbutler reviewed Jan 10, 2025

View reviewed changes

Update build-tools/packages/build-tools/src/fluidBuild/fluidBuild.ts

6bdf652

Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>

curtisman force-pushed the fix branch from b8a0344 to 0344a12 Compare January 11, 2025 00:01

comment

fd6ba7a

curtisman force-pushed the fix branch from 0344a12 to fd6ba7a Compare January 11, 2025 00:01

alexvy86 reviewed Jan 13, 2025

View reviewed changes

build-tools/packages/build-tools/src/fluidBuild/fluidUtils.ts Outdated Show resolved Hide resolved

Update build-tools/packages/build-tools/src/fluidBuild/fluidUtils.ts

2ae7ce9

Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com>

Abe27342 enabled auto-merge (squash) January 13, 2025 18:31

Abe27342 merged commit 7a16748 into microsoft:main Jan 13, 2025
26 checks passed

curtisman deleted the fix branch January 13, 2025 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement(fluid-build): Fix group task dependencies and reduce message clutter #23523

improvement(fluid-build): Fix group task dependencies and reduce message clutter #23523

curtisman commented Jan 10, 2025 •

edited

Loading

curtisman commented Jan 10, 2025

Abe27342 left a comment

tylerbutler commented Jan 10, 2025

tylerbutler left a comment

tylerbutler Jan 10, 2025

curtisman Jan 10, 2025 •

edited

Loading

alexvy86 Jan 13, 2025 •

edited

Loading

tylerbutler commented Jan 10, 2025

tylerbutler commented Jan 10, 2025

azure-pipelines bot commented Jan 10, 2025

azure-pipelines bot commented Jan 10, 2025

curtisman commented Jan 10, 2025

curtisman commented Jan 11, 2025

tylerbutler commented Jan 11, 2025

azure-pipelines bot commented Jan 11, 2025

tylerbutler commented Jan 11, 2025

azure-pipelines bot commented Jan 11, 2025

Abe27342 commented Jan 13, 2025

azure-pipelines bot commented Jan 13, 2025

Abe27342 commented Jan 13, 2025

azure-pipelines bot commented Jan 13, 2025

improvement(fluid-build): Fix group task dependencies and reduce message clutter #23523

improvement(fluid-build): Fix group task dependencies and reduce message clutter #23523

Conversation

curtisman commented Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

curtisman commented Jan 10, 2025

Abe27342 left a comment

Choose a reason for hiding this comment

tylerbutler commented Jan 10, 2025

tylerbutler left a comment

Choose a reason for hiding this comment

tylerbutler Jan 10, 2025

Choose a reason for hiding this comment

curtisman Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

alexvy86 Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

tylerbutler commented Jan 10, 2025

tylerbutler commented Jan 10, 2025

azure-pipelines bot commented Jan 10, 2025

azure-pipelines bot commented Jan 10, 2025

curtisman commented Jan 10, 2025

curtisman commented Jan 11, 2025

tylerbutler commented Jan 11, 2025

azure-pipelines bot commented Jan 11, 2025

tylerbutler commented Jan 11, 2025

azure-pipelines bot commented Jan 11, 2025

Abe27342 commented Jan 13, 2025

azure-pipelines bot commented Jan 13, 2025

Abe27342 commented Jan 13, 2025

azure-pipelines bot commented Jan 13, 2025

curtisman commented Jan 10, 2025 •

edited

Loading

curtisman Jan 10, 2025 •

edited

Loading

alexvy86 Jan 13, 2025 •

edited

Loading