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.

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

flake: drop most external inputs & improve packaging #687

Merged
merged 7 commits into from
Oct 5, 2024

Conversation

getchoo
Copy link
Contributor

@getchoo getchoo commented Sep 27, 2024

The primary goals of this are to make this flake

  • Easier to consume

    • All inputs aside from nixpkgs and (the very small) nix-filter have been removed. This is basically free as the advantages of Crane and Fenix were never really leveraged here
    • Stable Nix is now supported via flake-compat
    • niri is now overridable
    • Important Cargo build features can be set by the user like so (the defaults are unchanged)
    niri.override { withScreencastingSupport = false; }
  • Follow community best practices

    • callPackage is used for the package expression, making it much easier to share code between here, nixpkgs, and anywhere else
    • alejandra has been replaced with nixfmt-rfc-style, the now standard formatter of Nix code
    • autoPatchelfHook is dropped in favor of forcing linkage through ld arguments where needed
    • Meta-attributes are now provided
    • passthru.providedSessions is now provided

See commit messages for more context

Previously, inputs like Crane and Fenix were used to only build the
`niri` package. This isn't really required, and can easily be replaced
by nixpkgs' `rustPlatform` -- which will also lead to less dependencies
being pulled into user's lockfiles
@YaLTeR
Copy link
Owner

YaLTeR commented Sep 28, 2024

cc @sodiboo

@sodiboo
Copy link
Contributor

sodiboo commented Sep 28, 2024

i'll be honest, the flake was kind of a mess, thank you for cleaning it up. I'm not convinced all of this is necessary as this flake is mainly for niri development on NixOS (for end-users, github:sodiboo/niri-flake provides a nicer packaging experience with a binary cache and a guarantee that it won't suddenly break; YaLTeR doesn't use nix and can't easily fix the flake, and as such the CI doesn't mark nix failure as a CI failure for example). However, most of it is definitely an improvement and will help nix-based niri development be less painful, so i'm still happy for it to be merged. I'd also love to improve the niri-flake package in similar ways, and honestly given that this is really nice i might just start importing niri as a flake (though probably after a grace period; at least 1 or 2 new stable releases from now, if ever, to make sure anyone depending on an older branch or fork they're working on don't get a nasty surprise as it fails suddenly)


One of the commit messages says:

Removing some unnecessary dependencies

And I do notice that some of the dependencies removed are... Not Unnecessary. As-is, this pull request breaks cargo run of niri-visual-tests/, which previously did work. I'm not sure how significant that is, since i've never used this crate before (honestly did just now just to test if it was suddenly broken; and it was) and i think it's just used by YaLTeR to test various visual properties of the src/layout/ module and animations and such, but it'd still be quite nice if it didn't break?


Switching from crane to rustPlatform makes a lot of sense, and it'll be nice to more easily share packing code across niri-flake, this repo, and nixpkgs, now that all three use the same build system. However, you say:

the advantages of [...] Fenix were never really leveraged here

