Skip to content

Comments

fetchurl: Make it overridable using callPackage#66503

Merged
infinisil merged 1 commit intoNixOS:masterfrom
nh2:overridable-fetchurl
Aug 12, 2019
Merged

fetchurl: Make it overridable using callPackage#66503
infinisil merged 1 commit intoNixOS:masterfrom
nh2:overridable-fetchurl

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 on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Aug 12, 2019
lilyball
lilyball reviewed Aug 12, 2019
View reviewed changes
@infinisil
Copy link
Member

infinisil commented Aug 12, 2019

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 Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes 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 Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 12, 2019
@nh2 nh2 mentioned this pull request Aug 12, 2019
Merged
11 tasks
@nh2
fetchurl: Make it overridable.
cd6033b 
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.
@nh2 nh2 force-pushed the overridable-fetchurl branch from ee55f19 to cd6033b Compare August 12, 2019 04:51
@ofborg ofborg bot added the 8.has: clean-up This PR removes packages or removes other cruft label Aug 12, 2019
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. 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. and removed 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. 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. 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

infinisil commented Aug 12, 2019

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

@infinisil infinisil merged commit 096031a into NixOS:master Aug 12, 2019
This was referenced Aug 12, 2019
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@infinisil infinisil Awaiting requested review from infinisil

@FRidh FRidh Awaiting requested review from FRidh

@Profpatsch Profpatsch Awaiting requested review from Profpatsch

@ttuegel ttuegel Awaiting requested review from ttuegel

1 more reviewer

@lilyball lilyball lilyball left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nh2 @infinisil @FRidh @lilyball