Reuse managed windows artifacts in release#5002
Conversation
iskiselev
changed the title
Unify container and non-container build to use same actions with custom actions to hide the difference
iskiselev
force-pushed
the
artifact-normalize
branch
from
c2c320a to
89b346d
Compare
SdkVersionAnalyzer scan actions in addition to workflow Restrict non-literal argument for actions/setup-dotnet Yaml modification now replace only required block, not touching other part of file Docker file does not force dotnet 8 base.
iskiselev
force-pushed
the
artifact-normalize
branch
from
f219355 to
f0df27e
Compare
| - name: Replace target paths from artifacts | ||
| if: ${{ inputs.replace-artifacts != '' }} | ||
| shell: pwsh | ||
| run: '& "${{ github.action_path }}/replace-build-artifacts.ps1"' |
There was a problem hiding this comment.
Use environment variables such as ${env:GITHUB_ACTION_PATH} is preferable to interpolation as it removes any possibility of shell injection and makes it easier to copy-paste snippets to terminals for testing.
| - name: Validate cleanup inputs | ||
| shell: pwsh | ||
| run: | | ||
| $environmentType = '${{ inputs.environment-type }}' | ||
| $containerName = '${{ inputs.container-name }}' | ||
|
|
||
| if ($environmentType -notin @('host', 'container')) { | ||
| throw "environment-type must be either 'host' or 'container'." | ||
| } | ||
|
|
||
| if ($environmentType -eq 'container' -and [string]::IsNullOrEmpty($containerName)) { | ||
| throw 'container-name input is required when environment-type is container.' | ||
| } | ||
|
|
||
| - name: Stop build container | ||
| if: ${{ inputs.environment-type == 'container' && inputs.container-name != '' }} | ||
| shell: bash | ||
| run: | | ||
| set -u | ||
| docker rm -f "${{ inputs.container-name }}" |
There was a problem hiding this comment.
Same here with interpolation. Either assign them to env vars and use those, or maybe they'll be available already as ${env:INPUT_NAME} like ${env:INPUT_ENVIRONMENT-TYPE} (I can't remember if that's a thing or not).
| } | ||
|
|
||
| - name: Stop build container | ||
| if: ${{ inputs.environment-type == 'container' && inputs.container-name != '' }} |
There was a problem hiding this comment.
${{ and }} isn't needed for if:.
There was a problem hiding this comment.
Same comments around interpolation.
There was a problem hiding this comment.
Same comments around interpolation.
| $sourceRoot = [System.IO.Path]::GetFullPath((Join-Path $workspaceRoot 'bin/ci-artifacts/replacements')) | ||
| $targetRoot = [System.IO.Path]::GetFullPath((Join-Path $workspaceRoot $env:TARGET_ROOT)) | ||
|
|
||
| function Test-IsUnderRoot { |
There was a problem hiding this comment.
Is Path.IsPathRooted() available to the PowerShell version on the runners?
There was a problem hiding this comment.
Same comments around interpolation.
There was a problem hiding this comment.
Same comments around interpolation.
| artifact-name: windows-2022 | ||
| runner: windows-2022 | ||
| environment-type: host | ||
| log-directory: /c/ProgramData/OpenTelemetry .NET AutoInstrumentation/logs |
There was a problem hiding this comment.
Shouldn't this be a Windows form of path?
| set -e | ||
| docker build -t mybuildimage -f "./docker/${{ matrix.base-image }}.dockerfile" . | ||
| docker run -e OS_TYPE=${{ matrix.os-type }} --rm --mount type=bind,source="${GITHUB_WORKSPACE}",target=/project mybuildimage \ | ||
| docker run -e HOME=/tmp -e OS_TYPE=${{ matrix.os-type }} --user "$(id -u):$(id -g)" --rm --mount type=bind,source="${GITHUB_WORKSPACE}",target=/project mybuildimage \ |
There was a problem hiding this comment.
Same comments around interpolation.
There was a problem hiding this comment.
Pull request overview
Updates the CI/release build matrix so non-Windows builds replace the net managed artifacts with those produced by the Windows build, aiming to make managed binaries consistent across distributions (per #4520).
Changes:
- Add
bin-windows-2022!nettoreplace-artifactsfor non-Windows matrix entries. - Ensure the reusable
_build.ymlworkflow receives thereplace-artifactsinput for each matrix job (already wired; now exercised more broadly).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@martincostello, thanks for the review. We will track improvements on #5011. Merging in current state as other changes are already on main. |
Why
Fixes #4520
What
All non-windows build after compilation replace
/netfolder from windows compilation - so we still validate that build on all systems not broken, but test prepared artifacts - using same model as profiler replace on ubuntu build.Done on top of #5005. Main change in 89b346d.
Checklist
- [ ]CHANGELOG.mdis updated.- [ ] Documentation is updated.- [ ] New features are covered by tests.