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: remove flake-utils dependency #1226

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

toastal
Copy link

@toastal toastal commented Jan 13, 2024

By inlining the for loop from the utility library, downstream consumers do not need to pull in any additional dependencies & bloat their flake.lock files. Some additional reorganizing due to inlining as well—with the net effect of more lines deleted than added.

@toastal
Copy link
Author

toastal commented Jan 13, 2024

Diff is slightly easier to follow without white space (indentation needed adjusting since ) https://github.com/ocaml/ocaml-lsp/pull/1226/files?diff=split&w=1

@@ -1,23 +1,5 @@
{
"nodes": {
"flake-utils": {
Copy link
Author

Choose a reason for hiding this comment

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

This would need to be added to all downstream users’ flake.lock which is a pretty heavy price for a basic loop.

dune-rpc = osuper.dune-rpc.overrideAttrs fixPreBuild;
stdune = osuper.stdune.overrideAttrs fixPreBuild;

supportedSystems = [
Copy link
Author

Choose a reason for hiding this comment

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

Not Nix’s defaults (theirs includes 32-bit Linux last I checked), but matches the default systems of flake-utils so the change should have no effect on anyone.

let
pkgs = import nixpkgs { overlays = [ overlay ]; inherit system; };
inherit (pkgs.ocamlPackages) buildDunePackage;
fast = rec {
Copy link
Author

Choose a reason for hiding this comment

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

I think these are stubs to speed up the build? I moved them all to packages since it doesn’t hurt while using less recs & temporary attrsets which slow down evaluation.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4083

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 20.384%

Totals Coverage Status
Change from base Build 4082: 0.0%
Covered Lines: 4851
Relevant Lines: 23798

💛 - Coveralls

Copy link
Contributor

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

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

By inlining the for loop from the utility library, downstream consumers do not need to pull in any additional dependencies & bloat their flake.lock files.

  • flake-utils adds a comparable number of lines to flake.lock than any other flake. I don't understand why this is considered "bloating"
    • flake.lock is a generated file that most people don't inspect, apart from when the git diff changes
  • the utility library doesn't provide just "a basic for loop", and this reductionism feels a bit disingenuous; rather, it attempts to settle on a common set of systems that flake-utils users can support, apart from abstracting away the nitty-gritty of specifying the correct structure for systems in flake.nix.

Personally, and subjectively, this PR also affects readability. What was once a single function call to eachDefaultSystem now becomes 3 calls to forAllSystems.

@toastal
Copy link
Author

toastal commented Feb 22, 2024

Adding 1 dependency is more than 0 dependency. Dependencies need to be audited & unless you remember to follows downstream will end up with a several versions. …And a lot of things in the OCaml ecosystem I have noticed are depending on this LSP. The defaultSystems flake-utils support doesn’t even match Nixpkgs default systems & with it being explicitly shown, the abstraction makes it more difficult for a downstream read to understand if their architecture will be supported (which is why nix flake init generates its defaults as explicitly outputs.packages.x86_64-linux.hello since the package will be immediately be tested and can only be assured to run on the platform of my machine (not platforms I don’t own like anything Darwin)).

With utilities, we are also talking about a leftPad situation tho… like, sure everyone using that same utility means folks can share logic, but when the dependency disappeared the ecosystem broke despite that utility being a couple of lines of JavaScript. flake-utils.forEach is just a wrapper around a loop while + adding the pkgs to the closure (which this in-file version could add to the closure if that is preferred).

Currently at a physical Nix sprint in Thailand… & for an anecdote I asked, without pretense, a Numtide employee “thoughts on adding Numtide’s flake-utils as a flake input dependency for only eachDefaultSystem” with the response “we pretty much only use flake-parts for [more complicated] stuff now, but no, I wouldn’t include [flake-utils] just for that”. This has also been corroborated in larger community Matrix chats & others spending a part of their sprint culling unnecessary inputs from their flakes in order to remove dependencies (since there is no devInputs, devDependiences, etc.).

By inlining the for loop from the utility library, downstream consumers
do not need to pull in any additional dependencies & bloat their
flake.lock files. Some additional reorganizing due to inlining as well
—with the net effect of more lines deleted than added.
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.

3 participants