stdenv: don't include drvs in disallowedRefs as build-time deps. #211783

r-vdp · 2023-01-20T15:12:25Z

Derivations listed as disallowedReferences or disallowedRequisites, currently end up as build-time dependencies.
This is problematic since the disallowed derivations will be built by nix as build-time dependencies, while those derivations might take a very long time to build, or might not even build successfully on the platform used. However, in order to scan for disallowed references in the final output, knowing the out path is sufficient, and the out path can be calculated from the derivation without needing to build it, saving time and resources.

While the problem is less severe for allowedReferences and allowedRequisites, since we want the derivation to be built eventually, we would still like to get the error early and without having to wait while nix builds a derivation that might not be used (e.g. if we listed the wrong one).

Before this PR:

$ nix-store --query --references (nix-store --query --deriver (nix build --print-out-paths --impure --expr 'with import <nixpkgs> {}; hello.overrideAttrs(_: { disallowedReferences = [ busybox ]; })'))
/nix/store/6xg259477c90a229xwmb53pdfkn6ig3g-default-builder.sh
/nix/store/8xiy49msk7nv3k3rmqzjxsspk3ssdmx8-bash-5.2-p15.drv
/nix/store/49dccsl44fyr6dvq9rwcgfr83girbyld-hello-2.12.1.tar.gz.drv
/nix/store/kh9inglxgaqj5098maivgy9iljbxpjww-stdenv-linux.drv
/nix/store/ip726rdk102f49mfkkkks81bqixrrqli-busybox-1.35.0.drv

After this PR:

$ nix-store --query --references (nix-store --query --deriver (nix build --print-out-paths --impure --expr 'with import ./. {}; hello.overrideAttrs(_: { disallowedReferences = [ busybox ]; })'))
/nix/store/6xg259477c90a229xwmb53pdfkn6ig3g-default-builder.sh
/nix/store/vqc7gh7myk6jpj642f9w2amygvazvjs6-bash-5.2-p15.drv
/nix/store/5ygyqwm9b47jfsxwck22fva8h15d50dg-stdenv-linux.drv
/nix/store/m58pdpyr80sais1bi36kicwc2bcd0s51-hello-2.12.1.tar.gz.drv

Note how in the first case busybox ends up as a build-time dependency, and will be built or substituted if it's not yet in the nix store. In the second case, with the changes in this PR, it no longer ends up as a build-time dependency and won't be built or substituted.

There is also an issue for nix itself to fix this on a lower level, where the approach implemented in this PR was suggested.
I don't agree with the conclusion on that issue and I think that it is a perfectly sensible use case to disallow references so that they cannot be accidentally included in the future and that we should not expect from our users to have to use unsafeDiscardStringContext to get the correct behaviour in that case.

Description of changes

Things done

Artturin · 2023-01-20T19:56:19Z

pkgs/stdenv/generic/make-derivation.nix

+    # knowing the out path is sufficient, and the out path can be calculated from
+    # the derivation without needing to build it, saving time and resources.


how does this interact with CA derivations

That should probably work (although a bit fragile) because drv.outPath will evaluate to a placeholder that will be rewritten just before the build if drv is part of the derivation closure. Which means that

