-
Notifications
You must be signed in to change notification settings - Fork 553
feat(fluid-build): Leaf Task File Dependencies #24929
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
Conversation
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.
Pull Request Overview
This PR introduces file dependency support for leaf tasks within fluid-build, allowing tasks to specify input/output globs for incremental builds. The changes remove free function task handlers in favor of constructor-based ones, update task creation logic to handle file dependencies, and enforce validation rules for file configurations in global and non-script task definitions.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
build-tools/src/fluidBuild/tasks/taskHandlers.ts | Removed the free function task handler type and related type guard. |
build-tools/src/fluidBuild/tasks/taskFactory.ts | Updated task creation functions to pass file dependency configurations and added error checks for file dependencies on unsupported tasks. |
build-tools/src/fluidBuild/tasks/leaf/leafTask.ts | Modified return types of glob-fetching methods to accommodate optional file lists. |
build-tools/src/fluidBuild/tasks/leaf/declarativeTask.ts | Renamed and refactored declarative task handler to use the updated constructor-based approach. |
build-tools/src/fluidBuild/fluidTaskDefinitions.ts | Added TaskFileDependencies along with validations for file dependency configurations. |
build-tools/src/fluidBuild/fluidBuildConfig.ts | Updated DeclarativeTask interface to extend TaskFileDependencies. |
build-tools/src/fluidBuild/buildGraph.ts | Propagated file dependency information to script task creation. |
build-tools/packages/build-tools/src/fluidBuild/tasks/taskFactory.ts
Outdated
Show resolved
Hide resolved
build-tools/packages/build-tools/src/fluidBuild/tasks/taskFactory.ts
Outdated
Show resolved
Hide resolved
build-tools/packages/build-tools/src/fluidBuild/tasks/taskFactory.ts
Outdated
Show resolved
Hide resolved
@tylerbutler @scottn12 FYI. |
0108a67
to
c59d63b
Compare
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.
Mostly nits.
build-tools/packages/build-tools/src/fluidBuild/fluidTaskDefinitions.ts
Outdated
Show resolved
Hide resolved
build-tools/packages/build-tools/src/fluidBuild/tasks/taskFactory.ts
Outdated
Show resolved
Hide resolved
build-tools/packages/build-tools/src/fluidBuild/tasks/taskFactory.ts
Outdated
Show resolved
Hide resolved
* @param command the command to create the task for | ||
* @param context the build context | ||
* @param taskName target name | ||
* @param files file dependencies for the task, if any |
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.
Are these paths relative to the package or to the repo root? Or are they absolute?
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.
Package relative
// If the task has file dependencies specification, create a declarative task | ||
if (files !== undefined) { | ||
return new DeclarativeLeafTask(node, command, context, taskName, files); | ||
} |
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.
Putting this first means that you can't override one of the built-in tasks (e.g. the eslint one) to change its inputs/outputs, right?
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.
Do you mean it can override the built-in behavior? If so, then yes, that's the intent
build-tools/packages/build-tools/src/fluidBuild/tasks/taskFactory.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
@scottn12 I have a follow up PR after this one. Wondering what's the next step to get this merged? |
Sorry, I've been a bit busy with some other work. I'll review this shortly! |
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.
Tested this out the changes locally, LGTM!
@scottn12 Is there something that needs to happen for the CI to start? |
@curtisman, maybe try updating the branch from main? I think I've seen this happen before and that restarted it |
/azp run Build - protocol-definitions,Build - test-tools,server-gitrest,server-gitssh,server-historian,server-routerlicious,Build - client packages,repo-policy-check |
/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 successfully started running 1 pipeline(s). |
1 similar comment
Azure Pipelines successfully started running 1 pipeline(s). |
Add optional file dependency specification on task definitions for leaf tasks, similar to
declarativeTask
per executable name.Thus, it enables: