Avoid hardcoding signtool PATH in package-windows build step #4535
Conversation
McMastS
requested review from
oleiade and
codebien
and removed request for
a team
.github/workflows/build.yml
Outdated
| # 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' }} |
There was a problem hiding this comment.
Instead of checking it on the single step, we can probably do it on the entire job and skip it fully.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
codebien
requested review from
codebien and
mstoykov
and removed request for
oleiade
| $SignTool = 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 | ||
|
|
There was a problem hiding this comment.
I am not familiar with this windows syntax so no idea how well that is working.
cc @oleiade as our windows person on that.
On the remaining parts - I am also not certain what the directory structure of this is - I rememer trying to figure it out when I last updated it and .... the docs weren't useful.
is the 10 here the windows version? And we need to change it to 11 when we go to windows 11?
Is it the version of the Windows kits?!?!
I guess this works somewhat better as at least we do not need the 10.0.17763.0 part 🤷
But IMO M$ should just have the thing in the PATH ... or have a way to set it without having to stuff like this.
There was a problem hiding this comment.
Yea, I 100% agree this should just be on the PATH. Everything I found in my search was people hardcoding the path or doing a search like this. As you said the official docs were very unhelpful.
I'm not sure what the 10 is in the signtool path. My Windows 11 machine also has the latest signtool in the same Windows Kits\10\bin directory.
There was a problem hiding this comment.
LGTM in general, thanks for taking on this @McMastS 🙇
Again I am not certain this improves stuff enough, I was really hoping I just have skipped some standard M$ blessed way of getting/calling singtool.
I have not tested this and do not understand enough of windows/pwsh syntax to know if this is how you are suppsoed to do it.
Maybe @oleiade knows more about this.
|
Regarding the failed |
@McMastS No, you shouldn't. This is our fault, we are going to look into it, and we'll let you know as soon as it is fixed. Sorry for the inconvenience. |
|
Hey @McMastS thanks for the contribution! 🎉 Much appreciated 🙇 |
Oh, no problem! I wanted to make sure I hadn't broken anything. Happy to help! |
What?
Searches the
Windows Kit\bindirectory for the most recentsigntoolexecutable, instead of hardcoding the exact version.This PR also moves the
Add signtool to PATHbuild step to right above the signing step, and only runs in the same conditions used for the signing step. The condition is added because the search adds around 20 seconds, so it shouldn't be run for all builds.Successful manual test showing signtool was called
Why?
The path to the
signtoolexecutable recently had to be changed because Windows moved its directory. This change means thesigntoolpath won't need to be manually updated every time the Windows version is upgraded.Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)
Closes #4105