treewide: assemble all `fetchurlBoot` uses in overrides to `fetchurl` #56067

oxij · 2019-02-19T21:16:50Z

What?

This kills top-level fetchurlBoot attribute and isolates rest of the uses to stdenv, normal fetchurl and one Darwin reference I don't touch because I can't test it.

Note that this change is a noop for nix-env -qaP --out-path diff. nix-env -qaP --drv-path diff is huge because everything that depends on curl itself and anything curl itself depends on will evaluate to new derivations. I.e. this changes a lot of .drvs but does zero rebuilds.

Stuff referencing pkgsCross works as before, so I declare this not breaking anything. Before rebase I was running on top of this for 3 months or so.

Why?

fetchurlBoot got in the way of clean breaking of dependency cycles for my doCheckByDefault thingy (#39464), I got pissed and (mostly) killed it.

`git log`

treewide: assemble all fetchurlBoot uses in overrides to fetchurl itself

The only outside-curl uses of fetchurlBoot left are stdenv
and apple-source-releases. The latter one can probably be removed
too, but I can't test it.

Pros:
- Aggregates all behind-the-scenes insanity in a single place.
Cons:
- At the cost of 10 more derivations (but 0 new outpaths).

`nix-instantiate` environment

Host OS: Linux 4.9, SLNOS 19.03
Nix: nix-env (Nix) 2.1.3
Multi-user: yes
Sandbox: yes
NIXPKGS_CONFIG:

{
  checkMeta = true;
  doCheckByDefault = true;
}

`nix-env -qaP --out-path` diffs

On x86_64-linux: noop
On aarch64-linux: noop
On x86_64-darwin: noop

/cc @Ericson2314 @matthewbauer @7c6f434c

… itself The only outside-curl uses of `fetchurlBoot` left are `stdenv` and `apple-source-releases`. The latter one can probably be removed too, but I can't test it. Pros: - Aggregates all behind-the-scenes insanity in a single place. Cons: - At the cost of 10 more derivations (but 0 new outpaths).

matthewbauer

Awesome work! This definitely looks good to me.

grahamc · 2019-02-19T21:23:15Z

pkgs/development/interpreters/perl/default.nix

@@ -1,4 +1,4 @@
-{ lib, stdenv, fetchurlBoot, buildPackages
+{ lib, stdenv, fetchurl, buildPackages


I feel this is a degradation in clarity, making it clear this is a super low dependency package. Do you think otherwise?

Well, I see your point, but I think uniformity of package expressions should be preferable to low-level implementation details, and fetchurlBoot is such an implementation detail.

Ericson2314 · 2019-02-21T15:46:18Z

Thanks!! I've been hoping to see this go for awhile now.

nh2 · 2019-08-12T04:03:06Z

@oxij You write:

      # On darwin, libkrb5 needs bootstrap_cmds which would require
      # converting many packages to fetchurl_boot to avoid evaluation cycles.
      gssSupport = !stdenv.isDarwin && !stdenv.hostPlatform.isWindows;

The comment talks only about darwin, but there's also this part about Windows.

Why does hostPlatform.isWindows also disable it?

I'm trying to add some more comments here, so it would be great to know this.

oxij · 2019-08-12T23:33:10Z

@oxij You write: ``` # On darwin, libkrb5 needs bootstrap_cmds which would require # converting many packages to fetchurl_boot to avoid evaluation cycles. gssSupport = !stdenv.isDarwin && !stdenv.hostPlatform.isWindows; ```

That was introduced in c303047 by @vcunat. (`git log -p -S 'On darwin, libkrb5 needs bootstrap_cmds which would require'`)

The comment talks only about darwin, but there's also this part about Windows. Why does `hostPlatform.isWindows` also disable it?

I took that from the corresponding line of `pkgs/tools/networking/curl/default.nix` to keep the value of that parameter on Windows. Now that I think about it, #66506 should probably do ```diff - # On darwin, libkrb5 needs bootstrap_cmds which would require - # converting many packages to fetchurl_boot to avoid evaluation cycles. - gssSupport = !stdenv.isDarwin && !stdenv.hostPlatform.isWindows; + gssSupport = !stdenv.isDarwin || old.gssSupport; ```

oxij · 2019-08-12T23:37:15Z

Oops, I meant ```diff + gssSupport = !stdenv.isDarwin && old.gssSupport; ```

nh2 · 2019-08-13T00:49:28Z

@oxij I think old.gssSupport by itself without or is correct, because defaulted bool arguments are missing from old: #66546

Regarding what you said, so you mean that I should change my fix from #66506 from

       gssSupport =
        if stdenv.isDarwin || stdenv.hostPlatform.isWindows
          then false
          else old.gssSupport or true; # `? true` is the default

to

       gssSupport =
        if stdenv.isDarwin
          then false
          # The default of `gssSupport ?` in the curl package; see #66546 for the `or`.
          else old.gssSupport or (!stdenv.hostPlatform.isWindows);

Is that correct?

oxij · 2019-08-13T16:53:22Z

Is that correct?

No, my point was to remove duplicated sub-expressions in two definitions of `gssSupport` but, as you note in #66546, that needs a different semantics for `.override (old: ...)`.

nh2 · 2019-08-13T19:01:50Z

@oxij OK. Still, what I suggested there above still is better than what I have now, right?

Using

       gssSupport =
        if stdenv.isDarwin
          then false
          # The default of `gssSupport ?` in the curl package; see #66546 for the `or`.
          else old.gssSupport or (!stdenv.hostPlatform.isWindows);

would not force gssSupport off for Windows, instead it would use old.gssSupport if it had been set to true for Windows manually by the user.

Do you see what I mean?

The original forcing was added with the intent to be just for Darwin, so I'm trying to make it just for Darwin.

matthewbauer approved these changes Feb 19, 2019

View reviewed changes

grahamc reviewed Feb 19, 2019

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Feb 19, 2019

fpletz approved these changes Feb 19, 2019

View reviewed changes

7c6f434c approved these changes Feb 19, 2019

View reviewed changes

7c6f434c merged commit a059fc7 into NixOS:master Feb 21, 2019

infinisil mentioned this pull request Mar 6, 2019

replacement of openssl with libressl with a overlay results in evaluation failures #56994

Open

dylex mentioned this pull request Mar 25, 2019

fetchurl: use fetchurlBoot for zlib deps #58283

Merged

nh2 mentioned this pull request Aug 13, 2019

Document .override (old: ...) #66546

Open

oxij deleted the tree/fetchurl-boot branch August 12, 2023 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

treewide: assemble all `fetchurlBoot` uses in overrides to `fetchurl` #56067

treewide: assemble all `fetchurlBoot` uses in overrides to `fetchurl` #56067

oxij commented Feb 19, 2019 •

edited

matthewbauer left a comment

grahamc Feb 19, 2019

oxij Feb 19, 2019

Ericson2314 commented Feb 21, 2019

nh2 commented Aug 12, 2019

oxij commented Aug 12, 2019 via email

oxij commented Aug 12, 2019 via email

nh2 commented Aug 13, 2019

oxij commented Aug 13, 2019 via email

nh2 commented Aug 13, 2019 •

edited

		@@ -1,4 +1,4 @@
		{ lib, stdenv, fetchurlBoot, buildPackages
		{ lib, stdenv, fetchurl, buildPackages

treewide: assemble all fetchurlBoot uses in overrides to fetchurl #56067

treewide: assemble all fetchurlBoot uses in overrides to fetchurl #56067

Conversation

oxij commented Feb 19, 2019 • edited

What?

Why?

git log

nix-instantiate environment

nix-env -qaP --out-path diffs

matthewbauer left a comment

Choose a reason for hiding this comment

grahamc Feb 19, 2019

Choose a reason for hiding this comment

oxij Feb 19, 2019

Choose a reason for hiding this comment

Ericson2314 commented Feb 21, 2019

nh2 commented Aug 12, 2019

oxij commented Aug 12, 2019 via email

oxij commented Aug 12, 2019 via email

nh2 commented Aug 13, 2019

oxij commented Aug 13, 2019 via email

nh2 commented Aug 13, 2019 • edited

treewide: assemble all `fetchurlBoot` uses in overrides to `fetchurl` #56067

treewide: assemble all `fetchurlBoot` uses in overrides to `fetchurl` #56067

oxij commented Feb 19, 2019 •

edited

`git log`

`nix-instantiate` environment

`nix-env -qaP --out-path` diffs

nh2 commented Aug 13, 2019 •

edited