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.

Sign up for GitHub

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

Jump to bottom

fetchurl: Make it overridable using callPackage #66503

Merged
merged 1 commit into from Aug 12, 2019
Merged

fetchurl: Make it overridable using callPackage #66503

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

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

@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

@lilyball lilyball lilyball left review comments

@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

Assignees
No one assigned
Labels
8.has: clean-up 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nh2 @infinisil @FRidh @lilyball