No global settings in libnixfetchers and libnixflake

[Ericson2314]

Motivation

Progress on #5638

There are still a global fetcher and eval settings, but they are pushed down into libnixcmd, which is a lot less bad a place for this sort of thing.

Context

Continuing process pioneered in 52bfccf.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[edolstra]

Not in favor of this. As we discussed recently, this causes a huge amount of code churn and adds tedious settings parameters everywhere, for no/unclear benefits.

[fricklerhandwerk]

What if we instead made InputScheme a singleton and instantiated it together with the evaluator? We'd even solve two readability difficulties at once: Right now registerInputScheme is initialised statically, which really threw me off. Explicit initialisation would avoid that, too.

Unless there's a really good reason for static init?

[Ericson2314]

@edolstra

Not in favor of this. As we discussed recently, this causes a huge amount of code churn and adds tedious settings parameters everywhere, for no/unclear benefits.

We can keep on discussing this, but I consider gratuitous globals completely unacceptable in a library, and the new code much easier to read and test because the flow of information is made explicit. (And I think a lot of user experience stuff depends not on Nix CLI, but the downstream projects people will make with the libraries --- that's where the user benefit comes from.)

It is also a necessary step to removing the current configuration system, which I very much do not like, and make a nicer JSON one.

@fricklerhandwerk

What if we instead made InputScheme a singleton and instantiated it together with the evaluator?

There is multiple InputScheme globals, all registered. I would not be opposed to trying to get rid of our C++ static initializer stuff, which has some downsides, but that would be a larger project.

[fricklerhandwerk]

There is multiple InputScheme globals, all registered

I mean it could be one InputSchemes singleton that has a map of specialised InputScheme. I imagine it to be a fairly limited change.

[Ericson2314]

@fricklerhandwerk See InputSchemeMap, the global registration is already a map. It could have a fetchers settings too, but then different clients would be stepping on their toes with each others' settings --- no good.

[Ericson2314]

@fricklerhandwerk If it makes a difference to you, once I use this to move the config file handling code out of libutil and libstore and into libcmd or libmain, the rebuild times for changing a settings description ought to be much better :)

[fricklerhandwerk]

It could have a fetchers settings too, but then different clients would be stepping on their toes with each others' settings --- no good.

Wait, when would we even have multiple clients at once? And why can't we then register fetchers with the evaluator rather than globally? I understand that moving all of this code around is work, but there surely is a sequence of small steps that gets us there safely - if that's where we want to even want to go.

[Ericson2314]

Wait, when would we even have multiple clients at once?

Maybe I am writing an multi-tenant web application in which different customers want to have different fetch settings

And why can't we then register fetchers with the evaluator rather than globally?

The sort of auto registrations we have to day are inherently global. I would like to someday move away from that but it's a larger not smaller patch.

I understand that moving all of this code around is work, but there surely is a sequence of small steps that gets us there safely - if that's where we want to even want to go.

The one thing that makes this smaller is doing the fetchers stuff and flake stuff separately, but I still thinking "smaller" is a red herring here. It's not unreliably large, and splitting "churn" over multiple PRs doesn't actually decrease churn.

[edolstra]

@Ericson2314

I consider gratuitous globals completely unacceptable in a library

You have to consider the YAGNI principle here. These libraries are not intended for external consumers, but only for use in Nix. I'm not aware of an actual use case (other than multi-threaded unit tests) that would be significantly helped by this refactoring.

Meanwhile there is a real cost to code churn like this (e.g. I had to spend some time yesterday to update the lazy-trees branch to resolve the previous settings-related PR, and I imagine this one has an even bigger impact).

It is also a necessary step to removing the current configuration system,

That scares me even more...

[roberth]

NIX_PATH, #9574 shows that the settings system is not sustainable.
I wholeheartedly welcome John's efforts to make progress to simplify it. Reducing our reliance on globals is definitely a step in the right direction.

code churn

I can't hear this anymore. If we reject changes based on line count, how will we ever make process on cross-cutting issues?

to update the lazy-trees branch

That branch is too big and not in a mergeable state because it has a observable nondeterminism problem in the expression language. Haven't we already agreed that the right process to get that project done is to extract reviewable changes from that branch?

[Ericson2314]

I am happy to keep on merging master into lazy-trees, whenever I do one of these PRs, while @edolstra is still extracting reusable changes from it, as I have offered many times.

Perhaps @edolstra is skeptical of this because it hasn't happened yet. Well, let's make it start right now. I'll push to lazy-trees or open a PR against it in a moment from master.

[Ericson2314]

I just pushed a zero-conflict resolution of lazy-trees. Easy of course, but I'm happy to do hard ones.

[Ericson2314]

edolstra#14 fixes the conflicts in lazy-trees if we do this.

[roberth]

Could we merge this after this bugfix? #10891

[edolstra]

NIX_PATH, #9574 shows that the settings system is not sustainable.

Those issues have nothing to do with whether our settings objects are global or passed around by reference.

I can't hear this anymore. If we reject changes based on line count, how will we ever make process on cross-cutting issues?

Given that we all have limited time, big changes like this need to be useful, i.e. they need to be motivated by an actual user need, not because they make the code subjectively nicer (and I would argue that passing settings arguments around everywhere isn't nicer and doesn't make the code easier to read or maintain).

Haven't we already agreed that the right process to get that project done is to extract reviewable changes from that branch?

Making reviewable changes only becomes harder because of big refactoring PRs, since having more PRs open means more work keeping them in sync with master.

(Yes, annoyed because we now need to fix a bunch of merge conflicts.)

[fricklerhandwerk]

Just to clarify, I agree this particular change makes it more ugly than before, and I also hate it that we get more merge conflicts. But @Ericson2314 convinced me that things will get continuously better. That is not to make it "subjectively nicer", but "objectively less effort to understand for people not familiar with the code base". I may be the only vocal data point on this, but currently the config system is just confusing because information is not local and execution order isn't obvious or hard to trace.

Also, merge conflicts get more likely with large long-standing PRs. We discussed this often enough, and even wrote into the contribution guidelines to aim for small changes that can be merged quickly. Merge conflicts also get more likely where the code isn't cleanly separating concerns. We can alleviate that by talking to each other and coordinating more closely. Yes, this is overhead. Doing more stuff always means more overhead.

[Ericson2314]

1. The need isn't hypothetical: There are multiple library Nix projects in flight, including me working with @qknight to try to make a Nix backend for package managers like Cargo. We cannot have other executables just start snooping Nix config files and doing other arbitrary IO, or our library looks unprofessional and "trying to boss the end exe around".

2. I am fixing the lazy trees merge conflicts as we speak.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-07-10-nix-team-meeting-minutes-160/49101/1