Skip to content

Conversation

@iceweasel-oai
Copy link
Contributor

This fixes the previous bug (the exe path was wrong when we were generating SHAs)
and also makes it so the main job won't fail if this part fails.

Comment on lines +590 to +592
version: ${{ needs.release.outputs.version }}
windows_x64_sha256: ${{ needs.release.outputs.windows_x64_sha256 }}
windows_arm64_sha256: ${{ needs.release.outputs.windows_arm64_sha256 }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is needs.release the right prefix?

@@ -0,0 +1,36 @@
WinGet manifests for the Codex CLI
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not expect to find this here. I would make this the README.md for the .github/winget_templates folder. Or make this .github/workflows/winget.md.

Comment on lines +411 to +421
if [[ -f "$x64_path" ]]; then
x64_sha=$(sha256sum "$x64_path" | awk '{print $1}')
else
echo "::warning::Windows x64 binary not found at $x64_path; winget validation will be skipped."
fi
if [[ -f "$arm_path" ]]; then
arm_sha=$(sha256sum "$arm_path" | awk '{print $1}')
else
echo "::warning::Windows arm64 binary not found at $arm_path; winget validation will be skipped."
fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reasonable case in which these should fail? Should we just do this:

set -euo pipefail
x64_sha=$(sha256sum "$x64_path" | awk '{print $1}')
arm_sha=$(sha256sum "$arm_path" | awk '{print $1}')

why should we sweep errors under the rug?


- name: Compute SHA256 for Windows EXEs
id: win_hash
continue-on-error: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assuming we make my suggested change below, I would drop this, as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants