Fix windows velopack package creation in GHA#333
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdates Velopack tool and port metadata to 1.2.0, adds Windows packaging metadata and macOS DMG naming changes, and splits Windows packaging plus prerelease publishing into dedicated workflow jobs. ChangesVelopack 1.2.0 upgrade and CI restructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yaml:
- Around line 471-477: The Velopack packaging job restores a local .NET tool
without first bringing down the repository contents, so the tool manifest is
missing. Update the workflow step sequence around the Setup .NET for Velopack
and Install Velopack CLI steps to check out the repo before running dotnet tool
restore, ensuring dotnet-tools.json is present so vpk can be restored
successfully.
- Around line 479-483: The Fetch Windows app step is downloading a hard-coded
artifact name that does not match what the build job publishes. Update the
download-artifact usage in the workflow so it references the same artifact
naming pattern produced by the build matrix (the one used by the build job for
"${artifact}-app"), and keep the change aligned with the existing build/download
steps so the correct Windows app artifact is retrieved.
- Around line 487-497: The workflow is consuming Velopack metadata outputs that
the build job does not currently expose. Update the build job to export the new
metadata from viewer_manifest.py by adding the missing outputs for
velopack_pack_authors, velopack_splash, and velopack_splash_color, and ensure
the downstream job references the build job’s declared outputs consistently.
Locate the build job outputs and the env mapping that uses needs.build.outputs.*
to wire the new values through the job boundary.
- Around line 673-675: The release job dependency list is referencing a missing
workflow job, which makes the workflow invalid. Update the release job’s needs
in build.yaml to either remove sign-and-package-mac or restore the corresponding
job definition so the job graph matches the defined jobs and the workflow can
start successfully.
- Around line 686-689: The workflow step is downloading the wrong artifact name,
so the Linux package will not be found. Update the download step in the build
workflow to use the same published artifact name produced by the upload logic
for the Linux build, matching the `${artifact}-app` naming used by the build
job. Keep the change aligned with the artifact download step that uses
actions/download-artifact so it resolves the correct Linux app package.
- Around line 526-530: The Rename Velopack outputs step is pulling PACK_ID and
INSTALLER_BASE from undefined workflow inputs, so the values are empty and the
installer rename/staging logic fails. Update the environment setup in the rename
step to read these values from needs.build.outputs instead, using the build job
outputs that already carry the Velopack metadata. Make the change in the step
that uses the Rename Velopack outputs logic so the installer path is resolved
correctly and the file is staged into .app.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 92f878c6-b061-4859-8bcd-18e180127f62
📒 Files selected for processing (5)
.github/workflows/build.yamldotnet-tools.jsonindra/newview/viewer_manifest.pyindra/vcpkg/ports/velopack/portfile.cmakeindra/vcpkg/ports/velopack/vcpkg.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yaml:
- Line 696: The release name is referencing the wrong job output, since build
does not expose viewer_channel. Update the workflow expression in the release
naming step to read viewer_channel from the setup job output instead, while
keeping viewer_version unchanged, so the name uses the correct source values.
- Around line 501-512: The Windows pack step in the build workflow currently
runs only `vpk pack` without signing, so the resulting artifact is unsigned
before publish. Update the `vpk_args` assembly in the Velopack pack step to pass
a signing template via `--signTemplate` (or the `VPK_SIGN_TEMPLATE` environment
variable), alongside the existing pack metadata flags. Keep the change localized
to the `vpk_args` block so the packaged Windows artifact is signed as part of
the same step.
- Line 468: The Windows packaging step is incorrectly reading Velopack metadata
from the matrix-wide build job, which can be overwritten by non-Windows legs and
is also missing some required values. Update the workflow around
sign-and-package-windows so it sources Windows-only Velopack metadata from a
Windows-specific job or artifact instead of needs.build.outputs, and make sure
velopack_pack_authors, velopack_splash, and velopack_splash_color are published
and consumed from that Windows-only source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b8398eeb-a48e-4c6a-8022-6bff524adce2
📒 Files selected for processing (5)
.github/workflows/build.yamldotnet-tools.jsonindra/newview/viewer_manifest.pyindra/vcpkg/ports/velopack/portfile.cmakeindra/vcpkg/ports/velopack/vcpkg.json
✅ Files skipped from review due to trivial changes (2)
- dotnet-tools.json
- indra/vcpkg/ports/velopack/vcpkg.json
🚧 Files skipped from review as they are similar to previous changes (2)
- indra/vcpkg/ports/velopack/portfile.cmake
- indra/newview/viewer_manifest.py
RyeMutt
force-pushed
the
rye/gha-fixes
branch
3 times, most recently
from
0272fd2 to
9ea38d8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/build.yaml (3)
708-710: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winDownload the Linux artifact using the same build-type suffix as the build job.
For open-source builds, the build job publishes
Linux-OS-x64-app, but the release job always requestsLinux-x64-app.Proposed fix
- uses: actions/download-artifact@v8 with: - name: "Linux-x64-app" + name: ${{ needs.setup.outputs.build_type == 'proprietary' && 'Linux-x64-app' || 'Linux-OS-x64-app' }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yaml around lines 708 - 710, The release workflow is downloading the wrong Linux artifact name; update the `actions/download-artifact` step in the build/release workflow to request the same build-type suffixed artifact that the build job publishes, matching the naming used by the Linux build job for open-source builds. Use the existing artifact naming convention in the workflow so the download target aligns with the build output instead of hardcoding `Linux-x64-app`.
521-532: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPass signing configuration to
vpk pack.This step is named “Build and sign,” but no signing template or signing params are supplied, so the Windows installer can still be published unsigned.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yaml around lines 521 - 532, The “Build and sign” workflow step builds the installer with vpk_args but never passes any signing configuration, so update the vpk pack invocation to include the required signing template and/or signing parameters. Use the existing vpk_args block in the build workflow to thread the configured signing values into vpk pack so the Windows installer is actually signed before publishing.
166-172: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDon’t pass Windows Velopack metadata through matrix-wide
build.outputs.These outputs are shared across all matrix legs, so macOS/Linux can overwrite the Windows values consumed by
sign-and-package-windows. Move this metadata into a Windows-only artifact/job output and consume that from the packaging job.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yaml around lines 166 - 172, The Windows Velopack metadata is being exposed through shared matrix-wide build outputs, which can be overwritten by non-Windows legs before sign-and-package-windows consumes them. Move the Velopack fields currently emitted from the build job outputs into a Windows-only artifact or job output path, and update the packaging flow to read those values only from the Windows build. Keep the existing metadata keys associated with the build/packaging steps that produce them, but stop publishing them as shared matrix outputs.
🧹 Nitpick comments (1)
.github/workflows/build.yaml (1)
477-481: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable persisted checkout credentials in the packaging job.
This job only reads the repository, restores tools, and uploads artifacts; keeping the token in checkout config is unnecessary.
Proposed fix
- name: Checkout code uses: actions/checkout@v7 with: submodules: recursive ref: ${{ github.event.pull_request.head.sha || github.sha }} + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yaml around lines 477 - 481, The Checkout code step in the packaging job is persisting repository credentials unnecessarily. Update the actions/checkout usage in this workflow to disable persisted credentials while keeping the existing submodules and ref behavior, so the job can read the repo and upload artifacts without leaving the token in git config.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yaml:
- Around line 29-31: The build job is overriding the workflow-level permissions
and losing repository read access needed by actions/checkout. Update the build
job’s permissions block in the build workflow so the build job explicitly
includes contents: read, matching the existing workflow default behavior.
- Around line 521-522: The workflow step that builds the Velopack package is
calling vpk directly, but tool restore does not guarantee it is on PATH. Update
the packing step in the build job to invoke the restored tool through dotnet
tool run instead of relying on bare vpk, keeping the existing vpk_args usage and
the packaging command in that same section.
---
Duplicate comments:
In @.github/workflows/build.yaml:
- Around line 708-710: The release workflow is downloading the wrong Linux
artifact name; update the `actions/download-artifact` step in the build/release
workflow to request the same build-type suffixed artifact that the build job
publishes, matching the naming used by the Linux build job for open-source
builds. Use the existing artifact naming convention in the workflow so the
download target aligns with the build output instead of hardcoding
`Linux-x64-app`.
- Around line 521-532: The “Build and sign” workflow step builds the installer
with vpk_args but never passes any signing configuration, so update the vpk pack
invocation to include the required signing template and/or signing parameters.
Use the existing vpk_args block in the build workflow to thread the configured
signing values into vpk pack so the Windows installer is actually signed before
publishing.
- Around line 166-172: The Windows Velopack metadata is being exposed through
shared matrix-wide build outputs, which can be overwritten by non-Windows legs
before sign-and-package-windows consumes them. Move the Velopack fields
currently emitted from the build job outputs into a Windows-only artifact or job
output path, and update the packaging flow to read those values only from the
Windows build. Keep the existing metadata keys associated with the
build/packaging steps that produce them, but stop publishing them as shared
matrix outputs.
---
Nitpick comments:
In @.github/workflows/build.yaml:
- Around line 477-481: The Checkout code step in the packaging job is persisting
repository credentials unnecessarily. Update the actions/checkout usage in this
workflow to disable persisted credentials while keeping the existing submodules
and ref behavior, so the job can read the repo and upload artifacts without
leaving the token in git config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aef940c1-f1ba-49f3-a6a8-2c87cbec9797
📒 Files selected for processing (5)
.github/workflows/build.yamldotnet-tools.jsonindra/newview/viewer_manifest.pyindra/vcpkg/ports/velopack/portfile.cmakeindra/vcpkg/ports/velopack/vcpkg.json
✅ Files skipped from review due to trivial changes (3)
- dotnet-tools.json
- indra/vcpkg/ports/velopack/portfile.cmake
- indra/vcpkg/ports/velopack/vcpkg.json
🚧 Files skipped from review as they are similar to previous changes (1)
- indra/newview/viewer_manifest.py
Potential fix for pull request finding 'CodeQL / Workflow does not contain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/build.yaml (1)
708-710: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMirror the OSS/proprietary artifact split for Linux downloads.
For OSS builds, line 415 publishes
Linux-OS-x64-app, so this hard-codedLinux-x64-appdownload fails. This is the same artifact-name contract issue previously flagged, with the OSS suffix still missed here.Proposed fix
- - uses: actions/download-artifact@v8 + - uses: actions/download-artifact@v8 + if: needs.setup.outputs.build_type == 'proprietary' with: name: "Linux-x64-app" + + - uses: actions/download-artifact@v8 + if: needs.setup.outputs.build_type != 'proprietary' + with: + name: "Linux-OS-x64-app"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yaml around lines 708 - 710, The Linux artifact download is using the proprietary artifact name and will fail for OSS builds. Update the download step in the workflow to follow the same OSS/proprietary naming split used by the upload/publish steps, so the Linux OSS path requests the matching Linux-OS-x64-app artifact instead of the hard-coded Linux-x64-app name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yaml:
- Around line 477-481: The Checkout code step in the packaging job is leaving
persisted credentials behind even though this job does not need to push changes.
Update the actions/checkout configuration used there to disable persisted
credentials while keeping the existing submodules and ref behavior intact, so
the workflow does not store the token in Git config.
- Around line 566-582: The Velopack packaging step can silently continue with an
empty installer path, so make the package job fail immediately at the output
boundary. In the Find Velopack installer step, validate that the installer
variable is non-empty before writing it to GITHUB_ENV, and ensure the subsequent
actions/upload-artifact steps for Windows-installer and Windows-releases use
if-no-files-found: error so missing files fail the job. Use the existing Find
Velopack installer, Post the installer, and Upload Velopack releases steps to
place the check and tighten the artifact upload behavior.
---
Duplicate comments:
In @.github/workflows/build.yaml:
- Around line 708-710: The Linux artifact download is using the proprietary
artifact name and will fail for OSS builds. Update the download step in the
workflow to follow the same OSS/proprietary naming split used by the
upload/publish steps, so the Linux OSS path requests the matching
Linux-OS-x64-app artifact instead of the hard-coded Linux-x64-app name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2aa7e01d-60ef-414d-9c49-c9f44db25dee
📒 Files selected for processing (5)
.github/workflows/build.yamldotnet-tools.jsonindra/newview/viewer_manifest.pyindra/vcpkg/ports/velopack/portfile.cmakeindra/vcpkg/ports/velopack/vcpkg.json
✅ Files skipped from review due to trivial changes (3)
- dotnet-tools.json
- indra/vcpkg/ports/velopack/vcpkg.json
- indra/vcpkg/ports/velopack/portfile.cmake
🚧 Files skipped from review as they are similar to previous changes (1)
- indra/newview/viewer_manifest.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Fix windows velopack package creation in GHA
Related Issues
Issue Link:
Checklist
Please ensure the following before requesting review:
Additional Notes