fetchurl: add user agent #17757

copumpkin · 2016-08-15T14:29:58Z

[Looking for feedback, please don't merge yet as I haven't tested it]

It would be nice to be able to track Nix requests. It's not trustworthy, but can be helpful for stats and routing in HTTP logs.

Since fetchurl is used so widely, we should "magically" get a UA on fetchzip, fetchFromGitHub, and other related fetchers.

Since fetchurl is only used for fixed-output derivations, this should cause no mass rebuild.

mention-bot · 2016-08-15T14:30:00Z

@copumpkin, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @domenkozar and @urkud to be potential reviewers

copumpkin · 2016-08-15T14:36:57Z

@cleverca22 pointed out that Nix's built-in downloader already sets the UA to "Nix/", so I should probably replicate that format (but still want the Nixpkgs revision). https://github.com/NixOS/nix/blob/master/src/libstore/download.cc#L138

joachifm · 2016-08-15T18:23:04Z

I'm curious: why is the nixpkgs revision interesting?

copumpkin · 2016-08-15T18:24:26Z

Because it affects behavior, and can help explain why the HTTP request is being made the way it is. Same reason the Nix version is, really.

edolstra · 2016-08-16T09:44:45Z

pkgs/build-support/fetchurl/default.nix

@@ -124,6 +124,8 @@ if (!hasHash) then throw "Specify hash for fetchurl fixed-output derivation: ${s

  inherit curlOpts showURLs mirrorsFile impureEnvVars postFetch downloadToTemp executable;

+  userAgent = "Nix ${builtins.nixVersion} on nixpkgs ${stdenv.lib.nixpkgsVersion}";


Doing this will cause all .drv files to change every time you update Nixpkgs, which is a lot of unnecessary I/O.

Also, shouldn't this be the other way around ("Nixpkgs on Nix")?

What if we tracked a version specifically for fetchurl, which changes way less than nixpkgs?

Could be the hash of the fetchurl files, I suppose!

Not ideal (hard to track back to what it is) but sounds good to me.

On Wed, Aug 31, 2016 at 10:17 PM Daniel Peebles notifications@github.com
wrote:

In pkgs/build-support/fetchurl/default.nix
#17757 (comment):

@@ -124,6 +124,8 @@ if (!hasHash) then throw "Specify hash for fetchurl fixed-output derivation: ${s

inherit curlOpts showURLs mirrorsFile impureEnvVars postFetch downloadToTemp executable;

userAgent = "Nix ${builtins.nixVersion} on nixpkgs ${stdenv.lib.nixpkgsVersion}";

Could be the hash of the fetchurl files, I suppose!

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
https://github.com/NixOS/nixpkgs/pull/17757/files/75b22922b8f285cf0e36ab900a807e3ca5f4ca6f#r77104796,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAErrNsyIUwykglYvGFe5RECxzM05YBiks5qljXEgaJpZM4Jkbay
.

I think it would be enough to put "current nixos version" in there, e.g. 16.09 ATM, or are there any expectations about the version?

None, but many of us aren't on NixOS :P
On Fri, Sep 2, 2016 at 11:31 Vladimír Čunát notifications@github.com
wrote:

In pkgs/build-support/fetchurl/default.nix
#17757 (comment):

@@ -124,6 +124,8 @@ if (!hasHash) then throw "Specify hash for fetchurl fixed-output derivation: ${s

inherit curlOpts showURLs mirrorsFile impureEnvVars postFetch downloadToTemp executable;

userAgent = "Nix ${builtins.nixVersion} on nixpkgs ${stdenv.lib.nixpkgsVersion}";

I think it would be enough to put nixos version in there, e.g. 16.09 ATM,
or are there any expectations about the version?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/NixOS/nixpkgs/pull/17757/files/75b22922b8f285cf0e36ab900a807e3ca5f4ca6f#r77365738,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAKP03oU-w8_fc6yA-SfjoQgse6O4cTks5qmEE2gaJpZM4Jkbay
.

grahamc · 2016-09-01T01:50:02Z

I am a huge fan of this change, as it gives mirrors and upstreams a way to identify and get in touch with us.

copumpkin · 2016-09-05T20:48:26Z

What I'd really like to do here is compute the git tree-style hash of the entire fetchurl folder, so it becomes fairly straightforward to search the git repo for trees that match the hash in the user agent.

But as far as I can tell, it's a huge pain in the ass to manually compute the git tree hash that respects all of git's semantics so I might just go with a recursive nix-hash of the fetchurl folder for now and have people use git-bisect to track down a set of nixpkgs revisions that match the nix hash.

@edolstra @grahamc @vcunat does that seem reasonable to you? Basically, it would be the same as this one, but the .drvs would only change when fetchurl's source changes.

aneeshusa · 2016-10-08T14:18:53Z

Can you please make it easy to override/completely disable setting this value? I've never liked the concept of the UA field, dislike the privacy leak and would prefer that servers do proper feature detection instead of sniffing/guessing.

lukateras · 2017-12-20T13:51:25Z

