refactor: nix flake for build and dev - #2219
Conversation
|
Hi @karlskewes! I've been working on similar improvements and have some suggestions for this flake based on the nixpkgs cosmic-comp package: Improvements I'd suggest:
I've implemented these changes in my fork: https://github.com/MasterofNull/cosmic-comp/tree/nix-flake Feel free to cherry-pick any of these improvements, or I can submit them as suggestions/PR to your branch if that would help! |
Looks great, thank you. I cherry-picked the commit.
Across the Cosmic repo's the flakes are all a little different and there are various PR's. I think basic criteria would be something like:
|
|
@karlskewes Thanks for cherry-picking the changes! I've pushed a follow-up fix for the double-slash issue you mentioned: Fix: 85587d2e - Remove double slashes from cosmic-protocols patch URLs The - cosmic-protocols = { git = "https://github.com/pop-os//cosmic-protocols", branch = "main" }
+ cosmic-protocols = { git = "https://github.com/pop-os/cosmic-protocols", branch = "main" }Feel free to cherry-pick this as well. The |
|
Is there any particular reason we're not using Anyway, it would be cool if this got merged, since the current flake is broken :). I was just about to make a PR updating it, but seeing this makes that redundant :D. |
@Elias-Graf, I've updated the PR description based on another comment. |
f7c5a71 to
a898d96
Compare
|
I've rebased this branch on I'll rebase on master if/when there's appetite to merge. Additionally have removed the redundant patch for |
8c45cc2 to
7ce73ef
Compare
ee242e4 to
78fa20c
Compare
|
@karlskewes We can take a look at reviewing this to merge and closing the obsolete #1374 if you want to rebase this on master and remove any extraneous changes. @nuwawa @faukah Feel free to provide any input. We'll assume you don't object to these changes if we don't hear back. |
4a60a72 to
4832463
Compare
| [patch."https://github.com/pop-os/cosmic-protocols"] | ||
| cosmic-protocols = { git = "https://github.com/pop-os//cosmic-protocols", branch = "main" } | ||
|
|
There was a problem hiding this comment.
The patch seems unnecessary and breaks nix build due to // or duplicate source if remove //.
Set branch = "main" in further up on L24 and update Cargo.lock as well.
This commit could be a separate PR if preferred.
Thanks @jacobgkau, rebased on master and ready for review. |
Improvements to the nix flake for building cosmic-comp: - Add inputs.nixpkgs.follows to rust-overlay to reduce duplication - Use explicit github: URL format for nixpkgs - Add missing udev to buildInputs (required for device access) - Fix xrdb path (xorg.xrdb instead of bare xrdb) - Use pname/version instead of just name (follows nixpkgs convention) - Add separateDebugInfo for better debugging experience - Add makeFlags and dontCargoInstall (matches nixpkgs package) - Add meta attribute with license, homepage, platforms, mainProgram - Add formatter output for 'nix fmt' support - Add overlays.default for easy integration with other flakes - Improve devShell with rust-analyzer, cargo-watch, and shellHook - Remove redundant rustToolchain from buildInputs (already in toolchain) - Clean up formatting and remove unnecessary parentheses These changes align the flake more closely with the nixpkgs cosmic-comp package while maintaining the development-focused nature of this flake.
- use longer variable name for clarity - use updated package name for `xrdb` - include Makefile and data (*.ron files) for install - drop cargo-watch `cargo-watch` is an optional developer tool not required to run or develop cosmic-comp. Additionally it is no longer maintained.
There's only one reference to cosmic-protocols so the patch is unnecessary and breaks `nix build` and other tooling. Error below, GitHub's git server rejects `//` in URL's. ``` $ nix build warning: Git tree '/home/karl/src/github.com/karlskewes/cosmic-comp' is dirty fatal: remote error: pop-os//cosmic-protocols is not a valid repository name Visit https://support.github.com/ for help warning: could not read HEAD ref from repo at 'https://github.com/pop-os//cosmic-protocols', u 'master' fatal: remote error: pop-os//cosmic-protocols is not a valid repository name Visit https://support.github.com/ for help ```
jacobgkau
left a comment
There was a problem hiding this comment.
Before, nix build, nix shell, and nix develop all failed with an error. On this branch, they all complete without error. So the Nix flake update seems like an improvement to me.
I also tested this through our normal build process and am not seeing any side effects from dropping the cosmic-protocols patch (but the engineering team can chime in if there's a reason that needs to still be in place).
Across the Cosmic repo's the flakes are all a little different and there are various PR's (e.g: #1993 #1374) .
If the maintainers are happy to have the files, then I think it would be great to align them.
I think basic criteria should be something like:
baconor similarnix buildsupportnix developsupportOpen to suggestions and alternatives.
AI Disclosure