Skip to content
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.

Sign up for GitHub

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

niri: refactor & cleanup #347232

Merged
merged 7 commits into from
Oct 12, 2024
Merged

niri: refactor & cleanup #347232

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6521bf4
niri: format with nixfmt
getchoo Oct 8, 2024
d9aecbc
niri: cleanup dependencies
getchoo Oct 8, 2024
f1cf61e
niri: force linking with linker args instead of patchelf
getchoo Oct 8, 2024
f03374e
niri: cleanup
getchoo Oct 8, 2024
6a2a508
niri: add `meta.changelog`
getchoo Oct 8, 2024
0c5d185
niri: allow configuration of build features
getchoo Oct 8, 2024
9b12b98
niri: add getchoo as maintainer
getchoo Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 91 additions & 58 deletions pkgs/by-name/ni/niri/package.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,26 @@
{ lib
, rustPlatform
, fetchFromGitHub
, nix-update-script
, pkg-config
, libdisplay-info
, libxkbcommon
, pango
, pipewire
, seatd
, stdenv
, wayland
, systemd
, libinput
, mesa
, fontconfig
, libglvnd
, autoPatchelfHook
, clang
{
lib,
clang,
dbus,
eudev,
fetchFromGitHub,
libdisplay-info,
libglvnd,
libinput,
libxkbcommon,
mesa,
nix-update-script,
pango,
pipewire,
pkg-config,
rustPlatform,
seatd,
systemd,
wayland,
withDbus ? true,
withDinit ? false,
withScreencastSupport ? true,
withSystemd ? true,
}:

rustPlatform.buildRustPackage rec {
Expand All @@ -26,10 +30,16 @@ rustPlatform.buildRustPackage rec {
src = fetchFromGitHub {
owner = "YaLTeR";
repo = "niri";
rev = "v${version}";
rev = "refs/tags/v${version}";
hash = "sha256-4YDrKMwXGVOBkeaISbxqf24rLuHvO98TnqxWYfgiSeg=";
};

postPatch = ''
patchShebangs resources/niri-session
substituteInPlace resources/niri.service \
--replace-fail '/usr/bin' "$out/bin"
'';

cargoLock = {
lockFile = ./Cargo.lock;
outputHashes = {
Expand All @@ -38,56 +48,79 @@ rustPlatform.buildRustPackage rec {
};
};

strictDeps = true;

nativeBuildInputs = [
clang
pkg-config
rustPlatform.bindgenHook
autoPatchelfHook
clang
];

buildInputs = [
wayland
systemd # For libudev
seatd # For libseat
libdisplay-info
libxkbcommon
libinput
mesa # For libgbm
fontconfig
stdenv.cc.cc.lib
pipewire
pango
];

runtimeDependencies = [
wayland
mesa
libglvnd # For libEGL
];
buildInputs =
[
libdisplay-info
libglvnd # For libEGL
libinput
libxkbcommon
mesa # For libgbm
pango
seatd
wayland # For libwayland-client
]
++ lib.optional (withDbus || withScreencastSupport || withSystemd) dbus
++ lib.optional withScreencastSupport pipewire
++ lib.optional withSystemd systemd # Includes libudev
++ lib.optional (!withSystemd) eudev; # Use an alternative libudev implementation when building w/o systemd

passthru.providedSessions = [ "niri" ];
buildFeatures =
lib.optional withDbus "dbus"
++ lib.optional withDinit "dinit"
++ lib.optional withScreencastSupport "xdp-gnome-screencast"
++ lib.optional withSystemd "systemd";
buildNoDefaultFeatures = true;

postPatch = ''
patchShebangs ./resources/niri-session
substituteInPlace ./resources/niri.service \
--replace-fail '/usr/bin' "$out/bin"
'';
postInstall =
''
install -Dm0644 resources/niri.desktop -t $out/share/wayland-sessions
''
+ lib.optionalString withDbus ''
install -Dm0644 resources/niri-portals.conf -t $out/share/xdg-desktop-portal
''
+ lib.optionalString withSystemd ''
install -Dm0755 resources/niri-session -t $out/bin
install -Dm0644 resources/niri{-shutdown.target,.service} -t $out/share/systemd/user
'';

postInstall = ''
install -Dm0755 ./resources/niri-session -t $out/bin
install -Dm0644 resources/niri.desktop -t $out/share/wayland-sessions
install -Dm0644 resources/niri-portals.conf -t $out/share/xdg-desktop-portal
install -Dm0644 resources/niri{-shutdown.target,.service} -t $out/share/systemd/user
'';
env = {
# Force linking with libEGL and libwayland-client
# so they can be discovered by `dlopen()`
RUSTFLAGS = toString (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toString of an array? i actually had to test this in nix repl now because that looks like it shouldn't be allowed. and indeed, toString (["a" "b"]) == "a b". that's... very unintuitive? please no

Suggested change
RUSTFLAGS = toString (
RUSTFLAGS = builtins.concatStringsSep " " (

Copy link
Member Author

@getchoo getchoo Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is well documented. It is also overwhelmingly common in nixpkgs, with 192+ files doing the same thing compared to only 4 following this suggestion (or ~34 if we're more vague with the search). I don't see any reason to reimplement the functionality of toString when this is the case, as that would be a lot more unintuitive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @sodiboo on this one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toString of an list shouldn't be unintuitive, that's how buildInputs, checkInputs.... and others work. interesting that env argument doesn't allow list.

https://github.com/NixOS/nixpkgs/blob/master/pkgs/stdenv/generic/make-derivation.nix#L584
(isString v || isBool v || isInt v || isDerivation v) but since it allows derivation, my guess list should be valid too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the real question probably is, whether we really want to go into this discussion, or whether we can just add the more explicit concatStringsSep and get this PR merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind either way. I am just adding more info in support of @getchoo
eg https://nix.dev/manual/nix/2.24/language/derivations

Lists of the previous types are also allowed. They are simply concatenated, separated by spaces.

https://github.com/NixOS/nixpkgs/tree/master/maintainers#definition-and-role-of-the-maintainer
as existing maintainer, @sodiboo request should be honored, but we can still have a discussion about choices, pros/cons

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or whether we can just add the more explicit concatStringsSep and get this PR merged

I'm a bit confused as to why this would be a blocker? These are functionally identical, and as noted in the contributing guide:

If you find anything related that could be improved but is not immediately required for acceptance [...] please remember not to make such additional considerations a blocker

If anyone believes there are problems with this practice and would like to discuss it, I'd recommend taking the guide's suggestion of opening a tracking issue -- especially with how it seems toString is the default tree-wide

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is well documented.

I don't agree that this makes it okay. One line down you'll also see that false and null map to the same string value, and true -> "1", which i also think is unintuitive along with the quirks of passing a path to it. Language design mistakes happen, Nix has a history of them, and they're hard to meaningfully change or deprecate, but this doesn't mean we should rely on their behaviour and pretend like that's obvious. To be clear, the issue isn't that this is an obscure undocumented quirk of Nix, it's that i don't hesitate to even call it a quirk, because what toString means in this context is not very obvious, and i had to look up what it even means to know what it did here.

[...] reimplement the functionality of toString [...], as that would be a lot more unintuitive

unintuitive? to read builtins.concatStringsSep " " over toString?

it's also not reimplementing anything, it's just... calling a different builtin.

toString of an list shouldn't be unintuitive, that's how buildInputs, checkInputs.... and others work.

No? These take lists. You don't have to call toString on them. If mkDerivation uses toString to concatenate them under the hood that's their problem, but this behaviour of toString is not obvious just from how these other things work, because you aren't expected to concatenate the lists or know what format they should be concatenated in.

I guess the real question probably is, whether we really want to go into this discussion, [...]

lol too late

I'm a bit confused as to why this would be a blocker?

I guess i didn't mean to make this a Blocker™. Like, i don't really care that much and this is a trivial change with zero real effect. I didn't mean to start some discussion about this, because i genuinely thought this was a very unobjectionable suggestion, but it seems i had misjudged the case. Without this suggestion, everything still works and behaves identically, so whatever, go ahead and merge it, the actual package is fine.

If anyone believes there are problems with <the practice of using toString to space-concat lists> and would like to discuss it, [...]

I do think there are problems with it, but honestly i don't care enough to drive much more meaningful discussion about the issue other than the single paragraph i posted at the start of this comment. There's a lot of stuff that bugs me about Nix, and this behaviour of toString is just a tiny drop in that ocean which ultimately matters very little.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sodiboo greetings !

This is well documented.
I don't agree that this makes it okay

this is ok, but should not affect this PR

as it has been pointed multiple times, this is well expected behavior can we agree to proceed with this PR ?

map (arg: "-C link-arg=" + arg) [
"-Wl,--push-state,--no-as-needed"
"-lEGL"
"-lwayland-client"
"-Wl,--pop-state"
]
);
};

passthru.updateScript = nix-update-script { };
passthru = {
providedSessions = [ "niri" ];
updateScript = nix-update-script { };
};

meta = with lib; {
meta = {
description = "Scrollable-tiling Wayland compositor";
homepage = "https://github.com/YaLTeR/niri";
license = licenses.gpl3Only;
maintainers = with maintainers; [ iogamaster foo-dogsquared sodiboo ];
changelog = "https://github.com/YaLTeR/niri/releases/tag/v${version}";
license = lib.licenses.gpl3Only;
maintainers = with lib.maintainers; [
iogamaster
foo-dogsquared
sodiboo
getchoo
];
mainProgram = "niri";
platforms = platforms.linux;
platforms = lib.platforms.linux;
};
}