Add MacOS and Linux support (UPLOAD-1512) #2
Conversation
…t create fat binary
|
@krystophv I have reviewed this myself, but it would be good to have another set of eyes even if you haven't programmed in Zig yet. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds macOS and Linux support to the uploader helper utility, expanding it from Windows-only to a cross-platform solution for detecting and communicating with LifeScan devices.
Key Changes:
- Implemented platform-specific device discovery for Linux (via
/sys/block) and macOS (via/dev) - Added cross-platform build configuration supporting Windows x64, Linux x64, macOS ARM64, and macOS x64
- Refactored Windows-specific code into conditional compilation blocks with platform detection
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| testing.js | Adds a Node.js test script that detects the OS/architecture and spawns the appropriate helper binary for testing |
| helper.zig | Implements cross-platform device detection and I/O, with platform-specific functions for Linux/macOS alongside refactored Windows code |
| build.zig | Configures multi-target builds for all supported platforms (Windows, Linux, macOS ARM64/x64) |
| README.md | Documents the new testing procedure for different platforms |
| .gitignore | Adds .zig-cache/ to ignored build artifacts |
| .github/workflows/build-helper.yml | Updates CI to build on macOS runner with executables for all platforms, packages them into a zip, and updates Zig to 0.15.2 |
Comments suppressed due to low confidence (1)
helper.zig:613
- Memory leak: When "closeDevice" command is received, the loop breaks without calling
parsed.deinit()first. This will leak the memory allocated for the parsed JSON. Callparsed.deinit()before thebreakstatement.
if (std.mem.eql(u8, data.command, "closeDevice")) {
try sendReply("info", "Closing device");
break;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
helper.zig
Outdated
| var string = std.array_list.Managed(u8).init(allocator); | ||
| defer string.deinit(); | ||
| try string.appendSlice(json_string); |
There was a problem hiding this comment.
The string ArrayList is unnecessary since json_string already contains the serialized JSON. You can directly write json_string to stdout without creating an ArrayList and copying the data. Simply use json_string.len for the length calculation and write json_string directly.
There was a problem hiding this comment.
Make sense, I'll do this.
Co-authored-by: Copilot <[email protected]>
|
@krystophv Ready for re-review |
See UPLOAD-1512 for details.