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

fetchurl: Make it overridable using callPackage #66503

Merged
merged 1 commit into from
Aug 12, 2019

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Aug 12, 2019

Motivation for this change

This provides a workaround for #66499, because this way #66499 (comment)

self: super: {
  fetchurl = super.fetchurl.override (old: {
    curl = old.curl.override {
      gssSupport = false;
      # ...
    };
  });
}

works as expected.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @infinisil via

@nh2 nh2 requested a review from infinisil August 12, 2019 01:44
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Aug 12, 2019
@infinisil
Copy link
Member

As I was expecting, this doesn't actually change any semantics, so there's no rebuilds, so this can go to master :D. And judging from the evaluation performance report, this isn't that bad for performance either (though it's not negligible):

stat before after Δ Δ%
cpuTime 159.32 162.18 ↗ 2.86 1.79%
envs-bytes 3,104,910,512 3,109,107,072 ↗ 4,196,560 0.14%
envs-elements 166,823,790 166,998,908 ↗ 175,118 0.10%
envs-number 110,645,012 110,819,738 ↗ 174,726 0.16%
gc-heapSize 12,365,262,848 12,382,040,064 ↗ 16,777,216 0.14%
gc-totalBytes 24,105,335,232 24,116,554,288 ↗ 11,219,056 0.05%
list-bytes 1,608,574,016 1,608,574,976 ↗ 960 0.00%
list-concats 7,548,881 7,548,881 0
list-elements 201,071,752 201,071,872 ↗ 120 0.00%
nrAvoided 155,782,387 155,783,101 ↗ 714 0.00%
nrFunctionCalls 96,490,868 96,665,470 ↗ 174,602 0.18%
nrLookups 77,578,468 77,579,037 ↗ 569 0.00%
nrOpUpdateValuesCopied 240,027,894 240,028,230 ↗ 336 0.00%
nrOpUpdates 13,333,203 13,333,344 ↗ 141 0.00%
nrPrimOpCalls 67,845,752 67,846,059 ↗ 307 0.00%
nrThunks 159,833,904 159,834,668 ↗ 764 0.00%
sets-bytes 8,074,679,456 8,074,714,480 ↗ 35,024 0.00%
sets-elements 326,921,442 326,922,789 ↗ 1,347 0.00%
sets-number 28,570,606 28,570,943 ↗ 337 0.00%
sizes-Attr 24 24 0
sizes-Bindings 8 8 0
sizes-Env 16 16 0
sizes-Value 24 24 0
symbols-bytes 15,063,659 15,063,659 0
symbols-number 331,928 331,928 0
values-bytes 5,615,326,464 5,619,533,208 ↗ 4,206,744 0.07%
values-number 233,971,936 234,147,217 ↗ 175,281 0.07%

Ping @grahamc because he was interested in this

@nh2 nh2 force-pushed the overridable-fetchurl branch 2 times, most recently from 04bbb1b to ee55f19 Compare August 12, 2019 04:34
@nh2 nh2 changed the base branch from staging to master August 12, 2019 04:34
@nh2
Copy link
Contributor Author

nh2 commented Aug 12, 2019

there's no rebuilds, so this can go to master

Great, I've changed base to master.

@nh2
Copy link
Contributor Author

nh2 commented Aug 12, 2019

@GrahamcOfBorg eval

@nh2
Copy link
Contributor Author

nh2 commented Aug 12, 2019

Does changing base prevent ofborg to eval? I don't see it pop up in the Github checks.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 6.topic: qt/kde 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` and removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 6.topic: qt/kde 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 12, 2019
This provides a workaround for NixOS#66499, because this way
NixOS#66499 (comment)

    self: super: {
      fetchurl = super.fetchurl.override (old: {
        curl = old.curl.override {
          gssSupport = false;
          # ...
        };
      });
    }

works as expected.
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild 10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500 labels Aug 12, 2019
@FRidh
Copy link
Member

FRidh commented Aug 12, 2019

Please include the motivation in the commit message instead of only referring to a ticket.

@infinisil
Copy link
Member

@FRidh I think he did?

@nh2
Copy link
Contributor Author

nh2 commented Aug 12, 2019

Yes, I have in there:

This provides a workaround for #66499, because this way
https://github.com/NixOS/nixpkgs/issues/66499#issuecomment-520277782

    self: super: {
      fetchurl = super.fetchurl.override (old: {
        curl = old.curl.override {
          gssSupport = false;
          # ...
        };
      });
    }

works as expected.

Happy to elaborate more if given some hints what to elaborate on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants