stats: refactor build to data-driven Nix functions - #542371
Conversation
|
kinnrai
left a comment
There was a problem hiding this comment.
Built and tested on aarch64-darwin. LGTM.
|
@emilytrau, can you take a look at this when you get a chance? |
ed24ba9 to
b9b3832
Compare
✅
|
|
Could you rebase this onto master once #555873 is merged and drop the redundant version bump? Also, this is partly a review note to myself since the refactor commit was cherry-picked from my earlier PR (lol): I think it'd be better to split it into smaller, independently reviewable commits Right now the module model, build commands, plist generation, asset handling, and install logic all change together, which makes it hard to distinguish structural cleanup from behavior changes |
| "CoreBluetooth" | ||
| ]; | ||
| } | ||
| { name = "Sensors"; } # custom build (ObjC + Swift) |
There was a problem hiding this comment.
Can we fold Sensors into the same module table instead of keeping a second build path?
{
name = "Sensors";
bridgingHeader = "Modules/Sensors/bridge.h";
frameworks = [ "IOKit" ];
libraries = [ "IOReport" ];
objcSource = "reader.m";
}Then the shared builder can compile objcSource when present.
There was a problem hiding this comment.
Addressed in 7c96ad8. Sensors is now a moduleConfigs entry with its bridging header, IOKit, IOReport, and Objective-C source. The shared builder compiles objcSource when present
| ${lib.optionalString (mod ? bridgingHeader) ''-import-objc-header "${mod.bridgingHeader}"''} \ | ||
| -I "$buildDir" -L "$buildDir" \ | ||
| -Xlinker -install_name -Xlinker "@rpath/${mod.name}.framework/${mod.name}" \ | ||
| -lKit ${lib.concatMapStringsSep " " (f: "-framework ${f}") (mod.sysFrameworks or [ ])} \ |
There was a problem hiding this comment.
Can we scope -lIOReport to CPU, GPU, and Sensors? Those are the only modules that use it; NIX_LDFLAGS currently links it into everything.
libraries = [ "IOReport" ];
linkFlags = [ "-lKit" ]
++ map (library: "-l${library}") (module.libraries or [ ]);There was a problem hiding this comment.
Addressed in 2b0756b. The global NIX_LDFLAGS is gone and IOReport is expressed through the per-module libraries field. otool -L on the final bundle shows IOReport only in CPU, GPU, and Sensors.
| --platform macosx \ | ||
| --minimum-deployment-target 14.0 \ | ||
| ${lib.optionalString (appIcon != null) "--app-icon ${appIcon}"} \ | ||
| --output-partial-info-plist /dev/null \ |
There was a problem hiding this comment.
This drops upstream plist keys, including NSAppleEventsUsageDescription. I'd keep the upstream plist as the source of truth and merge the actool output into it:
cp "Stats/Supporting Files/Info.plist" "$appInfo"
# substitute the Xcode build variables here
actool ... --output-partial-info-plist "$assetInfo" ...
${lib.getExe' re-plistbuddy "PlistBuddy"} -c "Merge $assetInfo" "$appInfo"
${lib.getExe' re-plistbuddy "PlistBuddy"} -c 'Delete :SMPrivilegedExecutables' "$appInfo"There was a problem hiding this comment.
Addressed in a2b8526. The build now copies and substitutes upstream's plist, merges actool's partial plist with re-plistbuddy, and removes SMPrivilegedExecutables. The final plist retains NSAppleEventsUsageDescription, reports version 3.0.13/build 835, and has no privileged-helper entry.
| if [ -f "Modules/$mod/config.plist" ]; then | ||
| cp "Modules/$mod/config.plist" "$app/Contents/Frameworks/$mod.framework/Resources/config.plist" | ||
| ${lib.concatMapStrings (name: '' | ||
| if [ -f "Modules/${name}/config.plist" ]; then |
There was a problem hiding this comment.
These configs are force-unwrapped at runtime, so this should fail during the build instead of crashing at launch:
for name in ${lib.escapeShellArgs moduleNames}; do
cp "Modules/$name/config.plist" \
"$app/Contents/Frameworks/$name.framework/Resources/config.plist"
doneThere was a problem hiding this comment.
Addressed in dc807d7. Every module config is now copied unconditionally, so a missing input fails the build. The derivation succeeds and the final app contains all ten module config.plist files.
| done | ||
| '') moduleNames} | ||
|
|
||
| makeWrapper "$app/Contents/MacOS/Stats" "$out/bin/stats" |
There was a problem hiding this comment.
makeWrapper pulls Bash into the closure even though we pass no wrapper args. makeBinaryWrapper keeps the call unchanged and took the closure from 14.0 to 11.2 MiB:
nativeBuildInputs = [ makeBinaryWrapper ];makeWrapper "$app/Contents/MacOS/Stats" "$out/bin/stats"There was a problem hiding this comment.
Addressed in add26be. The input is now makeBinaryWrapper while the existing makeWrapper call is unchanged. $out/bin/stats is a Mach-O wrapper. the runtime closure contains only Stats, LevelDB, and Snappy (no Bash) and measures 11.2 MiB versus 14.0 MiB.
b9b3832 to
32f776c
Compare
|
@4evy Rebased onto master at aab1f6e after #555873 merged. The obsolete version bump and temporary LLD workaround are no longer in the branch. I split the changes into six commits:
Each commit passed formatting, |
|
@ofborg build stats |
✅
|
Refactors the existing Stats 3.0.13 build around a shared, data-driven module configuration. The package attribute, app bundle, and
$out/bin/statsinterface are unchanged.The rewrite addresses the maintainer review by:
Info.plist, merging actool metadata, and removing the unused privileged-helper declaration;config.plistfiles at build time; andmakeBinaryWrapperso Bash is not retained in the runtime closure.Additional verification:
treefmt,git diff --check, andnix-build -A stats --no-out-linkbefore each of the six commits.nixpkgs-review rev HEAD -p stats --systems aarch64-darwin --no-shell: 1 package built (stats).CFBundleShortVersionString = 3.0.13, upstreamCFBundleVersion = 835,NSAppleEventsUsageDescriptionis present, andSMPrivilegedExecutablesis absent.$out/bin/statsis a Mach-O binary wrapper, the runtime closure contains no Bash, and closure size falls from 14.0 MiB to 11.2 MiB.codesign --verify --deep --strict.x86_64-darwinwas not tested locally.Automation disclosure: This contribution, including the code changes and pull request summary, was prepared with assistance from Codex (GPT-5).
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.