I postulate that this was leveraged here. Fenix was used to provide a nightly toolchain in the dev shell, which is not a huge deal (niri isn't a nightly-only package), except for the fact that niri uses Nightly rustfmt.

Why does niri use Nightly rustfmt?

From the niri matrix room, YaLTeR said the following on 2024-03-04 (in response to a github PR comment):

@sodiboo nightly rustfmt is required bc they never stabilize the flags

WIth these changes, the dev shell now provides stable Rust from nixpkgs (currently 1.80.0), which is not the nightly required for our formatting. Moreover, the new dev shell actually doesn't currently include rustfmt at all, stable or not. The previous dev shell environment was fenix's complete toolchain, which has too many items (like, hello? why did we have RLS? MinGW? not my fault) but it included among other things rustfmt and rust-analyzer, which are not part of the new dev shell in this PR. I think fenix is useful/necessary to provide a nightly development toolchain (and to make use of nightly rustfmt; the rest will just be used for their stable features)

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 28, 2024

it'd still be quite nice if it didn't break

Yeah, visual tests should definitely keep working on the development flake.

@tazjin
Copy link
Contributor

tazjin commented Sep 29, 2024

Stable Nix is now supported via flake-compat

I think flake-compat is the wrong way around. There should just be normal stable Nix code (a standard default.nix) and a flake that calls into it.

@sodiboo
Copy link
Contributor

sodiboo commented Sep 29, 2024

Stable Nix is now supported via flake-compat

I think flake-compat is the wrong way around. There should just be normal stable Nix code (a standard default.nix) and a flake that calls into it.

I strongly disagree. I much prefer working with flakes, and if people wish to consume it through flake-compat that's whatever, but it's annoying to design for "stable Nix" first. Flakes are widely adopted and haven't changed in years; they are not officially marked as "stable" but they are stable. It's not some experimental thing that might not go anywhere. We're not going to be flipping the structure of the flake. That doesn't benefit anyone, and is just annoying to work with. flake-compat is however not annoying to work with; it doesn't cause any bother once installed.

@getchoo
Copy link
Contributor Author

getchoo commented Sep 30, 2024

I'd also love to improve the niri-flake package in similar ways, and honestly given that this is really nice i might just start importing niri as a flake

Yeah, this was the original plan here. I had the idea to try out Niri but wanted to clean this up a bit first before pulling it into my personal monorepo, then hopefully get it into nixpkgs. niri-flake was something I came across while doing this, and I wanted to make sure it'd be usable there as well :)

And I do notice that some of the dependencies removed are... Not Unnecessary.

In the context of the package output of this flake, I believe I'm still right. Running a quick test in the basic niri test session didn't show any errors and everything appeared to behave as usual

As-is, this pull request breaks cargo run of niri-visual-tests/, which previously did work.

This I didn't account for though -- as I'm not really familiar with the development of this project and assumed if the niri crate compiled everything would be fine. Looks like it only needs gtk4, libadwaita, and friends, which should only be necessary in the dev shell, as this isn't included in the niri package (like the readme says)

but it'd still be quite nice if it didn't break?

100%!

except for the fact that niri uses Nightly rustfmt.

Another case of me not being very familiar with development here 😆

Fenix will definitely have to come back for this, but I'm not completely sure how. Since this is a (single) development tool, I would rather not have end user's flake.lock get stuck with it. We could use it with rustPlatform very easily, but that would in turn make Fenix a hard requirement in niri-flake (and for consumers of only the package in this flake) even though the niri itself doesn't require nightly anything. The two solutions I habe in mind are:

  • Just overriding rustPlatform with a nightly toolchain and including rustfmt from it in a dev shell
  • or only pulling in the default.rustfmt (nightly rustfmt) from Fenix in the dev shell, while using the nixpkgs toolchain for the package and cargo, rustc, etc in the dev shell

The latter here would allow for consumers of this flake (including niri-flake) to use the same inputs.fenix.follows = "" syntax as I documented with flake-compat in this PR. It could be a bit of a messy solution though, so I'll leave that up to you and then implement either

@getchoo
Copy link
Contributor Author

getchoo commented Sep 30, 2024

I think flake-compat is the wrong way around. There should just be normal stable Nix code (a standard default.nix) and a flake that calls into it.

I can agree this would be nicer (especially since it wouldn't require always downloading upstream's Nix revision and needing to deal with system), but the issue here is that it makes a lot of small things in this Flake kinda tricky to impelement. Some examples include

  • self not being available
    • This would require some weird trickery in the callPackage arguments to account for
    • Stable Nix would have to use the version declared in Cargo.toml, which isn't great for debugging compared to the current short commit SHA (we can't use .git either as that is an impurity)
    • We could no longer use self as a source and would instead need to use ./., which causes some inefficiencies in the flake
  • No external inputs could be used
    • This includes nix-filter
    • Fenix will now be used in the dev shell in both of the options mentioned above. This would mean we couldn't have a dev shell for stable Nix users
    • nixpkgs would have to be imported from its channel path, introducing purity issues for stable Nix users
    • And while yes, we could technically mess around with the flake.lock to reuse the hashes and urls in a stable Nix expression...I don't think that's a good idea and could very easily break with a different source type (i.e., GitLab, srht, etc
  • It would require more Nix files in the tree
    • Not a massive deal, but we would need to split at least the package expression into a new file in order for it to be reused
    • I'm not sure if this is something the maintainers here really want, as this isn't niri-flake after all

In the end, either stable Nix users or flake users will have a lesser experience here. There's no good solution that doesn't hurt either, and for the sake of maintenance (and the fact that literally all users of the Nix code in this repository have been doing so via a flake up until this point), keeping flake.nix as the single "source of truth" and leveraging flake-compat is probably the best option

@sodiboo
Copy link
Contributor

sodiboo commented Sep 30, 2024

And while yes, we could technically mess around with the flake.lock to reuse the hashes and urls in a stable Nix expression...

Isn't this just exactly the main thing that flake-compat does?

@getchoo
Copy link
Contributor Author

getchoo commented Sep 30, 2024

Isn't this just exactly the main thing that flake-compat does?

Yes, but it is quite limited (see the comment at the end) and this functionality isn't exposed at all

@sodiboo
Copy link
Contributor

sodiboo commented Sep 30, 2024

Fenix will definitely have to come back for this, but I'm not completely sure how. Since this is a (single) development tool, I would rather not have end user's flake.lock get stuck with it. We could use it with rustPlatform very easily, but that would in turn make Fenix a hard requirement in niri-flake (and for consumers of only the package in this flake) even though the niri itself doesn't require nightly anything. The two solutions I habe in mind are:

  • Just overriding rustPlatform with a nightly toolchain and including rustfmt from it in a dev shell

  • or only pulling in the default.rustfmt (nightly rustfmt) from Fenix in the dev shell, while using the nixpkgs toolchain for the package and cargo, rustc, etc in the dev shell

The latter here would allow for consumers of this flake (including niri-flake) to use the same inputs.fenix.follows = "" syntax as I documented with flake-compat in this PR. It could be a bit of a messy solution though, so I'll leave that up to you and then implement either

I would rather have the dev shell use all nightly fenix tooling, I don't see why this would mean it's a hard requirement for the end user's flake lock. Can we not just provide a niri package output with a stable rustPlatform, while using an unstable rustPlatform for the dev shell? in any case, i don't actually see making fenix a hard requirement as a huge issue, because at least as of right now, this flake isn't for end users. It's for development, and developers would be using the fenix toolchain anyway. Why is it for developers? see below


Something i forgot to bring up earlier, but this is important:

The way the packages output is structured leads to a common issue of the mesa version differing from the one installed on a NixOS system. The result is that the DRM backend shows a black screen. This can (in a use case where a system configuration depends directly on this flake) probably be mitigated by overriding the nixpkgs output, but is insufficient for e.g. niri-flake which tries to not require input overrides. The way it solves this issue is by encouraging the use of overlays, which allows me to give the user a package built for their system's packageset, sidestepping the issue entirely because mesa doesn't ultimately come from flake inputs. The way I see it, it doesn't actually feel very useful to provide a packages output in this repo (for CI, we can use checks, and for nix run we can use apps). It feels more useful to provide a package that can be called using pkgs.callPackage. I'm kinda brainstorming in these sentences as i'm vaguely distracted as i write this, but the mesa version mismatch is a recurring problem on NixOS, that i feel would get worse if we encouraged users to use the flake in this repo directly with a packages output. It might be worth only providing a package closure that can be called with pkgs.callPackage instead of an actual derivation. If we want that to be usable in the dev shell, we call the package while providing our nightly rustPlatform (and otherwise the dev shell is basically what's present in this PR now?). We also don't have to expose that Nightly-based package in any output. What do you think of this, forcing the consumer to use pkgs.callPackage?

