Conversation
Add a windows-2022 build workflow (x64/ARM64/x86 matrix) with vcpkg NuGet binary caching and a tag-triggered draft release job, replacing the self-hosted AppVeyor worker. Decouple build/Build.cs from AppVeyor: DmfSolution always resolves to the DMF submodule, BuildDmf takes an explicit TargetPlatform parameter instead of reading AppVeyor.Instance, Compile/PublishControlApp stamp versions from the BUILD_VERSION env var, and DownloadAppVeyorArtifacts is replaced by DownloadCiArtifacts using gh run download. appveyor.yml is intentionally left in place until a tagged GitHub Actions run has been verified against a same-commit AppVeyor build. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a GitHub Actions build and release workflow, updates NUKE build and artifact-download targets for GitHub Actions, stamps CI versions, disables CI package generation, and replaces AppVeyor references in schemas, documentation, and setup instructions. ChangesCI migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHubActions
participant BuildJob
participant ArtifactStorage
participant ReleaseJob
Developer->>GitHubActions: Push, tag, pull request, or manual dispatch
GitHubActions->>BuildJob: Run platform matrix
BuildJob->>ArtifactStorage: Upload platform artifacts
ReleaseJob->>ArtifactStorage: Download dshidmini artifacts
ReleaseJob->>GitHubActions: Create draft release with CAB and EXE files
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build/Build.cs (1)
147-184: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass
TargetPlatforminto the solution rebuildCompileis invoked with--target-platformfrom the workflow, but its MSBuild call only setsConfiguration. Sincedshidmini.slndefinesx64,ARM64, andx86solution platforms, this can fall back to the solution default instead of rebuilding the requested architecture.🤖 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 `@build/Build.cs` around lines 147 - 184, Update the Compile target’s MSBuildSettings construction to pass the requested TargetPlatform alongside Configuration when rebuilding the Solution. Use the existing TargetPlatform value from the workflow so the solution selects the corresponding x64, ARM64, or x86 platform instead of its default.
🧹 Nitpick comments (4)
build/Build.cs (3)
26-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
BuildVersion(run ID) andBuildVersionStamp(semantic version) share a confusingly similar name.
BuildVersion(Lines 29-30) is a GitHub Actions run ID consumed only byDownloadCiArtifacts, whileBuildVersionStamp(Line 57) readsBUILD_VERSIONand is an actual semantic version (e.g.3.3.2015.0) used to stamp assemblies. A user could easily pass a version string to-buildversionexpecting it to set the assembly version, or vice versa. Consider renaming the CLI parameter (e.g.RunId) to disambiguate.Also applies to: 57-57
🤖 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 `@build/Build.cs` around lines 26 - 36, Rename the CLI parameter field BuildVersion used by DownloadCiArtifacts to an unambiguous run-ID name such as RunId, and update all references, parameter metadata, and call sites accordingly. Keep BuildVersionStamp unchanged as the semantic assembly version sourced from BUILD_VERSION.
171-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated version-stamp property block.
The same four
SetPropertycalls (Version/AssemblyVersion/FileVersion/InformationalVersion) gated by!string.IsNullOrWhiteSpace(BuildVersionStamp)appear in bothCompileandPublishControlApp. Extract a small helper (e.g. returning the property dictionary, or an extension method applying it to any settings builder) to avoid drift between the two call sites.Also applies to: 209-217
🤖 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 `@build/Build.cs` around lines 171 - 181, The version-stamp logic is duplicated between Compile and PublishControlApp. Extract the shared BuildVersionStamp validation and Version, AssemblyVersion, FileVersion, and InformationalVersion assignments into a helper reusable by both methods, then replace both inline blocks with that helper while preserving the existing behavior for blank stamps.
113-129: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnrecognized
TargetPlatformvalues silently default to x64.Any value other than
"ARM64"/"x86"/"Win32"(e.g. a typo like"X64x") silently falls through toMSBuildTargetPlatform.x64instead of failing loudly, which could mask a CI misconfiguration.🤖 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 `@build/Build.cs` around lines 113 - 129, Update the TargetPlatform handling in the build configuration flow to validate accepted values explicitly: allow ARM64 and x64, continue skipping x86/Win32, and throw an InvalidOperationException for any other value instead of defaulting to MSBuildTargetPlatform.x64. Preserve case-insensitive matching and the existing buildCombinations assignment for valid platforms..github/workflows/build.yml (1)
24-30: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winvcpkg binary cache is writable from
pull_requestbuilds — cache-poisoning risk.
VCPKG_BINARY_SOURCESisreadwriteand the job grantspackages: write(Line 38) for every trigger, includingpull_request(Lines 15-17). A malicious or compromised same-repo branch can push poisoned vcpkg binaries to the shared GitHub Packages feed, which trustedpush/tag builds will then restore and trust.actions/cache@v4(Line 79) is flagged for the same reason by static analysis.Restrict write access to
push/tag builds only, e.g. make the NuGet source mode conditional:🔒 Suggested scoping
env: BUILD_VERSION_OFFSET: 2000 DOTNET_CLI_TELEMETRY_OPTOUT: 'true' DOTNET_NOLOGO: 'true' SCPLIB_ENABLE_TELEMETRY: 'false' - VCPKG_BINARY_SOURCES: 'clear;nuget,https://nuget.pkg.github.com/nefarius/index.json,readwrite' + VCPKG_BINARY_SOURCES: "clear;nuget,https://nuget.pkg.github.com/nefarius/index.json,${{ github.event_name == 'pull_request' && 'read' || 'readwrite' }}"Note
permissions:itself doesn't support expressions, sopackages: writewill still be granted on PR runs; the env-level gate above is the practical mitigation.Also applies to: 36-38, 78-84
🤖 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.yml around lines 24 - 30, Restrict vcpkg binary cache writes to trusted push/tag builds by making VCPKG_BINARY_SOURCES use read-only mode for pull_request runs and readwrite only for push or tag-triggered runs. Update the workflow env configuration and the vcpkg cache step using VCPKG_BINARY_SOURCES so PR builds cannot publish binaries, while preserving read access and existing behavior for trusted builds.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.yml:
- Around line 46-50: Update the actions/checkout@v4 step named “Checkout” to set
persist-credentials to false, while preserving the existing recursive submodule
and Git LFS options.
- Around line 138-149: Update the “Create draft release” step to pass
github.ref_name through a dedicated environment variable, then reference that
variable in the gh release create arguments for both the release identifier and
title. Remove direct github.ref_name interpolation from the run script while
preserving the existing repository, build-version, draft, notes, and artifact
arguments.
---
Outside diff comments:
In `@build/Build.cs`:
- Around line 147-184: Update the Compile target’s MSBuildSettings construction
to pass the requested TargetPlatform alongside Configuration when rebuilding the
Solution. Use the existing TargetPlatform value from the workflow so the
solution selects the corresponding x64, ARM64, or x86 platform instead of its
default.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 24-30: Restrict vcpkg binary cache writes to trusted push/tag
builds by making VCPKG_BINARY_SOURCES use read-only mode for pull_request runs
and readwrite only for push or tag-triggered runs. Update the workflow env
configuration and the vcpkg cache step using VCPKG_BINARY_SOURCES so PR builds
cannot publish binaries, while preserving read access and existing behavior for
trusted builds.
In `@build/Build.cs`:
- Around line 26-36: Rename the CLI parameter field BuildVersion used by
DownloadCiArtifacts to an unambiguous run-ID name such as RunId, and update all
references, parameter metadata, and call sites accordingly. Keep
BuildVersionStamp unchanged as the semantic assembly version sourced from
BUILD_VERSION.
- Around line 171-181: The version-stamp logic is duplicated between Compile and
PublishControlApp. Extract the shared BuildVersionStamp validation and Version,
AssemblyVersion, FileVersion, and InformationalVersion assignments into a helper
reusable by both methods, then replace both inline blocks with that helper while
preserving the existing behavior for blank stamps.
- Around line 113-129: Update the TargetPlatform handling in the build
configuration flow to validate accepted values explicitly: allow ARM64 and x64,
continue skipping x86/Win32, and throw an InvalidOperationException for any
other value instead of defaulting to MSBuildTargetPlatform.x64. Preserve
case-insensitive matching and the existing buildCombinations assignment for
valid platforms.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22b5d15d-0cce-43a4-817a-7d191d6aedc7
📒 Files selected for processing (5)
.github/workflows/build.yml.nuke/build.schema.jsonREADME.mdbuild/Build.cssetup/README.md
msbuild /t:Rebuild over the whole solution does not implicitly restore SDK-style csproj projects, so ipctest, the IPC SDK, and ControlApp failed with NETSDK1004 (missing project.assets.json). Restore build.cmd skips this since Restore only runs as a PublishControlApp dependency. Add back the explicit dotnet restore calls that appveyor.yml used to run in before_build. Also stop using GITHUB_TOKEN for the vcpkg NuGet binary cache: personal GitHub accounts cannot push new packages with it (only update existing ones), so every push silently failed. Gate the cache on an optional VCPKG_NUGET_TOKEN secret (a PAT with read/write:packages) instead, falling back to no remote cache when it is not configured. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.yml:
- Around line 69-83: Replace the direct secrets reference in the step’s if
condition with an output from a preceding step. Add a prior step that checks
whether VCPKG_NUGET_TOKEN is present and writes a boolean step output, then gate
the PowerShell token configuration step on that output while preserving its
existing environment and commands.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b77756b4-af6f-4717-9c08-32bc1f9dcf74
📒 Files selected for processing (1)
.github/workflows/build.yml
GitHub Actions rejects `secrets.X` inside if: expressions outright (Unrecognized named-value: secrets), only env: and with: can reference them. Promote VCPKG_NUGET_TOKEN to a job-level env var and check env.VCPKG_NUGET_TOKEN in the step condition instead. Co-authored-by: Cursor <cursoragent@cursor.com>
- Set persist-credentials: false on checkout; nothing in this job needs to push back to the repo with the checkout-persisted token. - Fix expression injection: route github.ref_name through an env var instead of interpolating it directly into the release gh CLI script. - Skip GeneratePackageOnBuild in CI (Directory.Build.props): it triggers NuGet packing as a side effect of building every csproj, including Nefarius.DsHidMini.IPC (net8.0-windows), which hits a known NU1012 packing bug for Windows-specific TFMs on the unpinned/newer .NET SDK this runner resolves. Nothing consumes the CI-built .nupkg; it is a local/dev convenience only. Co-authored-by: Cursor <cursoragent@cursor.com>
…form Compile never passed an explicit Platform to MSBuild. On AppVeyor this was masked because its "platform:" matrix axis set a $env:PLATFORM variable that MSBuild picked up implicitly; GitHub Actions has no such variable, so MSBuild fell back to "Any CPU", which dshidmini.sln maps to a Release|x64 driver build on every leg (including x86), linking against a DMF x64 lib that BuildDmf never produces for the x86 leg. Pass --target-platform through to MSBuild explicitly so each leg builds the driver configuration the .sln actually defines for it. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Add a windows-2022 build workflow (x64/ARM64/x86 matrix) with vcpkg NuGet binary caching and a tag-triggered draft release job, replacing the self-hosted AppVeyor worker.
Decouple build/Build.cs from AppVeyor: DmfSolution always resolves to the DMF submodule, BuildDmf takes an explicit TargetPlatform parameter instead of reading AppVeyor.Instance, Compile/PublishControlApp stamp versions from the BUILD_VERSION env var, and DownloadAppVeyorArtifacts is replaced by DownloadCiArtifacts using gh run download.
appveyor.yml is intentionally left in place until a tagged GitHub Actions run has been verified against a same-commit AppVeyor build.
Summary by CodeRabbit