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
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
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.
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. |
No description provided.