niri: refactor & cleanup #347232
niri: refactor & cleanup #347232
Conversation
Some of these were not required, not explicitly listed, or shouldn't always be added to the rpath
We don't need to be patching an already built binary here
This reorders some of the attributes to be a bit more sensible, as well as adopts some "best practices"
ofborg
bot
added
11.by: package-maintainer
| env = { | ||
| # Force linking with libEGL and libwayland-client | ||
| # so they can be discovered by `dlopen()` | ||
| RUSTFLAGS = toString ( |
There was a problem hiding this comment.
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
| RUSTFLAGS = toString ( | |
| RUSTFLAGS = builtins.concatStringsSep " " ( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I agree with @sodiboo on this one.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
toStringof an list shouldn't be unintuitive, that's howbuildInputs,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
toStringto 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.
There was a problem hiding this comment.
@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 ?
getchoo
force-pushed
the
pkgs/niri/refactor
branch
from
d0ca21a
to
9b12b98
Compare
isn't this an incorrect label the bot added? getchoo wasn't a maintainer for this package (but wants to be with this PR). i thought that label was for when any of the already-listed maintainers (for example, me) opened a PR? |
|
It happens whenever someone is added as a maintainer, as OfBorg doesn't really consider past meta-attributes. You can see this in new packages for example |
There was a problem hiding this comment.
since ofborg requested me again i feel inclined to leave another review comment but uh i don't really have anything new to say other than i still want this changed: #347232 (comment)
wegank
added
the
12.approvals: 1
|
@sodiboo if you approve this PR, I'll merge it. Feel free to make a follow-up PR to change |
getchoo
added
12.approvals: 2
|
Sorry about not responding lol, it looks good, glad it was merged |
Basically a copy and paste of YaLTeR/niri#687
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.