Improve installer (Windows.plugin)#21911
Conversation
github-actions
bot
added
area/ci
area/packaging
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 2/5
- There is a clear merge risk:
CMakeLists.txtcallsconfigure_file()onsrc/collectors/windows.plugin/netdata_driver.inf, but that path is missing, which is likely to break Windows CMake configuration immediately. src/collectors/windows.plugin/netdata_driver.infappears to leave theNetdataWinDriverservice behind on uninstall, creating a concrete cleanup/regression issue for Windows users.- Given the high severity and high confidence of both findings, this is better treated as high risk until addressed.
- Pay close attention to
CMakeLists.txt,src/collectors/windows.plugin/netdata_driver.inf- missing INF input breaks configuration, and uninstall behavior is incomplete for service removal.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CMakeLists.txt">
<violation number="1" location="CMakeLists.txt:3072">
P1: `configure_file()` references `src/collectors/windows.plugin/netdata_driver.inf`, but that file is not present in the repository, which will break Windows CMake configuration.</violation>
</file>
<file name="src/collectors/windows.plugin/netdata_driver.inf">
<violation number="1" location="src/collectors/windows.plugin/netdata_driver.inf:24">
P1: Uninstall does not remove the `NetdataWinDriver` service that install creates; only the `.sys` file is deleted.</violation>
</file>
Architecture diagram
sequenceDiagram
participant CI as GitHub Actions (CI)
participant MSI as Windows Installer (MSI)
participant SCM as Service Control Manager
participant FS as File System
participant WP as Windows Plugin (Process)
participant K as Windows Kernel
Note over CI, K: Build & Signing Phase
CI->>CI: Build driver and INF
CI->>CI: CHANGED: Sign binaries including .inf
Note over CI, K: Installation / Upgrade Phase
MSI->>MSI: NEW: Execute NDKillProcess
MSI->>SCM: NEW: Stop & Remove old Driver Service
rect rgb(20, 30, 60)
Note right of MSI: File Deployment
MSI->>FS: NEW: Install netdata_driver.sys to %SystemRoot%\System32\drivers
MSI->>FS: NEW: Install netdata_driver.inf to %Windir%\INF
end
Note over CI, K: Runtime Execution Phase
WP->>WP: Initialize collector
WP->>SCM: Open/Start "NetdataDriver" service
alt Load Driver
SCM->>FS: CHANGED: Locate driver at %SystemRoot%\System32\drivers\netdata_driver.sys
SCM->>K: Load Driver Image
K-->>WP: Driver Handles / IOCTL Interface
else Driver Missing/Error
SCM-->>WP: Service Start Failure
WP->>WP: Fallback / Log Error
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/collectors/windows.plugin/GetHardwareInfo.c">
<violation number="1" location="src/collectors/windows.plugin/GetHardwareInfo.c:170">
P2: If the SCM or service reopen fails during the self‑healing path, the function still returns success. Set `ret = -1` (and log) when `OpenSCManagerA` or `OpenServiceA` returns null so callers don’t assume the driver is running.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
This PR updates the Windows build/install flow to improve deployment and lifecycle handling of the Windows kernel driver used by windows.plugin, including adding an INF, adjusting install locations, and improving upgrade/start behavior.
Changes:
- Add a
netdata_driver.infand include it in the Windows build artifacts/signing. - Update
windows.plugindriver handling to use thesystem32\driverspath and self-heal by installing/retrying driver start when missing. - Update the Windows MSI to place the driver in
%SystemRoot%\System32\drivers, place the INF in%WinDir%\INF, and show progress messages during upgrade driver actions.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/collectors/windows.plugin/netdata_driver.inf | New INF intended for driver installation/uninstallation. |
| src/collectors/windows.plugin/netdata_driver.h | New shared header for user-mode driver IOCTL/types. |
| src/collectors/windows.plugin/netdata_driver.c | New kernel driver implementation for MSR reads via IOCTL. |
| src/collectors/windows.plugin/GetHardwareInfo.c | Updated driver service install/start logic and driver path. |
| packaging/windows/netdata.wxs.in | MSI changes to install driver/INF to system folders and show progress text. |
| CMakeLists.txt | Build/package wiring for the renamed driver sources and INF artifact. |
| .github/workflows/build.yml | CI signing updated to include .inf files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [DefaultUninstall.NTamd64] | ||
| LegacyUninstall=1 | ||
| DelFiles = DriverCopyFiles | ||
|
|
There was a problem hiding this comment.
The INF uninstall section only deletes the copied files (DelFiles = DriverCopyFiles) but doesn’t remove the service created by AddService. If the INF is used for install/uninstall, this can leave a stale NetdataDriver service entry behind; consider adding a [DefaultUninstall.NTamd64.Services] section to delete the service (or otherwise ensure service removal is handled reliably).
| [DefaultUninstall.NTamd64.Services] | |
| DelService = NetdataWinDriver |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <File Id="NetdataDrv" Name="netdata_driver.sys" Directory="DRIVERDIR" Source="C:\msys64\opt\netdata\usr\bin\netdata_driver.sys" Condition="NDDRVINST=1"> | ||
| </File> | ||
| <File Id="NetdataDrvInf" Name="netdata_driver.inf" Directory="INFDIR" Source="C:\msys64\opt\netdata\usr\bin\netdata_driver.inf" Condition="NDDRVINST=1"> | ||
| </File> |
There was a problem hiding this comment.
The MSI packages netdata_driver.sys and netdata_driver.inf, but does not package a netdata_driver.cat even though the INF declares CatalogFile=netdata_driver.cat. If you keep catalog-based signing, the installer should also include the .cat in the same driver package directory; otherwise, consider dropping the catalog reference from the INF to avoid broken INF-based installs.
| </File> | |
| </File> | |
| <File Id="NetdataDrvCat" Name="netdata_driver.cat" Directory="INFDIR" Source="C:\msys64\opt\netdata\usr\bin\netdata_driver.cat" Condition="NDDRVINST=1"> | |
| </File> |
| certificate-profile-name: Netdata | ||
| files-folder: ${{ github.workspace }}\build | ||
| files-folder-filter: exe,dll,sys | ||
| files-folder-filter: exe,dll,sys,inf |
There was a problem hiding this comment.
The signing filter was extended to include inf, but driver packages are typically signed via a catalog (.cat), not by signing the .inf itself. If the intent is to support INF-based driver installation, consider generating/including/signing the catalog and adding cat to files-folder-filter (and potentially removing inf if the signing tool doesn’t support it).
| files-folder-filter: exe,dll,sys,inf | |
| files-folder-filter: exe,dll,sys,cat |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <UI> | ||
| <ProgressText Action="NDKillProcess" Message="Stopping Netdata service before update..." /> | ||
| <ProgressText Action="NDStopDriverSrv" Message="Stopping Netdata driver service..." /> | ||
| <ProgressText Action="NDRemoveDriverSrv" Message="Removing Netdata driver service..." /> | ||
| </UI> |
There was a problem hiding this comment.
This adds a second <UI> element without an Id (lines 174–178). In WiX, <UI> is typically defined with an Id and then referenced via UIRef; an anonymous <UI> can fail schema validation or won’t be associated with the UI sequence you use (FeatureTree_ViewLicense_X64). Consider moving these ProgressText entries into the existing <UI Id="FeatureTree_ViewLicense_X64"> block (or give this <UI> an Id and reference it) so the progress text is actually applied and the .wxs stays valid.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
This PR adjusts the CI and Windows installer to install Windows drivers on different Windows versions after we identified an issue in some scenarios.
Test Plan
Additional Information
For users: How does this change affect me?
Summary by cubic
Fixes Windows driver installation and startup: adds an INF, installs files to the correct system folders, and hardens upgrade/start flows. The installer shows clearer progress, and CI now signs the INF too.
Bug Fixes
netdata_driver.systo%SystemRoot%\System32\driversandnetdata_driver.infto%Windir%\INF.Refactors
netdata_driver.*and move driver sources tosrc/collectors/windows.plugin/driverto simplify INF catalog generation.Written for commit 0e14031. Summary will update on new commits.