prefer-remote-fetch: use override instead of knowledge of build helper internals

[philiptaron]

What do you all think of this? There's a lot of ways to approach this problem, but making these fetchers customizable this way makes sense to me.

Replaces:

• prefer-remote-fetch: fix fetchurl eval #457086

Things done

• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

[Mic92]

approach sounds good to me.

[emilazy]

I am wondering if setting preferLocalBuild for these is worth it at all… I guess it can save a round‐trip transfer due to the shortcomings of the remote builder protocol. In general I am sceptical of these kinds of heuristic flags that seem to cause trouble for remote builds as often as they help them. (Though this one not as much as allowSubstitutes, which is such a poor idea that Nix has a feature specifically to ignore it, and which I believe we should just remove across the tree.)

Edit: Well, no, actually, I don’t think it saves a round‐trip at all, it’s just a choice between uploading and downloading, right? When they’re symmetric it shouldn’t matter, and when they’re asymmetric download is pretty much invariably faster. So the only time you’d want preferLocalBuild on fetchers seems to be when you have a connection with fast upload but are using a remote builder that has slow upload. I’d much more often expect the reverse.

[philiptaron]

I am wondering if setting preferLocalBuild for these is worth it at all…

I take no position on the overlay. Shipping this sort of overlay without any tests and with the current implementation seems unwise... and these aren't the only fetchers with preferLocalBuild = true. Personally, I'd be very open to dropping this overlay.

But if it's here, it shouldn't grub around like it currently does.

[emilazy]

Right, for sure, no objection to making these less hacky. I am just wondering if the better (but less conservative, so longer‐term) approach, rather than introducing public API to all our fetchers, would be to drop preferLocalBuild = true; from them entirely – in other words, to apply the overlay for everyone.

[ShamrockLee]

I prefer making all configurations go through the overrideAttrs-related interface and reserving the override-related interface for callPackage purposes.

This is harder for packages, as there's little mechanisms we could use to expose the supported attributes. For build helpers, however, we have function arguments.

PR #457426 proposes a lib.extendMkDerivatin equivalence of args: super.fetch { preferLocalBuild = args.preferLocalBuild or false; } with function arguments inherited.

[ShamrockLee]

Or how about adding a new pkg.config option: config.preferRemoteFetch?

[SuperSandro2000]

I've pushed an extra commit to fix eval for fetchurl and now my system evaluates again.

[MattSturgeon]

I prefer making all configurations go through the overrideAttrs-related interface and reserving the override-related interface for callPackage purposes.

Rightly or wrongly, callPackage args are commonly used to configure high-level aspects of packages, like { withFoo ? true, withBar ? false, enableXyz ? true, }:.

I don't want to block a PR for continuing to use a pattern that is prevalent throughout nixpkgs. AFAIK there's been no discussion, guidance, or documentation around discouraging this pattern either?

---

In #457086 (review), @philiptaron complained this has no tests. So I'm slightly confused to see no tests added here 😉

Not blocking though, as this is a clear simplification even without test coverage.

[MattSturgeon]

This should be consistent with the rest of the PR:

Suggested change

	cpPreferLocalBuild = preferLocalBuild;
	preferLocalBuildDefault = preferLocalBuild;

[MattSturgeon]

This comment should be moved to where the default is now set. I.e., the ? true in the callPackage args.

[MattSturgeon]

The second commit feels part of the same "logical unit" as the first, so should be squashed.

[ShamrockLee]

I prefer making all configurations go through the overrideAttrs-related interface and reserving the override-related interface for callPackage purposes.

Rightly or wrongly, callPackage args are commonly used to configure high-level aspects of packages, like { withFoo ? true, withBar ? false, enableXyz ? true, }:`.

I don't want to block a PR for continuing to use a pattern that is prevalent throughout nixpkgs. AFAIK there's been no discussion, guidance, or documentation around discouraging this pattern either?

I agree. Enforcement should be based on consensus.

Still, pkgs.config seems to be more common when it comes to global defaults, and might even be easier to implement (no extra let preferLocalBuildDefault = preferLocalBuild; in).

[MattSturgeon]

Yeah using pkgs.config probably makes sense. My only hangup is naming: if it becomes a pkgs.config option, it should be clear this is specifically about “fetchers” rather than a global “preferLocalBuild” knob like --option substitute false. Something like pkgs.config.directlyFetchSources could work.

I’d also be fine leaving this PR as a cleanup and deferring a config-based refactor for a follow-up. Up to @philiptaron whether to expand scope here or keep this PR minimal and let a separate PR explore the config direction.

[SuperSandro2000]

Please ping me when you have decided on something. Thanks!

[ShamrockLee]

It just came to me that I expose fetchgit.getRevWithTag in #456751 , which inherently won't survive function composition of any kind (unless re-attached manually). The <function>.override-based overriding surely handles this better.

For the choice of override arguments, I still lean toward config for global default, but the design decision is up to @philiptaron.

[philiptaron]

Closing this in favor of #475142