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

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

Merged
merged 1 commit into from
Feb 21, 2019

Conversation

oxij
Copy link
Member

@oxij oxij commented Feb 19, 2019

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).
Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

Awesome work! This definitely looks good to me.

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Feb 19, 2019
@7c6f434c 7c6f434c merged commit a059fc7 into NixOS:master Feb 21, 2019
@Ericson2314
Copy link
Member

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

@nh2
Copy link
Contributor

nh2 commented Aug 12, 2019

@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
Copy link
Member Author

oxij commented Aug 12, 2019 via email

@oxij
Copy link
Member Author

oxij commented Aug 12, 2019 via email

@nh2
Copy link
Contributor

nh2 commented Aug 13, 2019

@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
Copy link
Member Author

oxij commented Aug 13, 2019 via email

@nh2
Copy link
Contributor

nh2 commented Aug 13, 2019

@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.

@oxij oxij deleted the tree/fetchurl-boot branch August 12, 2023 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants