-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Adding build scripts for building the workspace packages using pnpm #21326
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
base: master
Are you sure you want to change the base?
Conversation
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
…om/microsoft/azure-pipelines-tasks into users/bhatibhavesh/concurrentbuild
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
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 parallel build support for Azure Pipelines tasks using pnpm workspace filtering, enhancing build performance through concurrent execution. The changes add a new parallel build script, implement concurrent download utilities to prevent race conditions, and update the workspace configuration to support filtered parallel builds.
- Adds parallel build infrastructure with pnpm workspace support and task filtering capabilities
- Implements concurrent-safe download utilities to handle race conditions during parallel builds
- Updates build configuration and task-specific build commands for consistency
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
pnpm-workspace.yaml | Defines workspace packages for pnpm, excluding Common tasks from parallel builds |
package.json | Adds pnpm dependency and new parallel build scripts |
make.js | Adds conditional logic to skip main build steps when only pre-build steps are needed |
make-util.js | Integrates concurrent download utility for parallel build scenarios |
ci/build-all-steps.yml | Updates branch filter condition for CI pipeline |
build-scripts/download-utils.js | New concurrent download utility with atomic extraction and race condition prevention |
build-parallel.js | New parallel build orchestration script with pnpm workspace filtering |
Tasks/NotationV0/package.json | Updates build command to use make.js instead of direct tsc |
if(throwOnError) | ||
{ | ||
throw new Error('Failed to run: ' + cl + ' exit code: ' + err.status); | ||
}else{ |
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.
Missing space after 'if' keyword and inconsistent brace placement. The condition should be if (throwOnError)
and the else
should follow standard formatting conventions with proper spacing.
if(throwOnError) | |
{ | |
throw new Error('Failed to run: ' + cl + ' exit code: ' + err.status); | |
}else{ | |
if (throwOnError) | |
{ | |
throw new Error('Failed to run: ' + cl + ' exit code: ' + err.status); | |
} | |
else | |
{ |
Copilot uses AI. Check for mistakes.
const quotedArgs = args.map(arg => { | ||
// Quote arguments that contain pipes, parentheses, or spaces | ||
if (arg.includes('|') || arg.includes('(') || arg.includes(')') || arg.includes(' ')) { | ||
return `"${arg}"`; | ||
} | ||
return arg; | ||
}); |
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 argument quoting logic is flawed because arguments that are already quoted (like "./Tasks/{Task1,Task2,Task3}"
) will be double-quoted, potentially breaking shell command execution. The function should check if arguments are already properly quoted before adding additional quotes.
Copilot uses AI. Check for mistakes.
return ['--filter', `"./Tasks/${tasks[0]}"`]; | ||
} else { | ||
// Create a single filter pattern that matches multiple task paths | ||
const pathPattern = `./Tasks/{${tasks.join(',')}}`; | ||
return ['--filter', `"${pathPattern}"`]; | ||
} | ||
} | ||
|
||
// Handle single task or other patterns - use path-based filter | ||
return ['--filter', `"./Tasks/${taskPattern}"`]; |
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 filter argument generation is inconsistent and could lead to confusion. All three return statements manually wrap values in double quotes, but this should be handled consistently in one place, preferably in the executeCommand
function's argument processing.
return ['--filter', `"./Tasks/${tasks[0]}"`]; | |
} else { | |
// Create a single filter pattern that matches multiple task paths | |
const pathPattern = `./Tasks/{${tasks.join(',')}}`; | |
return ['--filter', `"${pathPattern}"`]; | |
} | |
} | |
// Handle single task or other patterns - use path-based filter | |
return ['--filter', `"./Tasks/${taskPattern}"`]; | |
return ['--filter', `./Tasks/${tasks[0]}`]; | |
} else { | |
// Create a single filter pattern that matches multiple task paths | |
const pathPattern = `./Tasks/{${tasks.join(',')}}`; | |
return ['--filter', `${pathPattern}`]; | |
} | |
} | |
// Handle single task or other patterns - use path-based filter | |
return ['--filter', `./Tasks/${taskPattern}`]; |
Copilot uses AI. Check for mistakes.
return ['--filter', `"./Tasks/${tasks[0]}"`]; | ||
} else { | ||
// Create a single filter pattern that matches multiple task paths | ||
const pathPattern = `./Tasks/{${tasks.join(',')}}`; | ||
return ['--filter', `"${pathPattern}"`]; | ||
} | ||
} | ||
|
||
// Handle single task or other patterns - use path-based filter | ||
return ['--filter', `"./Tasks/${taskPattern}"`]; |
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 filter argument generation is inconsistent and could lead to confusion. All three return statements manually wrap values in double quotes, but this should be handled consistently in one place, preferably in the executeCommand
function's argument processing.
return ['--filter', `"./Tasks/${tasks[0]}"`]; | |
} else { | |
// Create a single filter pattern that matches multiple task paths | |
const pathPattern = `./Tasks/{${tasks.join(',')}}`; | |
return ['--filter', `"${pathPattern}"`]; | |
} | |
} | |
// Handle single task or other patterns - use path-based filter | |
return ['--filter', `"./Tasks/${taskPattern}"`]; | |
return ['--filter', `./Tasks/${tasks[0]}`]; | |
} else { | |
// Create a single filter pattern that matches multiple task paths | |
const pathPattern = `./Tasks/{${tasks.join(',')}}`; | |
return ['--filter', pathPattern]; | |
} | |
} | |
// Handle single task or other patterns - use path-based filter | |
return ['--filter', `./Tasks/${taskPattern}`]; |
Copilot uses AI. Check for mistakes.
return ['--filter', `"./Tasks/${tasks[0]}"`]; | ||
} else { | ||
// Create a single filter pattern that matches multiple task paths | ||
const pathPattern = `./Tasks/{${tasks.join(',')}}`; | ||
return ['--filter', `"${pathPattern}"`]; | ||
} | ||
} | ||
|
||
// Handle single task or other patterns - use path-based filter | ||
return ['--filter', `"./Tasks/${taskPattern}"`]; |
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 filter argument generation is inconsistent and could lead to confusion. All three return statements manually wrap values in double quotes, but this should be handled consistently in one place, preferably in the executeCommand
function's argument processing.
return ['--filter', `"./Tasks/${tasks[0]}"`]; | |
} else { | |
// Create a single filter pattern that matches multiple task paths | |
const pathPattern = `./Tasks/{${tasks.join(',')}}`; | |
return ['--filter', `"${pathPattern}"`]; | |
} | |
} | |
// Handle single task or other patterns - use path-based filter | |
return ['--filter', `"./Tasks/${taskPattern}"`]; | |
return ['--filter', `./Tasks/${tasks[0]}`]; | |
} else { | |
// Create a single filter pattern that matches multiple task paths | |
const pathPattern = `./Tasks/{${tasks.join(',')}}`; | |
return ['--filter', `${pathPattern}`]; | |
} | |
} | |
// Handle single task or other patterns - use path-based filter | |
return ['--filter', `./Tasks/${taskPattern}`]; |
Copilot uses AI. Check for mistakes.
} | ||
|
||
// Atomically move to final location (only one process will succeed) | ||
try { |
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.
if this is remained from prev run it will assume it was downloaded fresh and skip ?
console.log('Running parallel build...'); | ||
console.log('pnpm command:', 'pnpm', args.join(' ')); | ||
try { |
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.
Currently the make.js prints output like this
===============================================================================
📊 BUILD SUMMARY
Total tasks: 5
✅ Successful: 0
❌ Failed: 0
⏭️ Skipped: 0
That does not work with this script anymore, we will need to update that as well.
console.log(''); | ||
console.log('Examples:'); | ||
console.log(' node build-parallel.js build --task "MyTask"'); | ||
console.log(' node build-parallel.js serverBuild --task "@(TaskA|TaskB|TaskC)"'); |
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.
This command silently ignores the wrong taskname instead it should throw an error to avoid typos and inform users of that.
node build-parallel.js build --task "@(BashV3|NonExistentTask|CmdLineV2)"
Context
This pull request introduces parallel build support for tasks, improves build performance, and adds robust concurrent download utilities. The main changes include a new parallel build script, enhancements to the build process, and updates to dependencies and workspace configuration for better task filtering and concurrency.
Work item: https://mseng.visualstudio.com/AzureDevOps/_workitems/edit/2318396
Task Name
NotationV0 - Only change is updating the build command. So not updating the Task version.
Description
Parallel build support and task filtering:
build-parallel.js
script that enables parallel building of tasks using pnpm workspace filters, supporting both single and multiple task patterns for efficient concurrent builds.package.json
with new scripts (build:parallel
,serverBuild:parallel
) and addedpnpm
as a devDependency to support parallel builds. [1] [2]pnpm-workspace.yaml
to define workspace packages and exclude common tasks from parallel builds.Concurrent download improvements:
build-scripts/download-utils.js
withdownloadArchiveConcurrentAsync
, providing safe, atomic concurrent archive extraction to prevent race conditions during parallel builds.make-util.js
so downloads are handled atomically when parallel build is enabled. [1] [2]Build process enhancements:
make.js
to allow skipping main build steps and only running pre-build steps when requested, supporting parallel build orchestration.Risk Assessment (Low)
Low risk. The change is limited to argument formatting and does not affect core build or task execution logic. Backward compatibility is maintained.
Change Behind Feature Flag (No)
This change cannot be behind a feature flag, as it affects command-line argument formatting required for correct build execution.
Tech Design / Approach
Documentation Changes Required (No)
No documentation changes required, as this is an internal implementation detail.
Unit Tests Added or Updated (No)
No new unit tests were added. Existing build/test flows cover this logic.
Additional Testing Performed
Logging Added/Updated (No)
No logging changes were necessary for this update.
Telemetry Added/Updated (No)
No telemetry changes required.
Rollback Scenario and Process (Yes)
Rollback is straightforward: revert the changes in
build-parallel.js
to the previous logic.Dependency Impact Assessed and Regression Tested (Yes)
No new dependencies introduced. Regression testing confirmed no breakage in existing build flows.
Checklist