-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
nixos/niri: init module #348193
nixos/niri: init module #348193
Conversation
cc @IogaMaster @foo-dogsquared (niri package maintainers) |
As it stands, a lot of niri users depend on my Because this should be coordinated with niri-flake (which i maintain), do not merge this PR without my approval. I will approve this PR eventually, once it (a) has relevant concerns addressed which are limited in scope to nixpkgs (i.e. normal review), and (b) it can be merged smoothly without breaking everything for niri-flake users. in particular, this module must be updated such that it doesn't cause build failures when this pull request is merged. It's also worth noting that it's exam season! Which means i might take a while to respond. I'm busy with a lot of schoolwork. Still, I'm gonna try to make sure this PR and the related niri-flake changes are all done by the end of the month, so we can make it into |
We can have a The only concern with this is potentially allowing "at the very least niri-flake could just define a separate |
be9ae07
to
01143a8
Compare
01143a8
to
c6513fd
Compare
…this completely transparently" This reverts commit a242626. apparently that's *not* the correct directory? NixOS/nixpkgs#348368 NixOS/nixpkgs#348193 (comment)
Hi @sodiboo. Could you include a You are then free to either keep on maintaining your flake, or deprecate it and redirect people to the new module introduced here. |
Oh, i did not know I'll definitely just be adding that, and then most of my concerns about compatibility no longer apply. for instance, everything I said about the hacks with providing a config option can just go out the window because no such hacks are necessary. most of the concerns about polkit being hacky and maybe breaking systems in subtle ways no longer apply because this change will be opt in, so at the very least it won't happen just from a nixpkgs update. there remains the question of whether it is fitting to include, on the basis that no other window manage, but there's no need to be paranoid about it subtly causing issues because it's opt in. niri-flake's primary NixOS module will probably be eventually deprecated in favor of this module, but in general niri-flake won't be deprecated because it also does a bunch of other stuff. still, even though most of the "paranoia"-like concerns are nullified, do not merge this without my approval so I can verify that niri-flake can still work smoothly with it and there's a clear upgrade path (in general, providing a clean way to add the rest of its capabilities but use this NixOS module to actually install the package and such) |
@sodiboo could you just add the |
avoid nasty conflicts when NixOS/nixpkgs#348193 is merged; also avoid any subtle differences that users haven't opted into
c6513fd
to
a8f4635
Compare
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.
as stated previously, a lot of my initial concerns were somewhat paranoid and based on just being afraid that this will silently cause subtle differences that lead to difficult-to-diagnose issues, and now that i am aware of a wonderful mechanism named disabledModules
(sodiboo/niri-flake@48bf49e), i'm no longer afraid. that being said, i do still have some comments on a couple things about this PR more generally
a8f4635
to
af8f1cf
Compare
af8f1cf
to
484658a
Compare
@sodiboo, are we good to go here? |
once this thread is resolved (#348193 (comment)), i think we'll be good. i'll approve it then. |
484658a
to
794c7c6
Compare
794c7c6
to
942b12c
Compare
@sodiboo @getchoo, I think we're good to merge now, right? I've been using this on my laptop for a couple of days now and everything seems to be working fine. @getchoo I see that you decided to remove the polkit agent. I don't have a strong opinion either way, but I kind of liked the "batteries-included" idea with having the agent included by default and offering an option to disable it. I'm not familiar with how modules for other compositors deal with this though. The DEs definitely have this stuff included by default. I don't think this is a blocker in any case, and I'm sorry about all the back-and-forth that this option has already caused, but I wouldn't mind personally if you'd re-add that part. |
Yeah, good to merge (that's what "approved" means, no?). I would also approve if the polkit agent stayed; either is fine. |
Yeah, I started writing that comment just before you approved. |
ah, what a timing coincidence. oddly enough not the first time that happened for niri in nixpkgs. |
I liked it too, but with the drop-in method I found, it's a lot easier now to do it correctly yourself
EDIT: It's way easier than I thought Most agents (including |
This adds a basic module for setting up Niri and its related services/dependencies. Options for
config.kdl
should be made after #295211I tested this with the following locally and it worked well :)
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.