@aneeshusa This is for stats, not for sniffing/guessing. Currently used User-Agent leaks the fact that you use curl and which particular version you have installed.

Since including Nixpkgs version will cause a lot of I/O, I think that having some custom User-Agent is more beneficial even if it is omitted. I propose the following User-Agent:

User-Agent: curl/7.57.0 Nix/1.11.16

It is compatible with one that is currently used, it states curl version (which is the actual fetcher), and it additionally states Nix version.

lukateras · 2017-12-20T18:59:28Z

I've dropped Nixpkgs version from User-Agent until perhaps we do that via impureEnvVars. @edolstra, could you take a look at this?

edolstra · 2017-12-21T10:47:16Z

I'm not convinced that depending on the Nix version is a good idea. That just leads to spurious .drv changes (e.g. an identical Nixpkgs expression evaluation with Nix 1.11.15 and 1.11.16 will produce a different result).

edolstra · 2017-12-21T10:51:02Z

It would be better to include the Nixpkgs major version, e.g. Nixpkgs/17.09.

lukateras · 2017-12-21T11:47:37Z

I agree, Nixpkgs version makes more sense. Fixed, now user agent is:

User-Agent: curl/7.57.0 Nixpkgs/18.03

It would be nice to be able to track Nix requests. It's not trustworthy, but can be helpful for stats and routing in HTTP logs. Since `fetchurl` is used so widely, we should "magically" get a UA on `fetchzip`, `fetchFromGitHub`, and other related fetchers. Since `fetchurl` is only used for fixed-output derivations, this should cause no mass rebuild. User-Agent example: curl/7.57.0 Nixpkgs/18.03

lukateras · 2017-12-23T22:23:11Z

pkgs/build-support/fetchurl/builder.sh

@@ -2,20 +2,23 @@ source $stdenv/setup

 source $mirrorsFile

+curlVersion=$(curl -V | head -1 | cut -d' ' -f2)


I'm doing this instead of curl.version to support native stdenv (e.g. on Darwin).

lukateras · 2017-12-24T00:03:38Z

@edolstra: I think this is ready to merge (fetchurl user agent), also it now uses Nixpkgs version instead of Nix version and doesn't cause a mass rebuild. Do you approve it?

orivej · 2017-12-24T00:16:33Z

There is no way to edit fetchurl's builder.sh and not cause a mass rebuild; the bot must be wrong.

lukateras · 2017-12-24T00:21:38Z

@copumpkin said:

Since fetchurl is only used for fixed-output derivations, this should cause no mass rebuild.

Also I haven't noticed any mass rebuilds when I was testing my changes locally.

orivej · 2017-12-24T00:48:59Z

Ah, ok. I was looking at the wrong metric: this changes almost all .drv files, but the outputs remain the same.

edolstra · 2017-12-25T13:38:51Z

pkgs/build-support/fetchurl/default.nix

