-
Notifications
You must be signed in to change notification settings - Fork 8
packaging: fix build with lowdown #313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
packaging: fix build with lowdown #313
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packaging/dependencies.nix (1)
43-43: Consider adding a comment explaining why patches is cleared.The explicit
patches = []is crucial to preventing the incompatible nixpkgs patch from being applied. Consider adding a brief inline comment explaining this, similar to the detailed comments for other overrides in this file (e.g., the boehmgc section).📝 Suggested comment addition
- patches = [ ]; + # Clear patches to avoid incompatible nixpkgs patches (e.g., Cygwin-specific patches for 2.0.4) + patches = [ ];
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packaging/dependencies.nix
🔇 Additional comments (1)
packaging/dependencies.nix (1)
33-48: Thepatches = []addition correctly clears nixpkgs patches when overriding to version 2.0.2.The fix addresses the issue where nixpkgs patches don't apply when upgrading from older lowdown versions to 2.0.2. The conditional logic is sound: if nixpkgs provides lowdown ≥ 2.0.2, use it directly; otherwise, override to 2.0.2 with patches cleared. If a specific nixpkgs version has lowdown with incompatible patches, that's a nixpkgs issue separate from this override.
Since DeterminateSystems/nix-src#313, there is no need to use our fork anymore. Refs: 5210aa3
This PR resolves a build failure caused by a
lowdownpatch fromnixpkgs(NixOS/nixpkgs#468178). The patch, intended forlowdown-2.0.4on Cygwin, is incompatible with thelowdown-2.0.2required by this project.To fix the build failure, the
lowdownpackage is now only overridden if the version provided bynixpkgsis older than2.0.2. When the override occurs, the incompatible patch is not applied.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.