Remove Nix dependency: flake-utils + fixups

[toastal]

This dependency isn’t useful since it is just a loop, doesn’t capture all supported systems, & isn’t worth propagating it to downstream users (currently it still is required for Nickel, but that is on them). Related #613.

Also caught a typo.

Small refactors:

• using inputs as an attrset to more easily track dependencies (allows removing *-input from one of the inputs)
• expose overlays
• attrset “caches” for nixpkgs, topiaryPkgs, binPkgs
• use self.packages where possible
• don’t calculate checksLight for all systems

Normally removing flake-utils the diff results in less line of code, but refactors added a little bit.

Note: reviewing without whitespace changes makes this easier to review (nesting level changed)

¹

Footnotes

1. Please consider giving up MS GitHub or offering a non-proprietary, non-US-corporate-controlled mirror for this free software project. I wish to delete this Microsoft account in the future, but I need more projects like this to support alternative methods to send patches & contribute. ↩

[Niols]

The two first commits definitely bring some nice improvements to the flake; thank you for catching the typo! As for the third one, removing flake-utils and replacing it by repeated calls to nixpkgs.lib.genAttrs, I am much less convinced by the motivation and the gain in usability and readability, but it does seem clean to me.

[toastal]

As for the third one, removing flake-utils and replacing it by repeated calls to nixpkgs.lib.genAttrs, I am much less convinced by the motivation and the gain in usability and readability, but it does seem clean to me.

The motivation is in #613. Tweag isn’t maintaining Topiary its package in upstream Nixpkgs, which is still on 0.3.x. I would like to use the latest version which seems easiest as a flake, but there are more dependencies than needed which pollute my own flake which I now need to audit for security. Currently, Nickel still has flake-utils as a dependency so it isn’t eliminated, but if I have to do a merge request along that, I would—which would fully eliminate flake-utils + 19 lines from the lockfile. I have spoken with Numtide folks in the past & they agree that it is never worth it to include their library just for the loop… its purpose is to handle Flake environments where you are not reliant on Nixpkgs or its lib, which this project very much already is (for example, having a flake.nix but not using Flakes to lock the nixpkgs version otherwise you have to maintain 2 Nixpkgs by Flake’s design & that lib isn’t in a separate repository). There are repeated calls to genAttrs to build attrsets per architecture that can be reused without calling & evaluating more loops or imports than is necessary. Mind us that Nix is evaluated lazily.

[toastal]

If topiaryPkgs & binPkgs were refactored a bit differently this could be smaller/more concise, but I was trying to get the diff as minimal as possible.

With regards to readability:
To view that specific commit without white space: 560b767?w=1
To view a full file: https://github.com/toastal/topiary/blob/remove-flake-utils/flake.nix

Subjectively I think this reads almost identical.

[toastal]

Actually I think I can squeeze one smaller commit in the middle here to break it down more. One sec.

✔️ self.packages.${system}.topiary-cli eliminated another function call.

[toastal]

For more motivation: https://ayats.org/blog/no-flake-utils/

[toastal]

It could make more sense to move to pkgsFor & just do that genAttrs once which holds { pkgs = …; topiaryPkgs = …; binPkgs = …; }, since it doesn’t seem that we need a one-off nixpkgsFor.

Yes, this is few lines of code & less loops.

[toastal]

Rebased & resolved conflicts

[toastal]

Rerebased & reresolved conflicts

[yannham]

LGTM. I don't know how the Topiary people feel about the flake-utils part - I personally don't have any strong opinion. However, in practice this part of the PR feels more guided by taste and doesn't have much impact currently, as flake-utils is still part of the inputs transitively and it's not sure that we'll get rid of it in Nickel.

@toastal: would that be ok to take out the last commit as a separate PR, where we can discuss the flake-utils question independently? I think the first three commits of this PR are uncontroversial and ready to merge.

[toastal]

Not sure I understand why this would be in any shape controversial. We all remember leftpad or see these Python & npm supply chain attacks surface for not wanting to write 5 lines of code. Normalize adding a couple of line over adding an entire dependency. -- toastal ไข่ดาว | https://toast.al PGP: 7944 74b7 d236 dab9 c9ef e7f9 5cce 6f14 66d4 7c9e

[Niols]

I agree with @yannham to say that the first three commit seem uncontroversial when the last one is a matter of taste (or potentially threat model). I would be up for merging the three ones today and spend a bit of time deciding what to do with the last one.

We all remember leftpad or see these Python & npm supply chain attacks surface for not wanting to write 5 lines of code. Normalize adding a couple of line over adding an entire dependency.

This is a very one-sided story. Sure we do remember or see those, but we also know of all the bugs introduced by people deciding to reinvent the wheel (even only for 5 lines). I am not saying that this is the case here — the change looks sound to me — but it is more of a personal choice and therefore I do think that the Topiary team should make the call.

[toastal]

A for loop vs. map falls more into a “taste” camp than to add a dependency vs. removing one. My anecdotal experience is there wasn’t a good rhyme or reason (or malice) for why flake-utils was chosen… someone just followed some old Nix tutorial or copied another project to accomplish a task without even thinking about what systems they actually support (why their strangely limited “defaults” that doesn’t match all the systems Rust can output in this case?) or if all outputs need systems (& I have seen these in the wild + crazy forEach* looks with a // merges trying to deal with the output).

Regardless, these patches stack cleanly so maintainers can cherry-pick commits if they choose. There isn’t a good reason to be opening new patchsets willy-nilly just because the Microsoft GitHub UX is flawed for doing patches/review.

[Xophmeister]

I'm not a Nix person, hence deferring to @Niols and @yannham -- thank you both 🙏 -- but I think a decision needs to be made on this...so I'm going to stick my neck out 🙈

I'm going to approve this, including the "opinionated" part of the PR, for the following reasons:

1. If it replaces a dependency with something straightforward -- and certainly clear (and, therefore, maintainable) -- then that seems like a good thing.
2. Given that @toastal maintains Topiary in Nixpkgs for us, if this makes that process easier for them -- or, indeed, anyone -- then that also seems like a good thing.

[Niols]

Sounds good to me. The patch definitely looks safe to me; let's merge!