I don't actually know what's the best way around that really. What we've been doing is pretending it doesn't matter, because to my knowledge the primary consumers of this flake are just people working on niri from NixOS. The issue isn't as prevalent then, because development mostly happens through the winit backend, which doesn't have this problem. If i want to try the drm backend, it's generally just a nix flake update to solve the issue too. This "meh" approach to maintenance can definitely be changed for the better, but it's why i'm cautious of pushing this repo's flake to be used by end users. In practice, it doesn't currently provide any guarantees other than "it's probably a very useful starting point to set up a nix based development environment", in that it might be broken at times (in which case it's usually a relatively small change to fix it) and it might be incredibly outdated (like, months old lockfile) in which case you should update the flake lock. In this lens, it's very nice to have a cleaner packaging and dev shell setup (way easier to work with and fix when it breaks), but it's generally not very relevant to think about "the end user" because this flake and its package isn't used by end users. It doesn't meet any relevant quality standard over time. Most end users should just depend on the package in nixpkgs, else niri-flake will likely meet their needs if they want the latest versions (unstable niri; or latest released niri on a stable nixos).

Is that all coherent? Just to be clear, this is just my understanding of the purpose of this repo's flake. I don't mean this in a way as "Thou Shalt Not Make It End User Friendly"; it's just that. i think it already isn't end-user friendly due to the repo management structure (which shouldn't need to change to properly support nix; it should be the other way around that the nix parts can adapt to how niri is structured), and i don't see that changing any time soon. which isn't necessarily a bad thing, but it shapes how i think about this repo's flake and what it "ought to" accommodate and what it "oughtn't" accommodate.

@sodiboo
Copy link
Contributor

sodiboo commented Sep 30, 2024

Is that all coherent?

sidenote: i'm mostly sleep deprived as i wrote all of this. i wasn't like, falling asleep or anything, but i felt very Unfocused while writing it. it just felt weird and difficult to formulate stuff right now. i tried my best to be coherent, though.

@getchoo
Copy link
Contributor Author

getchoo commented Sep 30, 2024

Can we not just provide a niri package output with a stable rustPlatform, while using an unstable rustPlatform for the dev shell?

We can. I was just being a bit lazy with leveraging inputsFrom

in any case, i don't actually see making fenix a hard requirement as a huge issue, because at least as of right now, this flake isn't for end users.

Even if this flake isn't meant for end users, it may be consumed by flakes that are -- like niri-flake. Making Fenix a hard dependency here would in turn make it one there, which is even more troublesome as it's not possible to use the inputs.<input>.follows syntax with nested inputs (unless you use Lix)

The way it solves this issue is by encouraging the use of overlays, which allows me to give the user a package built for their system's packageset, sidestepping the issue entirely because mesa doesn't ultimately come from flake inputs.

I don't think this makes much of a difference? It's the same behavior as overriding the nixpkgs input, but without the advantages of pure attributes like evaluation caching and not creating multiple instances of nixpkgs (this is especially important in flakes meant to be consumed, like niri-flake again). It's fairly easy to re-use a flake's overlay in a package set though, so we can both

that i feel would get worse if we encouraged users to use the flake in this repo directly with a packages output

As long as overriding the nixpkgs input is documented, I don't think it would be that much of an issue. It takes about the same amount of effort as applying an overlay to your instance of nixpkgs after all

What do you think of this, forcing the consumer to use pkgs.callPackage?

This isn't what the flake's packages output is meant for, it will make Nix complain, and will provide a horrible API to end users who do choose to use the packages output as opposed to overlay (which isn't uncommon for the reasons mentioned above). There's a good reason basically no other project does this

sidenote: i'm mostly sleep deprived as i wrote all of this. i wasn't like, falling asleep or anything, but i felt very Unfocused while writing it. it just felt weird and difficult to formulate stuff right now. i tried my best to be coherent, though.

No worries! It was completely fine

Some highlights include:

- Removing some unnecessary dependencies of the package itself
- Allowing for overriding the package
- Adding Cargo feature toggles
- Installing all niri-related resources
- Avoiding `LD_LIBRARY_PATH` hacks
The (debug) package is already set as a check and will still be built
with this, but Nix will now also check other outputs automatically --
such as the dev shell
@getchoo
Copy link
Contributor Author

getchoo commented Oct 1, 2024

niri-visual-tests now builds and runs as expected in the dev shell and the nightly toolchain is back. The only catch here is that rust-overlay had to be used over Fenix as the latter doesn't properly wrap rust-lld

An overlays.default output has been added as well for the reasons mentioned above, while still retaining a packages output

@sodiboo
Copy link
Contributor

sodiboo commented Oct 1, 2024

This isn't what the flake's packages output is meant for, it will make Nix complain, and will provide a horrible API to end users who do choose to use the packages output as opposed to overlay (which isn't uncommon for the reasons mentioned above).

Yes, i know what the packages output is for. I was suggesting we don't provide that output (as I've considered doing for niri-flake; though I haven't done so yet) because it's easy to misuse. I know it would be a weird API to consume, but I feel that's better than an API that is very easy to silently consume in a way that will cause a nonfunctional graphical session eventually, without being obvious at first.

There's a good reason basically no other project does this

Yes, because for basically all other projects, they will either have some obvious failure state, or they will straight up just Work. The only downside in 99% of applications is that it might cause a slightly larger system closure by duplicating deps, but here it poses a larger issue to the operating system's wider usability if niri is the only graphical session.

Also note that I'm not convinced what I am suggesting here is the actual best way to go about it, but I do think you misunderstood what I was suggesting and why, which is why I tried to clarify what I actually meant.


An overlays.default output has been added as well for the reasons mentioned above, while still retaining a packages output

This is nice, although the other reason I suggested to use a pkgs.callPackage-callable output is for use in niri-flake, which provides its own overlay with unique attr names. I suppose this overlay provides the same capabilities, although it's just slightly more annoying to consume in the way niri-flake needs (I can do something like (pkgs.extend niri.overlays.default).niri. Is there a better way to do that? Can I maybe pkgs.callPackage a package that has already been called in that way, to override "every" package input at once?)


niri-visual-tests now builds and runs as expected in the dev shell and the nightly toolchain is back.

Yay! Besides coming to some good consensus on how to deal with That Mesa Issue (and maybe some other bikeshedding matters; this is a Wayland compositor, after all), I do think it would probably be more or less okay to merge this now. Like, to be clear, none of those issues relating to mesa are introduced by this PR, so it may be best to solve at a different time, and submit another PR with the final change to mitigate it best we can. With the dev shell properly working again and supporting the relevant workflow (nightly toolchain, visual tests), this PR is unambiguously just improving the flake now. I still want to give it a proper review later, as I think there might be some other minor tweaks to do.

The only catch here is that rust-overlay had to be used over Fenix as the latter doesn't properly wrap rust-lld

I was going to ask "wait, what? why can't we use Fenix? it worked fine before" but I read the issue you linked and immediately upon seeing 2024-05-18 Nightly i had some vivid flashbacks. and yeah. let's use the one without this issue. is there any other practical consideration? they do the same thing, right? it doesn't ultimately matter, really. specifically Fenix isn't important here, just that the dev shell gets a nightly toolchain. is there a general reason to pick one over the other, though, if both work just fine? the way I see it, this is hardly a "catch"; Fenix was a means to an end, not some dear beloved child that we will mourn.

@YaLTeR
Copy link
Owner

YaLTeR commented Oct 2, 2024

Why is there a new default.nix file btw? I'm not a terribly big fan of adding more of these auxiliary files to the root directory of the repo

@sodiboo
Copy link
Contributor

sodiboo commented Oct 2, 2024

Why is there a new default.nix file btw?

When you import x in Nix where x is a directory, that's automatically rewritten to import "${x}/default.nix".

A lot of the Nix ecosystem uses flakes as their primary dependency management solution, but flakes are technically not "stable". They are not enabled by default on new Nix installations, and require manually setting a feature flag before you're allowed to make use of them. This repo has previously pretended flakes are always available, which in practice they almost always are because flakes aren't actually unstable, their "experimental" label is mostly a result of bureaucracy and bikeshedding.

flake-compat bridges this gap by polyfilling flake loading functionality for stable Nix. The primary thing flake-compat does is parse your lockfile and automatically fetch all the inputs, and call them appropriately, before calling your flake. This allows stable Nix consumers to import your flake as well.

The default.nix present in this PR handles calling flake-compat for you, such that a stable Nix consumer of this repo can write an expression like import (builtins.fetchGit "https://github.com/YaLTeR/niri"); this currently fails with error: opening file '/nix/store/<hash>-source/default.nix': No such file or directory, but with this PR, it would succeed and work properly and feel similar to adding niri to your flake inputs.

It's also worth noting that this default.nix is only a convenience for the consumer. This is how flake-compat is meant to be consumed, but without the default.nix in this repo, our consumers could with little friction do the seemingly-equivalent (although technically different) expression (import (builtins.fetchGit "https://github.com/edolstra/flake-compat") { src = (builtins.fetchGit "https://github.com/YaLTeR/niri"); }).defaultNix to achieve the same result. That snippet works right now, and can successfully import the flake in this repo as-is.

I'm not a terribly big fan of adding more of these auxiliary files to the root directory of the repo

I'm also not a fan of adding such files to the repo root, but from a Nix point of view it does make sense, which is why i didn't at all comment on it. However, it's not clear to me that there's... literally anyone... who actually needs this functionality, and adding a default.nix to our repo isn't actually required for them to consume it (this only makes it slightly more convenient for such a consumer). I'm not opposed to adding this file as it only makes things more convenient, but i'm not gonna pretend like it's some huge important deal if it's not there either.

@YaLTeR
Copy link
Owner

YaLTeR commented Oct 2, 2024

Thanks for the explanation. Let's remove default.nix since it's not important enough.

@getchoo
Copy link
Contributor Author

getchoo commented Oct 2, 2024

I was suggesting we don't provide that output (as I've considered doing for niri-flake; though I haven't done so yet) because it's easy to misuse.

Overlays aren't exactly hard to misuse either. For example, you said it could be consumed with (pkgs.extend niri.overlays.default).niri; this re-evaluates all of nixpkgs and adds at least 100mb of memory usage to the total evaluation. I strongly believe this is more a documentation issue than anything else

This is nice, although the other reason I suggested to use a pkgs.callPackage-callable output is for use in niri-flake, which provides its own overlay with unique attr names.

I would strongly recommend against implementing an API for this reason. niri-flake should not be considered as the only consumer, and this dogfooding makes very little sense in any other context

Is there a better way to do that? Can I maybe pkgs.callPackage a package that has already been called in that way, to override "every" package input at once?)

Unless you can guarantee the previous overlay is applied before yours, no. If you can, this will happen automatically and further overrides can be accessed with prev.niri

I actually think this is a good argument for adopting the packages output over overlays, though. Overlays are very poor at guaranteeing basically anything about what is contained in previous extensions to nixpkgs, like in this case. A much more robust solution here would be to override the nixpkgs input of this flake in niri-flake, use that (and whatever modifications you make to it) in the module, and advise users to override the root nixpkgs input of niri-flake. This would simplify things for other consumers of this flake, (probably) streamline how things are handled in niri-flake, and make little difference to users as they would only need to replace

{
  nixpkgs.overlays = [ inputs.niri.overlays.niri ];
}

in their configuration with

{
  inputs.nixpkgs.follows = "nixpkgs";
}

in flake.nix

This definitley more of a niri-flake conversation though, so I'll end it here 😆

is there any other practical consideration?

rust-overlay's tarball is a fair bit bigger since they archive many more versions of the nightly toolchain. The flake interface is also technically not stable, but (similar to flakes themselves) there haven't been any major breakages to my knowledge

they do the same thing, right?

So yeah, pretty much lol

This allows `niri-visual-tests` to still be built and run in the dev
shell where it's necessary, as well as brings back the nightly `rustfmt`
used by the project

We can't use `fenix` again though as it doesn't wrap `ld` like nixpkgs
and rust-overlay do; without it, the way we link `dlopen()`'d libraries
breaks
@YaLTeR YaLTeR merged commit 1b44e0c into YaLTeR:main Oct 5, 2024
10 checks passed
@YaLTeR
Copy link
Owner

YaLTeR commented Oct 5, 2024

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants