Skip to content
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

Avoid hardcoding signtool PATH in package-windows build step #4535

Merged
merged 5 commits into from
Feb 14, 2025
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,6 @@ jobs:
Expand-Archive -Path ".\dist\k6-$env:VERSION-windows-amd64.zip" -DestinationPath .\packaging\
move .\packaging\k6-$env:VERSION-windows-amd64\k6.exe .\packaging\
rmdir .\packaging\k6-$env:VERSION-windows-amd64\
- name: Add signtool to PATH
run: echo "${env:ProgramFiles(x86)}\Windows Kits\10\bin\10.0.17763.0\x64" | Out-File -FilePath $env:GITHUB_PATH -Append

- name: Create the MSI package
run: |
Expand All @@ -254,8 +252,18 @@ jobs:
candle.exe -arch x64 "-dVERSION=$env:VERSION" k6.wxs
light.exe -ext WixUIExtension k6.wixobj

- name: Add signtool to PATH
# Searching for the signtool executable adds around 10 seconds to the
# job, but it avoids hardcoding the path to the signtool executable.
# We only run this step if we are building from master or a version tag,
# or if the workflow was manually triggered, to avoid the extra time.
if: ${{ github.ref == 'refs/heads/master' || startsWith(github.ref, 'refs/tags/v') || github.event_name == 'workflow_dispatch' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of checking it on the single step, we can probably do it on the entire job and skip it fully.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I attempted moving the PATH addition into the same step here, but signtool wasn't added to the PATH, I suppose the shell environment needs to be reloaded for the change to take place. I'll look into it some more.

What do you think of something like:

$SignToolPath = Get-ChildItem -Path "${env:ProgramFiles(x86)}\Windows Kits\10\bin" -Recurse -Filter signtool.exe | Where-Object { $_.DirectoryName -like "*\x64" } | Sort-Object -Descending | Select-Object -First 1

# Sign the Windows binary
& $SignToolPath\signtool.exe sign /f k6.pfx /p "${{ secrets.WIN_SIGN_PASS }}" /tr "http://timestamp.digicert.com" /td sha256 /fd sha256 "packaging\k6.exe"

I'm forgetting the exact syntax to call the exe from the item returned from Get-ChildItem but if that looks good to your team I'll figure it out when I'm at my Windows machine. It's a little less readable (to me) but avoids the multiple steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up moving the PATH step into the package-windows job, and just calling the executable directly as I mentioned above.

Here's the "successful" build where I kicked it off:
https://github.com/McMastS/k6/actions/runs/13275739721/job/37064965620
image

run: |
$SignToolPath = Get-ChildItem -Path "${env:ProgramFiles(x86)}\Windows Kits\10\bin" -Recurse -Filter signtool.exe | Where-Object { $_.DirectoryName -like "*\x64" } | Sort-Object -Descending | Select-Object -First 1
$SignToolPath.Directory.FullName | Out-File -FilePath $env:GITHUB_PATH -Append

- name: Sign Windows binary and .msi package
# GH secrets are unavaileble when building from project forks, so this
# GH secrets are unavailable when building from project forks, so this
# will fail for external PRs, even if we wanted to do it. And we don't.
# We are only going to sign packages that are built from master or a
# version tag, or manually triggered dev builds, so we have enough
Expand Down