Build/CI: Add base nix build for cross compiles from linux#26
Build/CI: Add base nix build for cross compiles from linux#26tnichols217 wants to merge 4 commits into
Conversation
zgypa-agent
left a comment
zgypa-agent
left a comment
There was a problem hiding this comment.
model:claude-sonnet-4-6
Review: Missing Description + Bundled Unrelated Change
Step 1 — Tests
The flake.nix introduced in this PR defines no checks output, so nix flake check will not run the unit-test suite. The AGENTS.md-documented test command (dotnet test BFD9010/BFD9010.FhirApi.Tests/...) cannot be verified in a nix-only environment without an explicit checks target. Please add a checks.default or checks.tests output to flake.nix that runs the xUnit suite via dotnet test, or document in the PR description why tests are intentionally excluded from the flake.
Step 2 — PR Description is empty (blocker)
Both the PR body and the corresponding issue #26 are empty. The PR title alone is not sufficient:
- What problem are you solving? Is this enabling CI from a Linux host? Is it for developers without Windows? Is it for reproducible release builds?
- Has
nix buildsucceeded end-to-end (does abfd9010.execome out the other side)? If so, on which system? - Has the generated
bfd9010-serverwrapper (wine launcher) been tested on Linux/macOS? - Why is
BFD9010/BFD9010.Scanner/ScanConfig.csmodified? This change is completely unrelated to the nix build infrastructure (see below).
Step 3 — Unrelated application change bundled in
ScanConfig.cs changes the hardcoded default config file path from the executable directory to ~/Desktop/VidarScans/scan_config.ini. This is a functional behaviour change for every user on every platform and has nothing to do with adding a Nix build.
AGENTS.md states: "For single-file releases, the default base directory is a user temp folder; use --config to keep settings in a stable location." The new hardcoded ~/Desktop/VidarScans/ path contradicts this guidance and will silently create that directory on every startup via Directory.CreateDirectory in GetConfigPath.
Please extract this change into its own PR with its own description and issue reference, or explain in the PR description why it is a prerequisite for the nix build.
No further review will be done until the description is provided and the bundled ScanConfig.cs change is either extracted to a separate PR or clearly justified.
zgypa
left a comment
zgypa
left a comment
There was a problem hiding this comment.
I agree mostly with the agent's review. Please do add any comments or discuss by replying if you don't agree.
| public class ScanConfig | ||
| { | ||
| private const string CONFIG_FILENAME = "scan_config.ini"; | ||
| private const string CONFIG_FILENAME = "~\\Desktop\\VidarScans\\scan_config.ini"; |
There was a problem hiding this comment.
Hmm... why here? Why not inside the package? What if the user deletes that folder? I think you should either keep it there in the package, where it was, or put it in a standard windows location for application configuration files.
Also, the variable name suggests simple file name, not full path.
There was a problem hiding this comment.
that's fair. It's generally bad practice to modify the directory where the application was installed and i considered moving it into %APPDATA%, but i thought it would be more valuable in a visible spot where the users could also see it. If you're scared people will lose it or accidentally delete it then I think %APPDATA% would be the better path, however afaik i believe the configurations on the lab machines are currently the default ones. I will also rename the variable for better clarity
There was a problem hiding this comment.
Additionally, for nix builds, the application directory is read-only (being in the nix-store) so we would not be able to edit that location
There was a problem hiding this comment.
model:claude-sonnet-4-6
The move to %APPDATA% resolves the read-only-store and "standard location" concerns — thanks. Two naming follow-ups before resolving, though.
1. Config dir (this PR, ScanConfig.cs:301). GetConfigPath() writes config to %APPDATA%/VidarScans/, but the app is BFD9010 and the Vidar scanner is one (currently the only) backend — issue #13 is explicitly about modular scanner support, so a future Epson/other backend is in scope for this codebase. The Windows convention for per-app config is %APPDATA%/<AppName>/ (or %APPDATA%/<Vendor>/<AppName>/), i.e. %APPDATA%/BFD9010/ or %APPDATA%/open-ortho/BFD9010/.
2. Default output dir (pre-existing, ScanConfig.cs:38). OutputPath defaults to ~\Desktop\VidarScans. Same forward-compat issue, and naming the config dir VidarScans too would create two semantically different "VidarScans" folders. While you're touching the path logic, please also rename this default to something app-scoped like ~\Desktop\BFD9010Scans so both the config dir and the output dir are named after the app rather than the scanner vendor.
Cheap to get right now — once it ships, these directories are created on startup and renaming later means orphaned dirs / migration. Could you switch the config dir to BFD9010 (vendor-prefixed if you prefer) and the default output dir to BFD9010Scans?
1 TestsI will add the dotnet test to the flake, I didn't already do that because it wasn't in the flake prior so I did not change it 2 DescriptionThis PR adds nix building support to the project, allowing for linux users to build the windows executable, and run it within WINE. It additionally is for reproducible builds, ensuring the quality of all built artifacts. The nix build succeeds end-to-end and the program does work in WINE, albeit without testing on a real Vidar scanner yet, but this can be future work in a future PR if changes are necessary. We additionally lock the nuget dependencies with nix to ensure proper nix dependencies for the build. The config file location was changed due to the fact that the nix-store is read only, so the program is not allowed to add configuration files to it once it has been built, additionally, it is bad practice to edit the application directory outside of the installer. 3 Unrelated changesThis was not an unrelated change as it directly contributes to being able to build with nix. |
|
All requested changes have been made, except for keeping the .zed file in there as it is necessary for the editor to work with the cross-compile setup and serves as an example for all developers regardless of which editor they use. Config path directory was moved to APPDATA as well, instead of in the scan directory. The nix build was moved into its own file, and finally, nix checks do not apply this project as you cannot test windows binaries on linux (even though you can build them on linux). My suggested solution to this is to test in a windows VM or set up a complicated wine-wrapped windows test runner, but that is outside the scope of this PR as it didn't exist prior to this PR, so nothing broke since then. |
zgypa-agent
left a comment
zgypa-agent
left a comment
There was a problem hiding this comment.
model:claude-sonnet-4-6
Thanks for the restructure — build extracted to nix/build.nix, config moved to %APPDATA%, and the description in the comments all look good. Three things remain before this is ready to merge.
1. net8.0-windows on the non-GUI projects — was this forced?
This PR flips three projects from net8.0 to net8.0-windows:
BFD9010.Scanner/BFD9010.Scanner.csproj:4BFD9010.FhirApi/BFD9010.FhirApi.csproj:4BFD9010.FhirApi.Tests/BFD9010.FhirApi.Tests.csproj:4
That's what makes the unit tests un-runnable on Linux (and thus the "can't test windows on linux" point). But the tests themselves are cross-platform — FhirApiTests only uses Xunit/Moq/ILogger/Hl7.Fhir and mocks ScannerService; they never load Vscsi32.dll or any Windows API, and they ran on Linux as net8.0 before this PR.
My read is that the bump may be a side effect of the publish step in nix/build.nix, which passes -p:TargetFramework=net8.0-windows as a global property. Global properties propagate across ProjectReference edges, so that flag forces every referenced project to net8.0-windows — and since the GUI csproj already declares net8.0-windows itself, the flag looks redundant. Dropping it would let the GUI build as net8.0-windows while Scanner/FhirApi/Tests stay net8.0 (a Windows GUI can reference base-TFM libraries fine), keeping the tests cross-platform.
Is there a concrete build failure that required pushing all projects to net8.0-windows, rather than just removing that -p:TargetFramework flag? If something fails without it, what's the error? If not, could we keep the Scanner→FhirApi→Tests chain at net8.0 and add a checks.tests output to flake.nix running dotnet test BFD9010/BFD9010.FhirApi.Tests/BFD9010.FhirApi.Tests.csproj? That keeps the existing tests running in the flake/CI on Linux and backs the "quality of all built artifacts" goal from your description.
2. .zed/settings.json should not be tracked
Two issues here. First, it isn't editor-agnostic: the contents are Zed's lsp.roslyn.binary schema, which no other editor can consume — VSCode (OmniSharp/C# Dev Kit) and Vim (coc/nvim-lspconfig) configure the LSP through entirely different files and keys. The portable part is the information (the language server needs RuntimeIdentifier=win-x86, PlatformTarget=x86, TargetPlatformIdentifier=Windows to load the solution for the cross-compile) — that belongs in AGENTS.md or a CONTRIBUTING note where every editor's user can act on it, not in an editor-specific dotfile.
Second, this PR is internally inconsistent about it: the same PR adds .zed/ to .gitignore (commit "Ignore .zed from now out"), yet .zed/settings.json stays committed — it only survives because adding an ignore rule doesn't untrack an already-tracked file. Please git rm --cached .zed/settings.json so the tree matches the .gitignore this PR introduces, and move the LSP env vars into the docs.
3. Config / output directory naming (see inline thread on ScanConfig.cs)
The move to %APPDATA% is good, but the directory is named VidarScans while the app is BFD9010 and Vidar is just the current scanner backend (issue #13 is about modular scanner support). Details and the specific ask are in the inline thread on ScanConfig.cs — in short: rename the config dir to %APPDATA%/BFD9010/ and the default OutputPath from ~\Desktop\VidarScans to ~\Desktop\BFD9010Scans so both are app-scoped rather than vendor-scoped.
3 participants
Uh oh!
There was an error while loading. Please reload this page.