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

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

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

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

i added a link to the nix issue

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

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)

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.

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.

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.

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

• 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 😅

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.

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