If drv is part of the closure (because it's refered to somewhere else), then it will do the right thing (the unsafeDiscardStringContext will morally be a no-op)

Otherwise, the placeholder won't be rewritten, but it's fine since because the derivation isn't part of the build-time closure we know that it can't be in the runtime closure either

Artturin · 2023-01-20T20:00:46Z

should target staging https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#rebasing-between-branches-ie-from-master-to-staging and commit msg should follow the guide

r-vdp · 2023-01-20T20:37:47Z

@Artturin: right, I didn't know about staging, that should be fixed now.
I updated the commit message as well.

trofi

LGTM. I wonder if nix's derivation() builtin should already do the right thing. WDYT of filing a bug there? Sounds reasonable?

CA case is interesting. I think (hope) CA behaviour will not be changed by this context discard: our scanning operation does not need to pull in contents of checked derivations, but will still require building one to get the actual path for CA case. Thus I would guess this change will only help to non-CA builds. Which might be good enough. For CA-enabled builds such scans are probably already incorrect as there is no guarantee that build-CA has different store path from host-CA.

r-vdp · 2023-01-20T21:44:43Z

@trofi, agreed. Apparently there was an issue for nix already, but it got dismissed as not a bug, even though I think that the current behaviour is definitely surprising and unproductive and I don't think we should expect people to understand this intricacy and know about builtins.unsafeDiscardStringContext.

Regarding CA derivations, I must admit that I did not look into them enough to know how this would work in that case.

trofi · 2023-01-20T22:17:59Z

@trofi, agreed. Apparently there was an issue for nix already, but it got dismissed as not a bug, even though I think that the current behaviour is definitely surprising and unproductive and I don't think we should expect people to understand this intricacy and know about builtins.unsafeDiscardStringContext.

Good find! Worth adding a link to to the comment around builtins.unsafeDiscardStringContext?

Regarding CA derivations, I must admit that I did not look into them enough to know how this would work in that case.

I think it's OK to ignore it for now. We can always conditionally restore it back.

r-vdp · 2023-01-22T18:17:16Z

@trofi sure! I updated the description to include the reference, I didn't know about the issue when I made the PR.

Artturin · 2023-01-28T09:52:23Z

i added a link to the nix issue

Artturin · 2023-01-28T09:54:52Z

work for the future

__structuredAttrs supports per output closure checks

https://nixos.org/manual/nix/stable/release-notes/rl-2.2.html

outputChecks."out" = {
  # The closure of 'out' must not be larger than 256 MiB.
  maxClosureSize = 256 * 1024 * 1024;

  # It must not refer to C compiler or to the 'dev' output.
  disallowedRequisites = [ stdenv.cc "dev" ];
};

outputChecks."dev" = {
  # The 'dev' output must not be larger than 128 KiB.
  maxSize = 128 * 1024;
};

perhaps this could even be fixed in nix because there's not many users of structuredAttrs outputChecks yet

Artturin

built sway

thufschmitt

Thanks.

Adding this to stdenv makes a lot of sense since we can't really fix it in nixpkgs itself

thufschmitt · 2023-01-29T18:21:13Z

pkgs/stdenv/generic/make-derivation.nix

+    # knowing the out path is sufficient, and the out path can be calculated from
+    # the derivation without needing to build it, saving time and resources.


That should probably work (although a bit fragile) because drv.outPath will evaluate to a placeholder that will be rewritten just before the build if drv is part of the derivation closure. Which means that

If drv is part of the closure (because it's refered to somewhere else), then it will do the right thing (the unsafeDiscardStringContext will morally be a no-op)

Otherwise, the placeholder won't be rewritten, but it's fine since because the derivation isn't part of the build-time closure we know that it can't be in the runtime closure either

thufschmitt · 2023-01-29T18:22:50Z

pkgs/stdenv/generic/make-derivation.nix

+      disallowedReferences =
+        map unsafeDerivationToUntrackedOutpath (attrs.disallowedReferences or [ ]);
+      disallowedRequisites =
+        map unsafeDerivationToUntrackedOutpath (attrs.disallowedRequisites or [ ]);
+      allowedReferences =
+        lib.mapNullable unsafeDerivationToUntrackedOutpath (attrs.allowedReferences or null);
+      allowedRequisites =
+        lib.mapNullable unsafeDerivationToUntrackedOutpath (attrs.allowedRequisites or null);


Maybe (?) we could reduce the number of rebuilds by only adding these attributes if they were present if attrs

Hmm yes this introduces the attrs to all derivations

nix-repl> bash.allowedRe bash.allowedReferences bash.allowedRequisites nix-repl> bash.disallowedRe bash.disallowedReferences bash.disallowedRequisites

Also why do the allowed ref use or null

They use or null since that is the no-op value for those attrs, an empty list would mean that nothing is allowed in the output.

Agreed for the other point though, we should not add the attributes if they were not present to begin with. I'll push a fix for that tomorrow.

I pushed an update to fix this and to include the insights on the outPath in the comment.

thufschmitt · 2023-01-29T18:29:57Z

Apparently there was NixOS/nix#4629, but it got dismissed as not a bug, even though I think that the current behaviour is definitely surprising and unproductive and I don't think we should expect people to understand this intricacy and know about builtins.unsafeDiscardStringContext.

Nope. It is undoubtedly a bug, but a bug that we can't get rid of in the forseeable future because fixing it would change the hash of some derivations, which is a hard stop for Nix

Introduces unnecessary attrs to all derivations

trofi · 2023-01-29T19:26:53Z

Apparently there was NixOS/nix#4629, but it got dismissed as not a bug, even though I think that the current behaviour is definitely surprising and unproductive and I don't think we should expect people to understand this intricacy and know about builtins.unsafeDiscardStringContext.

Nope. It is undoubtedly a bug, but a bug that we can't get rid of in the forseeable future because fixing it would change the hash of some derivations, which is a hard stop for Nix

Would an opt-in flag into derivation() to enable new behaviour be an option? (like __contentAddressed)

thufschmitt · 2023-01-30T10:16:43Z

Would an opt-in flag into derivation() to enable new behaviour be an option? (like __contentAddressed)

It would. It would be less useful though since the main issue here is that it's fairly unintuitive, so people will likely overlook it until it actually bites.
Ideally we could warn if the only reference to a path is in one of these attributes, but that would be quite hard to implement because we don't really carry this information along.

Derivations listed as disallowedReferences or disallowedRequisites, currently end up as build-time dependencies. This is problematic since the disallowed derivations will be built by nix as build-time dependencies, while those derivations might take a very long time to build, or might not even build successfully on the platform used. However, in order to scan for disallowed references in the final output, knowing the out path is sufficient, and the out path can be calculated from the derivation without needing to build it, saving time and resources. While the problem is less severe for allowedReferences and allowedRequisites, since we want the derivation to be built eventually, we would still like to get the error early and without having to wait while nix builds a derivation that might not be used (e.g. if we listed the wrong one).

infinisil · 2023-01-31T17:25:24Z

I think this should really be fixed in Nix. With a proper deprecation strategy this should not be a problem. E.g. introduce a derivationSemantics attribute for derivation, defaulting to 0, but if it's explicitly set to 1 it will have the new behavior. Then to deprecate 0 and make 1 the default:

First give a warning when (derivationSemantics is unspecified or set to 0) and setting it to 1 would change semantics
In a future Nix version (once a NixOS release has passed), change this to an error, forcing everybody making use of the old semantics to set it to 1 or to not rely on them (with a builtins.unsafeDiscardStringContext)
Finally after more Nix and NixOS releases, change the default of derivationSemantics to 1, so that the new semantics are the default

If you want this to be more expressive you can also do something like __semantics.depCheckDeps = "include" | "exclude"

We should be brave enough to deprecate problematic behavior, otherwise these problems will never be fixed.

trofi · 2023-02-01T13:16:45Z

Noticed content-addressed failures on today's system build. Small reproducer against staging:

$ nix build -f. ruby --arg config '{ contentAddressedByDefault = true; }'
error: derivation contains an illegal reference specifier '/0mklx5bj004nxkcf7snqj2ssbnlj5s04fllg1gmdqjdrdhjyvz3r'
error: 1 dependencies of derivation '/nix/store/nq950lipshdnjnv3j64nj88x7xhbczxs-ruby-2.7.7.drv' failed to build

Did not look in detail yet. Will spend some time this evening to understand it unless someone gets earlier to it.

roberth · 2023-02-01T18:42:45Z

Finally after more Nix and NixOS releases, change the default of derivationSemantics to 1, so that the new semantics are the default

No, that's an unnecessary breaking change. stdenv.mkDerivation hides this from users, so there's virtually nothing to be gained from this break, yet we lose the ability to import ancient nixpkgs, which is one of our unique selling points.

We should be brave enough to deprecate problematic behavior, otherwise these problems will never be fixed.

Deprecate yes, remove no. A subtle difference until it punches you in the face while you're trying to get some old software to work.

If you want this to be more expressive you can also do something like __semantics.depCheckDeps = "include" | "exclude"

I think it'd be ok for supposedly easy changes like this to be linearized into a single __edition int and perhaps not even have an individual flag.
If we'd do this often and exclusively in the way you suggest, we'd end up with a bunch of unnecessary bloat in each .drv file. Not the end of the world, but it is a cost that is multiplied.

infinisil · 2023-02-01T19:34:08Z

Finally after more Nix and NixOS releases, change the default of derivationSemantics to 1, so that the new semantics are the default

No, that's an unnecessary breaking change. stdenv.mkDerivation hides this from users, so there's virtually nothing to be gained from this break, yet we lose the ability to import ancient nixpkgs, which is one of our unique selling points.

Fair enough. Instead then (and I think that's what you're implying), the default should be changed for just stdenv.mkDerivation so that most people have the benefits of the new semantics.

I think we should rethink this policy though, because not ever being able to deprecate old features really locks us in, requiring maintenance on code that may be only very rarely needed.

What if instead we were to not only specify a minimum support Nix version for nixpkgs (backwards compat), but also a maximum (forwards compat, perhaps with a date). Nixpkgs CI should make sure that it works with the minimum supported Nix version, and Nix CI should make sure it works with the earliest nixpkgs that supports its own version. This could be something like supporting Nix versions released around a NixOS release ± 5 years.

This does mean that we can't import arbitrary old nixpkgs versions anymore (there should be a check warning/erroring for that in nixpkgs), essentially limiting the time range of nixpkgs versions in a single Nix evaluation. But with a good error message we can present some workarounds:

Use an older Nix version, suggesting one that works (only works if you don't need to evaluate any super modern nixpkgs)
Show the semantic changes that happened between the newest supported version and the version you have, users can copy the expressions and update them according to those semantics if needed

This also very much relates to NixOS/rfcs#137, I'm abusing this issue as a scratch pad at this point 😅

Without the change build for packages that use `disallowedReferences` fails in `contentAddressedByDefault = true` mode: $ nix build -f. ruby_3_1 --arg config '{ contentAddressedByDefault = true; }' ... error: derivation contains an illegal reference specifier '/0j3hif3ni7zl5zhlzzr5q2q23z66136mnzp75pyiqp5c72q14im2' error: 1 dependencies of derivation '/nix/store/39ji7qp225pxvrm8cgvzmyjqsyis2n0h-ruby-3.1.2.drv' failed to build Original intent of NixOS#211783 was to avoid pulling in actual derivation for reference scanning purposes. Unfortunately CA derivations's outputPath are placeholders until they are instantiated. Let's restore string context for CA derivations for now.

trofi · 2023-02-01T20:51:59Z

Noticed content-addressed failures on today's system build. Small reproducer against staging:
$ nix build -f. ruby --arg config '{ contentAddressedByDefault = true; }'
error: derivation contains an illegal reference specifier '/0mklx5bj004nxkcf7snqj2ssbnlj5s04fllg1gmdqjdrdhjyvz3r'
error: 1 dependencies of derivation '/nix/store/nq950lipshdnjnv3j64nj88x7xhbczxs-ruby-2.7.7.drv' failed to build
Did not look in detail yet. Will spend some time this evening to understand it unless someone gets earlier to it.

Proposed restore of context just for CA derivations as #214044.

roberth · 2023-02-04T14:51:10Z

@infinisil I appreciate your care towards the Nix team, but "too much backcompat behavior" is not a problem I perceive in the Nix code.

I can build a 2013 zlib (using static = true to show that it actually builds; not just substitutes). We can probably build much older stuff, but 2013 is where the release branch names start.

$ nix-build -I nixpkgs=https://github.com/NixOS/nixpkgs/archive/release-13.10.tar.gz --expr 'with import <nixpkgs> {}; zlib.override(o: { static = true; })'

This capability can be a lifesaver if you have to deal with a really bad legacy binary in some academic, corporate or government environment or whatever. It's even useful for running old binary linux games. Being able to do this (with care of course) is uniquely valuable.

Use an older Nix version, suggesting one that works

This prevents installing anything old alongside anything new in a single expression.

Show the semantic changes that happened between the newest supported version and the version you have, users can copy the expressions and update them according to those semantics if needed

That's a lot of effort for very little gain (removing a couple of lines of code from Nix).

Furthermore, creating and implementing a policy for breaking stuff is also work. A bad kind of work that will make nobody happy.

Let's not make new problems if we don't have to. We can revisit this when compatibility becomes a proper burden, but until then, enjoy a working Nix and Nixpkgs.

r-vdp requested review from Ericson2314 and matthewbauer as code owners January 20, 2023 15:12

github-actions bot added the 6.topic: stdenv Standard environment label Jan 20, 2023

ofborg bot added 10.rebuild-darwin-stdenv 10.rebuild-linux-stdenv 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Jan 20, 2023

Artturin requested review from roberth, infinisil and trofi January 20, 2023 19:47

Artturin reviewed Jan 20, 2023

View reviewed changes

r-vdp force-pushed the fix_disallowed_references branch from 8db65db to ece8640 Compare January 20, 2023 20:34

r-vdp changed the base branch from master to staging January 20, 2023 20:34

r-vdp force-pushed the fix_disallowed_references branch from ece8640 to 1e5c2bf Compare January 20, 2023 20:36

r-vdp changed the title ~~Don't include drvs in disallowedRefs as build-time deps.~~ stdenv: don't include drvs in disallowedRefs as build-time deps. Jan 20, 2023

r-vdp force-pushed the fix_disallowed_references branch from 1e5c2bf to be21494 Compare January 20, 2023 20:54

trofi approved these changes Jan 20, 2023

View reviewed changes

r-vdp mentioned this pull request Jan 20, 2023

disallowedReferences/disallowedRequisites should not create a dependency on the disallowed derivations NixOS/nix#4629

Open

SuperSandro2000 added the 12.approvals: 1 label Jan 23, 2023

Artturin force-pushed the fix_disallowed_references branch from be21494 to bb2bc44 Compare January 28, 2023 09:51

Artturin previously approved these changes Jan 28, 2023

View reviewed changes

thufschmitt approved these changes Jan 29, 2023

View reviewed changes

r-vdp force-pushed the fix_disallowed_references branch from bb2bc44 to 15655db Compare January 30, 2023 16:48

r-vdp force-pushed the fix_disallowed_references branch from 15655db to 51acc62 Compare January 30, 2023 16:58

ofborg bot added 10.rebuild-darwin: 2501-5000 10.rebuild-linux: 2501-5000 and removed 10.rebuild-darwin-stdenv 10.rebuild-linux-stdenv 10.rebuild-darwin: 5001+ 10.rebuild-linux: 5001+ labels Jan 30, 2023

Artturin merged commit fa5f0d6 into NixOS:staging Jan 31, 2023

r-vdp deleted the fix_disallowed_references branch February 1, 2023 00:04

trofi mentioned this pull request Feb 1, 2023

stdenv: don't discard string context from ContentAddressed derivations #214044

Closed

13 tasks

tie mentioned this pull request Oct 3, 2023

{ruby,neovim}: fix build with content-addressed derivations #258705

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdenv: don't include drvs in disallowedRefs as build-time deps. #211783

stdenv: don't include drvs in disallowedRefs as build-time deps. #211783

r-vdp commented Jan 20, 2023 •

edited

Loading

Artturin Jan 20, 2023

thufschmitt Jan 29, 2023

Artturin commented Jan 20, 2023

r-vdp commented Jan 20, 2023

trofi left a comment

r-vdp commented Jan 20, 2023 •

edited

Loading

trofi commented Jan 20, 2023

r-vdp commented Jan 22, 2023

Artturin commented Jan 28, 2023

Artturin commented Jan 28, 2023 •

edited

Loading

Artturin left a comment

thufschmitt left a comment

thufschmitt Jan 29, 2023

thufschmitt Jan 29, 2023

Artturin Jan 29, 2023

r-vdp Jan 30, 2023 •

edited

Loading

r-vdp Jan 30, 2023

thufschmitt commented Jan 29, 2023

trofi commented Jan 29, 2023

thufschmitt commented Jan 30, 2023

infinisil commented Jan 31, 2023

trofi commented Feb 1, 2023

roberth commented Feb 1, 2023

infinisil commented Feb 1, 2023

trofi commented Feb 1, 2023

roberth commented Feb 4, 2023

		# knowing the out path is sufficient, and the out path can be calculated from
		# the derivation without needing to build it, saving time and resources.

stdenv: don't include drvs in disallowedRefs as build-time deps. #211783

stdenv: don't include drvs in disallowedRefs as build-time deps. #211783

Conversation

r-vdp commented Jan 20, 2023 • edited Loading

Description of changes

Things done

Artturin Jan 20, 2023

Choose a reason for hiding this comment

thufschmitt Jan 29, 2023

Choose a reason for hiding this comment

Artturin commented Jan 20, 2023

r-vdp commented Jan 20, 2023

trofi left a comment

Choose a reason for hiding this comment

r-vdp commented Jan 20, 2023 • edited Loading

trofi commented Jan 20, 2023

r-vdp commented Jan 22, 2023

Artturin commented Jan 28, 2023

Artturin commented Jan 28, 2023 • edited Loading

Artturin left a comment

Choose a reason for hiding this comment

thufschmitt left a comment

Choose a reason for hiding this comment

thufschmitt Jan 29, 2023

Choose a reason for hiding this comment

thufschmitt Jan 29, 2023

Choose a reason for hiding this comment

Artturin Jan 29, 2023

Choose a reason for hiding this comment

r-vdp Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

r-vdp Jan 30, 2023

Choose a reason for hiding this comment

thufschmitt commented Jan 29, 2023

trofi commented Jan 29, 2023

thufschmitt commented Jan 30, 2023

infinisil commented Jan 31, 2023

trofi commented Feb 1, 2023

roberth commented Feb 1, 2023

infinisil commented Feb 1, 2023

trofi commented Feb 1, 2023

roberth commented Feb 4, 2023

r-vdp commented Jan 20, 2023 •

edited

Loading

r-vdp commented Jan 20, 2023 •

edited

Loading

Artturin commented Jan 28, 2023 •

edited

Loading

r-vdp Jan 30, 2023 •

edited

Loading