darwin: show homebrew diff if available #395
Conversation
kiliankoe
force-pushed
the
darwin-brewdiff
branch
from
cacd6de to
e0c9494
Compare
WalkthroughAdds macOS-only Homebrew diff support to the darwin rebuild diff flow. Introduces brewdiff dependency, a new util::print_homebrew_diff function (macOS implementation and non-macOS stub), imports and invokes it alongside the existing Nix diff, and documents the behavior in CHANGELOG. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant D as darwin rebuild
participant UTL as util
participant N as print_dix_diff
participant H as print_homebrew_diff
participant B as brewdiff (macOS)
U->>D: nh darwin --diff switch
D->>N: print_dix_diff(old_profile, new_profile)
N-->>D: nix pkg diff printed
D->>H: print_homebrew_diff(old_profile, new_profile)
alt macOS with Homebrew managed
H->>H: Check brew in PATH and intent
H->>B: Compute Homebrew diff
B-->>H: Changes/no changes
H-->>D: Print stats and diff if changes
else non-macOS or not applicable
H-->>D: No-op / log and return Ok(())
end
D-->>U: Diff output complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed)✅ Passed Checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs. ✨ Finishing Touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
Cargo.toml (1)
30-30: Scope brewdiff to macOS target
brewdiff v0.2.0 exposes only macOS-specific APIs (extract_nix_darwin_intent, spawn_homebrew_diff, write_homebrew_stats, brewdiff::display::write_diff); move it under[target.'cfg(target_os="macos")'.dependencies]to avoid unnecessary builds:[dependencies] anstyle = "1.0.11" -brewdiff = "0.2.0" chrono = "0.4.41" @@ [target.'cfg(target_os="macos")'.dependencies] system-configuration = "0.6.1" +brewdiff = "0.2.0"CHANGELOG.md (1)
19-25: Set user expectations about MAS limitationConsider adding a short note that MAS removals aren't shown (only additions), to match current behavior.
src/util.rs (2)
341-406: Graceful, macOS-gated Homebrew diff looks solidNice separation, error downgrades to debug, and clean output integration.
Optional: align the “no changes” log level with print_dix_diff (info vs debug) for consistency.
408-420: Use the which crate instead of spawningwhichAvoids an external process and simplifies the check.
Apply:
#[cfg(target_os = "macos")] fn homebrew_available() -> bool { - use std::process::Command as StdCommand; - - StdCommand::new("which") - .arg("brew") - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - .map(|s| s.success()) - .unwrap_or(false) + which::which("brew").is_ok() }src/darwin.rs (2)
20-20: Add brewdiff as a macOS-only dependency to avoid cross-platform build costImport looks fine. To keep non-darwin builds lean, ensure the brewdiff crate is only compiled on macOS via target-specific deps in Cargo.toml.
You can gate it like this in Cargo.toml:
+[target.'cfg(target_os = "macos")'.dependencies] +brewdiff = "0.2.0"
145-146: Micro-nit: avoid PathBuf allocation; also consider deduping the repeated “Comparing…” debug lineNo functional issues. Two small tweaks:
- Use Path::new(CURRENT_PROFILE) to avoid an allocation.
- There are two identical debug! lines printing the target profile (Lines 131–135 and 140–143). Consider removing one.
Apply for allocation nit:
- let _ = - print_homebrew_diff(&PathBuf::from(CURRENT_PROFILE), &target_profile); + let _ = + print_homebrew_diff(std::path::Path::new(CURRENT_PROFILE), &target_profile);(Optional) For the duplicate debug, you can drop the one inside the else-branch and keep the earlier one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)Cargo.toml(1 hunks)src/darwin.rs(2 hunks)src/util.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/darwin.rs (1)
src/util.rs (2)
print_homebrew_diff(354-406)print_homebrew_diff(424-429)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: treewide-checks
🔇 Additional comments (2)
src/util.rs (2)
33-33: No-op formattingNothing to address.
422-429: Non-macOS stub is correctAPI preserved and no-op behavior is appropriate.
|
AGI is here... |
|
I assumed Coderabbit is an integration directly added to the project? Is that not the case? 🙈 Also, I don't know what is causing or what to do about that one CI failure. It's mentioning a formatting issue in Cargo.toml? But I don't see what it's referring to :/ |
It was made available to the nix-community recently, I'm currently trying it out :) Not very keen on AI, hence the remark.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/darwin.rs (1)
136-147: Tweak debug message to reflect both diffs, not just dix.Minor wording nit to avoid confusion when brew diff is also skipped.
Apply this diff:
- debug!("Not running dix as the --diff flag is set to never."); + debug!("Not running diffs as the --diff flag is set to 'never'.");src/util.rs (3)
353-406: Align error semantics with print_dix_diff (optional).print_dix_diff returns Err on join panic; here we log and return Ok(()). Consider consistent behavior (propagate as Err) for easier upstream handling, even if callers ignore today.
Example:
- let diff_data = match diff_handle.join() { + let diff_data = match diff_handle.join() { Ok(Ok(data)) => data, - Ok(Err(e)) => { - debug!("Failed to compute Homebrew diff: {}", e); - return Ok(()); - }, - Err(_) => { - debug!("Homebrew diff thread panicked"); - return Ok(()); - }, + Ok(Err(e)) => return Err(eyre::eyre!(e)), + Err(_) => return Err(eyre::eyre!("Homebrew diff thread panicked")), };
355-357: Unused old_generation parameter: keep or document why.You intentionally mirror print_dix_diff’s signature; consider noting that old_generation is unused (diff is intent vs current Homebrew state), or remove it in a future breaking change.
408-412: brew detection is fine; consider PATH caveat (optional).Some users install Homebrew but don’t have brew in PATH in non-login shells. If this bites users, consider also checking common locations (/opt/homebrew/bin/brew, /usr/local/bin/brew).
fn homebrew_available() -> bool { - which::which("brew").is_ok() + which::which("brew").is_ok() + || ["/opt/homebrew/bin/brew", "/usr/local/bin/brew"] + .iter() + .any(|p| std::path::Path::new(p).exists()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)Cargo.toml(1 hunks)src/darwin.rs(2 hunks)src/util.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/darwin.rs (1)
src/util.rs (4)
get_hostname(185-215)print_dix_diff(309-339)print_homebrew_diff(354-406)print_homebrew_diff(416-421)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: treewide-checks
- GitHub Check: Test NH on Darwin
🔇 Additional comments (4)
Cargo.toml (1)
61-63: macOS-scoped dependency looks good; please fix Taplo formatting if CI flags it.The cfg-targeted dep is appropriate. If CI complains, run Taplo locally in the repo root to normalize formatting.
Run locally:
- taplo fmt .
src/darwin.rs (1)
145-146: Good integration: Homebrew diff is invoked alongside dix and remains non-fatal.src/util.rs (2)
34-67: LGTM on Nix variant detection.Reasonable defaults and graceful handling when output is missing.
353-406: Robust, gated Homebrew diff with graceful fallbacks.Nice: gated on macOS, checks brew presence, exits quietly when not configured, and prints stats + diff only when changes exist.
| - `nh darwin` now displays Homebrew package changes alongside Nix package | ||
| changes when using nix-darwin configurations with Homebrew management enabled. | ||
| This shows formulae, casks, taps, and Mac App Store apps that will be | ||
| added or removed during a configuration switch. |
There was a problem hiding this comment.
Clarify Mac App Store behavior (removals aren’t shown/uninstalled).
brewdiff/mas won’t uninstall MAS apps; removals aren’t reported. The current wording implies added or removed for MAS as well.
Apply this diff:
- This shows formulae, casks, taps, and Mac App Store apps that will be
- added or removed during a configuration switch.
+ This shows formulae, casks, taps, and Mac App Store apps that will be
+ added during a configuration switch (Mac App Store removals are not
+ uninstalled by `mas` and are therefore not shown).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `nh darwin` now displays Homebrew package changes alongside Nix package | |
| changes when using nix-darwin configurations with Homebrew management enabled. | |
| This shows formulae, casks, taps, and Mac App Store apps that will be | |
| added or removed during a configuration switch. | |
| - `nh darwin` now displays Homebrew package changes alongside Nix package | |
| changes when using nix-darwin configurations with Homebrew management enabled. | |
| This shows formulae, casks, taps, and Mac App Store apps that will be | |
| added during a configuration switch (Mac App Store removals are not | |
| uninstalled by `mas` and are therefore not shown). |
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 21 to 24, the current entry implies Mac App Store
(MAS) apps will be both added and removed when using `nh darwin` with Homebrew
management; however brewdiff/mas does not uninstall MAS apps and does not report
removals. Update the text to explicitly state that MAS apps will be listed when
they will be added but removals are not reported or performed (i.e., MAS apps
are only shown for additions, not removals/uninstalls), and adjust the sentence
to mention formulae, casks, taps, and MAS apps with a parenthetical or clause
clarifying MAS removal behavior.
|
I believe I've integrated most of the sensible AI feedback here, it had some valid points. What's the best way forward here? Is this a feature that could make its way into nh? I'd love for it to exist and believe it has minimal impact on those not on darwin or not using homebrew through nix. I'd love some feedback :) |
|
I meant to give you some feedback earlier, but I'm a little unsure how to proceed as
That said I'm currently looking to hear from @faukah as the developer of dix (the diffing crate we use) to see whether he'd be interested in making Homebrew diffing a native feature so that we could handle the diff at once with uniform formatting. Also in the talks with nix-darwin users that I know to see if they can propose something better, or if this PR is good to go. |
|
That sounds great, thank you! I'm happy to see this go either way, but more input from people more familiar with nix-darwin, package diffing and so on feels very helpful. This was just the most minimal approach that I felt comfortable with on my own, more or less as a proof of concept if the output makes sense. For me it appears to work fine, and I very much enjoy seeing that diff, but any better integration into existing tooling is very welcome. |
|
Hey, author of dix here. I've taken a quick look at brewdiff and have a few notes. I fundamentally wouldn't be against adding something like this to dix, however: brewdiff calls the |
|
Thanks for the suggestion! I'll be on vacation for a few weeks, but will definitely look into reading necessary information directly from an underlying store after. |
3 participants
Hi!
I'm still fairly new to the nix ecosystem and nh as well. I've been using it for a short while now and have come to love its usability and diff outputs after building my config. I also manage homebrew (for GUI apps) through my nix-darwin installation, and have dearly missed seeing a proposed diff from that after building. After investigating a bit, I decided to hack together brewdiff with an API that tries to match dix so that the integration here is straight-forward.
The output can look something like this:
It lists diffs for casks (GUI apps) and formulae (command-line tools and libraries).
It also does Mac App Store diffs which can be configured through nix-darwin's homebrew config, but homebrew doesn't handle this by itself, passing it on to
mas. This unfortunately doesn't uninstall removals automatically, so brewdiff only lists additions here to match that.I hope this matches the rest of the implementation! Cheers!
Sanity Checking
nix fmtto format my Nix codecargo fmtto format my Rust codecargo clippyand fixed any new linter warnings.logic
description.
x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwinAdd a 👍 reaction to pull requests you find important.
Summary by CodeRabbit
New Features
Documentation
Chores