Optimize/split windows package signing #26403
Draft
+906
−375
Conversation
This file contains hidden or 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
9845544 to
fe1c814
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request makes several improvements to the PowerShell packaging and CI/CD infrastructure:
- macOS Package Naming: Changes macOS package architecture naming from
x86_64tox64for better compatibility and consistency with other platforms - Windows Packaging Pipeline Refactoring: Splits Windows package creation into separate build and signing stages for better separation of concerns
- OneBranch Template Improvements:
- Refactors templates to use parameterized
ob_restore_phasecontrol - Adds proper signing configuration for build-only vs signing jobs
- Adds validation for REPOROOT in cloneToOfficialPath template
- Refactors templates to use parameterized
- Test Improvements: Adds comprehensive validation tests for macOS package naming conventions
- Cleanup: Removes unused
UseJsonparameter from SetVersionVariables template - Bug Fix: Corrects Mariner dpkg installation commands to avoid duplicate "install -y" flags
- Code Quality: Extensive whitespace cleanup and improved verbose logging
All changes reviewed and only one minor issue identified with instruction file formatting.
PR Overview
This pull request modernizes the PowerShell packaging pipeline infrastructure with several key improvements focused on build/signing separation, macOS package naming standardization, and OneBranch template refactoring.
- Standardizes macOS package naming from
x86_64tox64for cross-platform consistency - Refactors Windows packaging to separate unsigned build and signing into distinct stages
- Improves OneBranch template configurability with parameterized
ob_restore_phasecontrol
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/packaging/packaging.psm1 | Implements x64 architecture naming for macOS packages; whitespace cleanup |
| test/packaging/macos/package-validation.tests.ps1 | Adds validation tests for macOS package naming convention; copyright header |
| build.psm1 | Fixes Mariner dpkg installation command syntax; adds diagnostic logging |
| .pipelines/templates/windows-package-build.yml | Removes unused CreateJson/UseJson parameters |
| .pipelines/templates/packaging/windows/sign.yml | New template for Windows package signing stage |
| .pipelines/templates/packaging/windows/package.yml | New template for Windows package build stage (unsigned) |
| .pipelines/templates/SetVersionVariables.yml | Refactors to extract set-reporoot logic; parameterizes ob_restore_phase |
| .pipelines/templates/set-reporoot.yml | New extracted template for repository root detection |
| .pipelines/templates/shouldSign.yml | Adds ob_restore_phase parameter |
| .pipelines/templates/install-dotnet.yml | Adds ob_restore_phase parameter |
| .pipelines/templates/cloneToOfficialPath.yml | Adds REPOROOT validation; parameterizes ob_restore_phase |
| .pipelines/templates/mac-package-build.yml | Fixes package upload to handle multiple files; removes CreateJson/UseJson |
| .pipelines/templates/package-create-msix.yml | Corrects artifact names after Windows build stage restructuring |
| .pipelines/PowerShell-Packages-Official.yml | Restructures to separate windows_package_build and windows_package_sign stages |
| .github/workflows/macos-ci.yml | Updates test path reference |
| .github/instructions/onebranch-signing-configuration.instructions.md | New guide for OneBranch signing configuration |
| .github/instructions/onebranch-restore-phase-pattern.instructions.md | New guide for OneBranch restore phase patterns |
| .github/instructions/build-configuration-guide.instructions.md | Adds pipelines to applyTo scope |
| Various other pipeline templates | Remove unused CreateJson/UseJson parameters; update artifact names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…cope Add comprehensive guides for: - OneBranch restore phase patterns (correct vs incorrect usage) - OneBranch signing configuration (when to disable signing) Update build-configuration-guide to include .pipelines/**/*.yml files in the applyTo scope to ensure guidance applies to all pipeline files.
Extract repository root detection logic into a separate reusable template (set-reporoot.yml) to improve modularity and reduce duplication. Update SetVersionVariables.yml to: - Use the new set-reporoot template - Add ob_restore_phase boolean parameter with proper type annotation - Pass ob_restore_phase to all steps that need restore phase control - Remove deprecated UseJson parameter handling
Add parameterized ob_restore_phase control to: - shouldSign.yml - install-dotnet.yml This allows callers to control whether these steps run in the OneBranch restore phase or the normal build phase, following the documented pattern.
… control Enhance cloneToOfficialPath.yml template with: - Add ob_restore_phase boolean parameter for restore phase control - Add validation to ensure REPOROOT environment variable is set - Add validation to verify build.psm1 exists at REPOROOT - Improve error messages with specific guidance - Add verbose logging for source and destination paths This prevents cryptic git clone errors when REPOROOT is not configured.
Clean up deprecated UseJson parameter across all templates: - Remove UseJson parameter from template calls - Set CreateJson to 'no' for regular builds (keep 'yes' only in prep stage) - Simplify SetVersionVariables template parameter lists The UseJson parameter was used for downloading build info from artifacts, but this approach has been superseded by environment-based configuration. Files updated: - mac-package-build.yml - nupkg.yml - linux-package-build.yml - checkAzureContainer.yml - testartifacts.yml - uploadToAzure.yml - PowerShell-Coordinated_Packages-Official.yml - MSIXBundle-vPack-Official.yml - PowerShell-vPack-Official.yml
Add packaging/windows/package.yml template to build unsigned Windows packages: - Builds MSI, ZIP, and MSIX packages from signed binaries - Downloads signed binaries from CoOrdinatedBuildPipeline artifacts - Uses ob_restore_phase: false for all template calls - Uploads to runtime-specific artifact names: - drop_windows_package_fxdependent - drop_windows_package_fxdependentWinDesktop - drop_windows_package_win7x64 - drop_windows_package_win7x86 This template focuses solely on building unsigned packages, enabling separation from the signing stage in the pipeline.
Add packaging/windows/sign.yml template to sign Windows packages:
- Signs MSI, ZIP, and MSIX packages from package stage
- Downloads unsigned packages from drop_windows_package_{runtime} artifacts
- Configures OneBranch signing for each package type
- Creates signed EXE installer from signed MSI
- Uploads to runtime-specific signed artifact names:
- drop_windows_package_signed_fxdependent
- drop_windows_package_signed_fxdependentWinDesktop
- drop_windows_package_signed_win7x64
- drop_windows_package_signed_win7x86
This template handles all package signing operations,
separating concerns from the package build stage.
Update PowerShell-Packages-Official.yml to use split Windows pipeline: - Replace single windows_package stage with two stages: - windows_package: Build unsigned MSI/ZIP/MSIX packages - windows_sign_package: Sign packages and create EXE - windows_sign_package depends on windows_package stage - Each runtime (fxdependent, fxdependentWinDesktop, win7x64, win7x86) flows through both stages Benefits: - Clearer separation of concerns (build vs sign) - OneBranch signing configuration isolated to signing stage - Easier to debug build vs signing issues - Better alignment with OneBranch security model
Update package-create-msix.yml to use simplified artifact names: - drop_windows_package_arm64 (was drop_windows_package_package_win_arm64) - drop_windows_package_x64 (was drop_windows_package_package_win_x64) - drop_windows_package_x86 (was drop_windows_package_package_win_x86) This aligns with the artifact naming convention established in the Windows package templates, removing the redundant 'package_win' prefix.
Complete UseJson parameter cleanup in remaining templates: - .pipelines/templates/compliance/apiscan.yml - .pipelines/templates/release-MakeBlobPublic.yml (2 jobs) Sets CreateJson to 'no' and removes UseJson parameter, consistent with other template updates.
Add .github/chatmodes/cherry-pick-commits.chatmode.md: - Provides instructions for cherry-picking commits between branches - Includes safety checks and verification steps - Handles rebased commits and branch divergence scenarios
Delete 4 pipeline templates that are no longer referenced: - windows-package-build.yml (replaced by packaging/windows/package.yml + sign.yml) - release-checkout-pwsh-repo.yml - release-download-packages.yml - release-install-pwsh.yml These files are not used by any pipeline configuration.
281ef7c to
e014746
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| env: | ||
| ob_restore_phase: true | ||
| - template: /.pipelines/templates/install-dotnet.yml@self |
There was a problem hiding this comment.
The install-dotnet.yml template now requires an ob_restore_phase parameter (default: true). This step should explicitly pass ob_restore_phase: true parameter for consistency with other restore phase steps (lines 40, 58, 66, 83).
Suggested change
| - template: /.pipelines/templates/install-dotnet.yml@self | |
| - template: /.pipelines/templates/install-dotnet.yml@self | |
| parameters: | |
| ob_restore_phase: true |
| ReleaseTagVar: $(ReleaseTagVar) | ||
| CreateJson: no | ||
|
|
||
| - template: /.pipelines/templates/shouldSign.yml@self |
There was a problem hiding this comment.
The shouldSign.yml template now requires an ob_restore_phase parameter (default: true). This step should explicitly pass the parameter for consistency, likely ob_restore_phase: true to match the checkout and download steps.
Suggested change
| - template: /.pipelines/templates/shouldSign.yml@self | |
| - template: /.pipelines/templates/shouldSign.yml@self | |
| parameters: | |
| ob_restore_phase: true |
|
|
||
| - template: /.pipelines/templates/cloneToOfficialPath.yml@self | ||
| parameters: | ||
| nativePathRoot: '$(Agent.TempDirectory)' |
There was a problem hiding this comment.
The cloneToOfficialPath.yml template now requires an ob_restore_phase parameter (default: true). This step should explicitly pass the parameter for consistency, likely ob_restore_phase: true to match the restore phase pattern used in other steps.
Suggested change
| nativePathRoot: '$(Agent.TempDirectory)' | |
| nativePathRoot: '$(Agent.TempDirectory)' | |
| ob_restore_phase: true |
| - template: /.pipelines/templates/SetVersionVariables.yml@self | ||
| parameters: | ||
| ReleaseTagVar: $(ReleaseTagVar) | ||
| CreateJson: yes |
There was a problem hiding this comment.
The UseJson parameter has been removed from SetVersionVariables.yml. However, this template is still passing CreateJson: yes while other templates have changed to CreateJson: no. Verify that this template actually needs JSON creation, as this appears to be the only remaining use of CreateJson: yes in the codebase.
Suggested change
| CreateJson: yes | |
| CreateJson: no |
Comment on lines
+15
to
+22
| Write-Verbose "reporoot detect at: ." -Verbose | ||
| $repoRoot = '.' | ||
| } | ||
| else{ | ||
| $path = "./PowerShell/build.psm1" | ||
| if(Test-Path -Path $path) | ||
| { | ||
| Write-Verbose "reporoot detect at: ./PowerShell" -Verbose |
There was a problem hiding this comment.
Corrected 'detect' to 'detected' for grammatical accuracy.
Suggested change
| Write-Verbose "reporoot detect at: ." -Verbose | |
| $repoRoot = '.' | |
| } | |
| else{ | |
| $path = "./PowerShell/build.psm1" | |
| if(Test-Path -Path $path) | |
| { | |
| Write-Verbose "reporoot detect at: ./PowerShell" -Verbose | |
| Write-Verbose "reporoot detected at: ." -Verbose | |
| $repoRoot = '.' | |
| } | |
| else{ | |
| $path = "./PowerShell/build.psm1" | |
| if(Test-Path -Path $path) | |
| { | |
| Write-Verbose "reporoot detected at: ./PowerShell" -Verbose |
Comment on lines
+15
to
+22
| Write-Verbose "reporoot detect at: ." -Verbose | ||
| $repoRoot = '.' | ||
| } | ||
| else{ | ||
| $path = "./PowerShell/build.psm1" | ||
| if(Test-Path -Path $path) | ||
| { | ||
| Write-Verbose "reporoot detect at: ./PowerShell" -Verbose |
There was a problem hiding this comment.
Corrected 'detect' to 'detected' for grammatical accuracy.
Suggested change
| Write-Verbose "reporoot detect at: ." -Verbose | |
| $repoRoot = '.' | |
| } | |
| else{ | |
| $path = "./PowerShell/build.psm1" | |
| if(Test-Path -Path $path) | |
| { | |
| Write-Verbose "reporoot detect at: ./PowerShell" -Verbose | |
| Write-Verbose "reporoot detected at: ." -Verbose | |
| $repoRoot = '.' | |
| } | |
| else{ | |
| $path = "./PowerShell/build.psm1" | |
| if(Test-Path -Path $path) | |
| { | |
| Write-Verbose "reporoot detected at: ./PowerShell" -Verbose |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
PR Summary
This pull request introduces several improvements and refactorings to the PowerShell build pipelines, focusing on clearer documentation, improved restore phase handling, and a more modular, maintainable pipeline structure. The main changes include new and updated documentation for OneBranch pipeline patterns, a refactor of how the restore phase is configured and propagated, and a restructuring of build and signing stages for better separation of concerns.
Documentation and Guidance Updates:
.github/instructions/onebranch-restore-phase-pattern.instructions.md)..github/instructions/onebranch-signing-configuration.instructions.md).Pipeline Restore Phase Improvements:
SetVersionVariables.yml,cloneToOfficialPath.yml,install-dotnet.yml) to use an explicitob_restore_phaseboolean parameter, ensuring restore phase variables are correctly propagated and making restore phase configuration more consistent. [1] [2] [3]ob_restore_phaseto use the parameterized value instead of hardcodedtrue, supporting better template reusability and correctness. [1] [2] [3]Pipeline Structure and Stage Refactoring:
windows_package_build,windows_package_sign), allowing unsigned artifacts to be built before signing and improving stage dependencies and clarity (PowerShell-Packages-Official.yml).Template and Variable Clean-up:
UseJsonfrom templates and their invocations for simplification and to prevent confusion. [1] [2] [3] [4]Build Configuration Scope Expansion:
.pipelines/**/*.ymlfiles, ensuring new documentation and patterns apply to all pipeline definitions.These changes collectively improve the maintainability, clarity, and reliability of the PowerShell build pipelines, making it easier for contributors to understand and correctly configure both restore and signing phases.
PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header