@@ -132,6 +132,8 @@ else stdenv.mkDerivation {

  impureEnvVars = impureEnvVars ++ netrcImpureEnvVars;

+  nixpkgsVersion = fileContents ../../../.version;


Why not just export the major version from lib?

lib doesn't export major version specifically, only suffixed version:

nixpkgs/lib/trivial.nix

Lines 62 to 65 in b5a61f2

nixpkgsVersion =

let suffixFile = ../.version-suffix; in

fileContents ../.version

+ (if pathExists suffixFile then fileContents suffixFile else "pre-git");

dtzWill · 2018-05-18T07:02:39Z

This looks ready, or close to it, shall we go ahead with this?

bobvanderlinden · 2018-05-31T20:00:56Z

pkgs/build-support/fetchurl/default.nix

@@ -135,6 +133,8 @@ stdenvNoCC.mkDerivation {

  impureEnvVars = impureEnvVars ++ netrcImpureEnvVars;

+  nixpkgsVersion = lib.fileContents ../../../.version;


You might be able to use ~~lib.version~~ lib.release now?

bobvanderlinden · 2018-05-31T20:01:50Z

pkgs/build-support/fetchurl/builder.sh


 # Curl flags to handle redirects, not use EPSV, handle cookies for
 # servers to need them during redirects, and work on SSL without a
 # certificate (this isn't a security problem because we check the
 # cryptographic hash of the output anyway).
-curl="curl \
- --location --max-redirs 20 \
- --retry 3 \


Was removing --retry 3 intentional?

Good catch, thanks!

lukateras · 2018-06-11T01:55:24Z

May I go ahead and merge this?

bobvanderlinden · 2018-06-11T12:47:35Z

Looking for feedback, please don't merge yet as I haven't tested it

@copumpkin Do you feel this is ready?

copumpkin · 2018-06-11T15:46:57Z

Yeah, if it doesn't cause excess .drv churn, this seems good!

lukateras · 2018-06-11T15:59:50Z

[...] if it doesn't cause excess .drv churn [...]

Only twice a year :-)

copumpkin · 2018-06-11T16:49:29Z

Here goes... 😄 thanks!

lukateras · 2018-06-11T16:56:26Z

Thank you!

copumpkin added 0.kind: enhancement 9.needs: reporter feedback labels Aug 15, 2016

edolstra reviewed Aug 16, 2016
View reviewed changes

lukateras self-assigned this Dec 20, 2017

lukateras force-pushed the fetchurl-user-agent branch from 75b2292 to e258ec6 Compare December 20, 2017 15:58

GrahamcOfBorg added 10.rebuild-darwin-stdenv 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Dec 20, 2017

lukateras force-pushed the fetchurl-user-agent branch from e258ec6 to 720cf53 Compare December 20, 2017 18:53

lukateras requested a review from edolstra December 20, 2017 18:54

lukateras changed the base branch from master to staging December 20, 2017 18:58

lukateras force-pushed the fetchurl-user-agent branch from 720cf53 to 84bdc22 Compare December 20, 2017 18:58

lukateras added 1.severity: mass-rebuild and removed 9.needs: reporter feedback 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Dec 20, 2017

lukateras force-pushed the fetchurl-user-agent branch from 84bdc22 to df29d72 Compare December 21, 2017 11:45

lukateras added the 6.topic: fetch label Dec 23, 2017

lukateras force-pushed the fetchurl-user-agent branch from d30f1e9 to 0cb623c Compare December 23, 2017 22:21

lukateras changed the base branch from staging to master December 23, 2017 22:21

lukateras reviewed Dec 23, 2017

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 and removed 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ labels Dec 23, 2017

edolstra reviewed Dec 25, 2017

View reviewed changes

Merge branch 'master' into fetchurl-user-agent

16ed09a

bobvanderlinden reviewed May 31, 2018

View reviewed changes

bobvanderlinden mentioned this pull request Jun 10, 2018

Pull requests becoming stale #41793

Closed

lukateras added 2 commits June 11, 2018 01:48

Readd --retry 3

a26d9e3

Leverage lib.release

5a232b0

lib.release -> lib.trivial.release

f44012b

bobvanderlinden approved these changes Jun 11, 2018

View reviewed changes

lukateras approved these changes Jun 11, 2018

View reviewed changes

copumpkin merged commit 3633632 into NixOS:master Jun 11, 2018

sikmir mentioned this pull request Jun 10, 2020

Overriding user-agent used by fetchurl #90034

Closed

		@@ -124,6 +124,8 @@ if (!hasHash) then throw "Specify hash for fetchurl fixed-output derivation: ${s

		inherit curlOpts showURLs mirrorsFile impureEnvVars postFetch downloadToTemp executable;

		userAgent = "Nix ${builtins.nixVersion} on nixpkgs ${stdenv.lib.nixpkgsVersion}";

		@@ -2,20 +2,23 @@ source $stdenv/setup

		source $mirrorsFile

		curlVersion=$(curl -V \| head -1 \| cut -d' ' -f2)

		@@ -132,6 +132,8 @@ else stdenv.mkDerivation {

		impureEnvVars = impureEnvVars ++ netrcImpureEnvVars;

		nixpkgsVersion = fileContents ../../../.version;

	nixpkgsVersion =
	let suffixFile = ../.version-suffix; in
	fileContents ../.version
	+ (if pathExists suffixFile then fileContents suffixFile else "pre-git");

		@@ -135,6 +133,8 @@ stdenvNoCC.mkDerivation {

		impureEnvVars = impureEnvVars ++ netrcImpureEnvVars;

		nixpkgsVersion = lib.fileContents ../../../.version;

fetchurl: add user agent #17757

fetchurl: add user agent #17757

Conversation

copumpkin commented Aug 15, 2016

mention-bot commented Aug 15, 2016

copumpkin commented Aug 15, 2016

joachifm commented Aug 15, 2016

copumpkin commented Aug 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcunat Sep 2, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamc commented Sep 1, 2016

copumpkin commented Sep 5, 2016

aneeshusa commented Oct 8, 2016

lukateras commented Dec 20, 2017 • edited

lukateras commented Dec 20, 2017

edolstra commented Dec 21, 2017

edolstra commented Dec 21, 2017

lukateras commented Dec 21, 2017 • edited

Choose a reason for hiding this comment

lukateras commented Dec 24, 2017

orivej commented Dec 24, 2017

lukateras commented Dec 24, 2017 • edited

orivej commented Dec 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtzWill commented May 18, 2018

bobvanderlinden May 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukateras commented Jun 11, 2018

bobvanderlinden commented Jun 11, 2018

copumpkin commented Jun 11, 2018

lukateras commented Jun 11, 2018 • edited

copumpkin commented Jun 11, 2018

lukateras commented Jun 11, 2018

vcunat Sep 2, 2016 •

edited

lukateras commented Dec 20, 2017 •

edited

lukateras commented Dec 21, 2017 •

edited

lukateras commented Dec 24, 2017 •

edited

bobvanderlinden May 31, 2018 •

edited

lukateras commented Jun 11, 2018 •

edited