NixOS / nix

Nix, the purely functional package manager
https://nixos.org/
GNU Lesser General Public License v2.1
12.27k stars 1.48k forks source link

Allow `builtins.storePath` in pure mode #5868

Open roberth opened 2 years ago

roberth commented 2 years ago

Is your feature request related to a problem? Please describe.

The storePath builtin is disallowed in pure mode. This hinders a use case for binary-only distribution via binary caches.

Describe the solution you'd like

storePath isn't impure. It's not even non-hermetic, because it specifies that a store path should exist, which is the same level of hermeticity that a fetcher provides (both built-in or FOD); for example that a git commit should exist.

Describe alternatives you've considered

Use Docker to distribute binary-only software that was originally built with Nix.

edolstra commented 2 years ago

It may not be impure, but it does make builds non-reproducible: it prevents rebuilding a flake unless you already have the store path or you have a substituter that can provide it. It's true that fetchers have the same problem, but they at least in principle provide a mechanism to get their output.

roberth commented 2 years ago

Indeed binary-only distribution is by definition not reproducible, as all relations to the source are intentionally stripped by the owner. It is the single constraint that defines this use case.

What if builtins.storePath or a similar primop had a parameter to suggest a substituter URL; would that put it on an equal footing with the fetchers?

tomberek commented 2 years ago

I consider storePath can be something like "fetchNar" or "fetchStorePathUsingNixMechanisms". It absolutely should be allowed for CA paths. The remaining question is what to do with non-CA paths, ie: non-deterministic output. I'd say this should be allowed as those paths are still signed and "morally" provide the same functionality.

Consuming something like https://github.com/NixOS/hydra/blob/master/src/lib/Hydra/View/NixExprs.pm#L30-L34 would be another use-case. While this mechanism can be abused it also opens up possibilities like pre-evaluating nixpkgs, binary distribution. Yes, it destroys build-time information, but sometimes that might be exactly the intention, or one can provide other mechanisms to provide that info (eg: can use a drvPath here )

Docs should suggest one not use this haphazardly and warn about the consequences.

What if builtins.storePath or a similar primop had a parameter to suggest a substituter URL; would that put it on an equal footing with the fetchers?

I'm not sure which way to go for this. We have several other places to put substituter information: nix.conf's, env var, CLI, flake nixConfig. This is a natural extension to specify it per-derivation. I'd suggest that it should only have the semantics "extra-substituters" if anything. But I'd be okay with it not being present or the empty-list to start with.

I started by removing that check, but it seems flake evaluation uses drvPath's to express what needs to be built.

roberth commented 2 years ago

I suppose the drvPath in BuiltPathBuilt could be made optional to support this. I appreciate that you want to go all the way with the use case, not just builtins.storePath itself, even in the experimental code, but I think it's ok to take small steps.

tomberek commented 2 years ago

I started implementing this, but must admit I started changing too much stuff and probably broke stuff I shouldn't. I am still interested and would like to collaborate on it. Also open to the "similar primop" idea if that is deemed more appropriate.

tomberek commented 2 years ago

Brainstorming: potentially provide a way to tie a substituter to the storePath, to make it similar to fetchers where one can lock the narHash from the perspective of a particular substituter (non-CA paths). My original idea was to leverage nixConfig.substituters at the flake.nix level, but that might not be granular enough?

Another approach: a new input type.

inputs.myPath = {
  type = "storePath";
  path = "/nix/store/HAHSFLSDKHFSDHFKSD";
  substituter = "https://cache.nixos.org";
};
roberth commented 2 years ago

Another approach: a new input type.

Implementation-wise, you'll still need some primop, as flake evaluation happens through call-flake.nix. I guess it could be a fetchTree variant instead of extending storePath, although that would put this feature under the flakes feature flag, fwiw.

leverage nixConfig.substituters at the flake.nix level, but that might not be granular enough?

Then it'd be similar to the status quo, but allowing storePath in pure mode. I'd be quite happy with just that.


In response to your earlier comment

We have several other places to put substituter information [...] This is a natural extension to specify it per-derivation.

Per-derivation is not a goal here, even if it is interesting. I believe it needs per-derivation meta info to be passed to libstore, which is not something we have, so that's quite a can of worms. (drv.meta is "erased" when using only outPath) Let's keep per-derivation out of scope and focus on store paths themselves. "Sources" in derivation terminology.

I'd suggest that it should only have the semantics "extra-substituters" if anything. But I'd be okay with it not being present or the empty-list to start with.

Yes, the regular substituters should be queried if the storePath-suggested one(s) fail. This allows the user to configure substitution via a reverse proxy with a different name for example.

tomberek commented 2 years ago

It may not be impure, but it does make builds non-reproducible: it prevents rebuilding a flake unless you already have the store path or you have a substituter that can provide it. It's true that fetchers have the same problem, but they at least in principle provide a mechanism to get their output.

I'm leaning toward the simpler approach and removing the restriction from the builtin. The loss of reproducibility would be identical compared to declaring any src via git/github/url/mirrors with regards to it being possible to be unable to fetch + reproduce the content if the source has changed/moved/permissions/etc. In some ways this would be more resilient due to substituters being utilized as the mechanism to get their output, they are similar to having mirrors available. The distinction is that other fetchers are normally locked with a content hash. I presume builtins.storePath could be used with CA-derivations as well, and this could be implemented in a mkFakeCADerivation function which would be distinct from the mkFakeNormalDerivation that relies upon the trust in a substituter to provide the correct output. [this is already trust we utilize].

ncfavier commented 1 year ago

This issue also prevents nixpkgs' replaceDependency from being used in pure mode: https://github.com/NixOS/nixpkgs/issues/199162.

I think it's a bit unfortunate that in order to get a dependency's reference graph we have to use a derivation with exportReferencesGraph and then import from that derivation, thereby destroying the local "proof" (string context) that the references exist in the store.

Is there any hope of making exportReferencesGraph a Nix builtin?

roberth commented 1 year ago

The binary distribution use case is solved by supporting input addressed path in pure mode in fetchClosure instead, without changing builtins.storePath.

@ncfavier:

This issue also prevents nixpkgs' replaceDependency from being used in pure mode: https://github.com/NixOS/nixpkgs/issues/199162.

to get a dependency's reference graph we have to use a derivation with exportReferencesGraph and then import from that derivation, thereby destroying the local "proof" (string context) that the references exist in the store.

This surprises me. Nix is pretty good at finding references to anything in the entire derivation graph; not just the input's outputs' closure as you might expect. It's why have https://github.com/NixOS/nix/pull/7087 instead of a rot13 hack to hide dependencies through an otherwise unnecessary derivation. So maybe I'm mistaken on that assumption?

Is there any hope of making exportReferencesGraph a Nix builtin?

I don't think you need output references, right? Just derivation references.

lf- commented 1 year ago

There's another use-case for builtins.storePath that is not available in pure evaluation mode.

Consider the following:

with import <nixpkgs> {};
writeTextDir "blah.nix" ''
{
  p = ./.;
}
''

This is basically the reduced version of what pkgs.path is, from inside of a nixpkgs copy in the store.

nix repl -f `nix-build test.nix`/blah.nix
nix-repl> p
/nix/store/4yv47alxkf1pgp1rmvph6ixwcpqyj6qv-blah.nix

nix-repl> :p builtins.getContext "${p}"
{ "/nix/store/8ii1f7dj23yas2bka8xqgvrhb587s9ii-4yv47alxkf1pgp1rmvph6ixwcpqyj6qv-blah.nix" = { path = true; }; }

nix-repl> :p builtins.getContext "${toString p}"
{ }

nix-repl> :p builtins.getContext "${builtins.path {path = p; name="blah.nix";}}"
{ "/nix/store/sibpj1qz7llir3g5qskkbgzsv8g2pkil-blah.nix" = { path = true; }; }

That is, there is no way of generating the path to the current file of nix source (which obviously is present) as a string, with context.

Motivating example:

Something that frustratingly cannot be done in nixpkgs of today is something like the following:

{config, pkgs, ...}:
{
# No! because of https://github.com/NixOS/nix/issues/7075. also because it copies nixpkgs again for no reason
# nix.nixPath = [ "nixpkgs=${pkgs.path}" ];
# No! because the string has no context, so the nixpkgs never gets onto a remote system if you do a remote build.
# nix.nixPath = [ "nixpkgs=${toString pkgs.path}" ];
# No! because you are a flake.
# nix.nixPath = [ "nixpkgs=${builtins.storePath pkgs.path}" ];
}

Yes, the pkgs.path can be yeeted in via an anonymous module in the flake.nix, but we cannot ship a generic and automatic version of this to users. This is sad! We should be able to do that: NixOS users deserve to be able to pin nixpkgs in the flake registry and in NIX_PATH without any funny incantations or anonymous modules.

It would be possible, since nixpkgs.lib.nixosSystem is a flakes only thing, to sneak a self reference in there, and do it for flakes, but it's annoying that this can't be generic for both kinds of systems, regardless of how nixpkgs is pinned.

Ericson2314 commented 1 year ago

I would like this as an alternative to https://github.com/NixOS/nix/pull/8963, making builtins.fetchClosure non-experimental.

I think builtins.fetchClosure is an awkward middle ground, trying to solve issues with builtins.storePath without "really" solving those problems. builtins.storePath might be strictly "worse", but it very simple and "honest" about its issues. builtins.fetchClosure is quite complex and still has very subtle security/correctness properties.

If we allow builtins.storePath in pure mode, we take the pressure off trying to stabilize builtins.fetchClosure soon, and give us time/breathing room to evolve the latter into something better (stronger guarantees and simpler).

roberth commented 1 year ago

The biggest risk is that due to indecisiveness we bother users with unnecessary fear, uncertainty and doubt until we finish all the other things. Because that's what should happen. In the big picture we have for the foreseeable future, solving the problems @Ericson2314 indicates is just not a priority, yet still a significant expenditure of effort.

I'm ok with either approach, but if in doubt pick the simpler one. Above all pick one though, because neglecting this functionality is really wasteful.

manveru commented 12 months ago

I also wouldn't mind if we do this first, I mostly opted for builtins.fetchClosure because it works in pure mode already and I have no idea what's involved in making this happen for builtins.storePath.

roberth commented 11 months ago

Use case: simplify source based deployment script expression

I'm (again) running into this issue, but now while generating a deployment script which needs a derivation closure for a string of bash statements, to be built and run on a remote host. I.e. this is for source-based deployment.

Solution using builtins.storePath

I'd like to avoid writing the bash statements to a script file, in order to keep the function as general as possible, so the way to get the derivation closure of the string is through builtins.getContext, which ironically does not have a context for its drv paths, because they occur as attribute names only - which can not have context. With builtins.storePath I would be able to fix that by myself (which I'm happy to do as a library developer), but pure mode stops me from doing this.

Alternate solution with new primop

Add a builtin that does the whole thing and works in pure mode. builtins.getContextDerivations. Its main requirement is that the following bashStatements constitute a working script (ignoring gc roots for simplicity of example):

bashStatements =
''
  nix-store -r ${builtins.getContextDerivations string}
  ${builtins.unsafeDiscardContext string}
''

While this seems to suit my requirements rather well, it would still require a polyfill for older Nix versions (using storePath), and it just seems a little ad hoc to do it this way. For instance, this solution does not seem to work for Nixpkgs replace-dependencies.nix, if I understand correctly.

Workaround

Without suitable builtins that work in pure mode, the only option I see is to generate a writeText that can be built in the remote, and then source its output. For this I have to complicate the deployment function to include a parameter for a Nixpkgs that is executable on the remote, which so far I've been able to avoid when doing a binary deployment (as opposed to source based deployment, which is my current goal).

Ericson2314 commented 11 months ago

I didn't immediate understand this, but then I talked to @roberth and now I get it.

builtins.storePath, builtins.outputOf, and (unmerged, in https://github.com/NixOS/nix/pull/8595) builtins.addDrvOutputDependencies are the 3 low level ways to make every kind of string context element. All 3 should work in pure mode, and all 3 should be used in the documentation for string contexts (which doesn't yet exist!) to make clear what is going on.

roberth commented 11 months ago

Improve getContext

As yet another alternative solution, we could improve getContext itself to return a string with context inside the attribute value. However, like "Alternate solution with new primop" this doesn't help with the replaceDependencies function in Nixpkgs.

 nix-repl> :p builtins.getContext "${hello}"
 { "/nix/store/6yz3c2v1bzrlwbyn9xzh832chwna3dh2-hello-2.12.1.drv" = {
   outputs = [ "out" ];
+  drvPath = "/nix/store/6yz3c2v1bzrlwbyn9xzh832chwna3dh2-hello-2.12.1.drv";  # having context, as appropriate for the context item type
 }; }

Output paths could be recovered with builtins.outputsOf, so they don't seem as necessary (and these serve a different use case than what I was describing just now).

enobayram commented 10 months ago

It may not be impure, but it does make builds non-reproducible:

@edolstra would it be feasible to relax this constraint in cases where we know that it won't make builds non-reproducible. The particular case I have in mind is IFD: When I import from a derivation, I know that I will trigger the realization of that derivation so that all of its runtime closure will be in my /nix/store by the time the content is available to my Nix interpreter. Can we not keep a set of /nix/store paths like this that are known to be available so that they are allowed to be passed into builtins.storePath in pure mode?

I believe this makes vanilla IFD more flexible by itself, but I've actually run into dead ends because of this when recursive-nix enters the picture. In particular, it's very tempting to write a recursive-nix derivation that evaluates some expensive Nix expressions (possibly expensive due to the IFD in them and in particular the build-time closure of the IFD, which needs to be fetched) and then writes the results to its output. This works well enough for the most part, but the lack of builtins.storePath in pure mode makes it impossible to return values containing references to other /nix/store paths built as part of the recursive derivation. This is unfortunate, because in this use case, it's impossible for builtins.storePath to become a reproducibility issue, since the paths in question are already in the closure of the derivation we're IFD'ing from.

nixos-discourse commented 10 months ago

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-11-20-nix-team-meeting-minutes-105/35902/1

thufschmitt commented 10 months ago

Discussed in the Nix maintenance meeting. We couldn't manage to reach an agreement, so this is currently in a limbo.

dweee commented 2 months ago

Hi all,

I've been tackling trying to get glibc patched on a NixOS system without having to recompile all derivations/packages as every update will cause a measurable amount of downtime/poor performance on the system and without having to sacrifice the pure essence of Nix. Thanks to help from a few peeps on Discord and Matrix, I have therefore gone for the below example. Unfortunately, this results in the error ┃ error: 'builtins.storePath' is not allowed in pure evaluation mode, which has led me to this issue. I am currently using --impure to get around this and laughbly the patch is not even working. Unsure if this is a user issue due to it working on other distros, but that's out the scope of this issue. If anyone has any suggestions on how to handle this, please let me know. Sorry for not being as in depth as other Nix users, I am still new to the Nix{,OS} ecosystem when it comes to this sort of stuff. Although this is a rare issue someone could encounter, it could be make or break for users trying to replace older/embedded stacks to run on NixOS due to patches required to core system packages, or people trying to hack away at new features I believe.

  system.replaceRuntimeDependencies = [
    {
      original = pkgs.glibc;
      replacement = pkgs.glibc.overrideAttrs {
        patches = [
          (import <nix/fetchurl.nix> {
            url = "[REDACTED]";
            hash = "[REDACTED]";
          })
        ];
      };
    }
  ];
tomberek commented 2 months ago

Replacing glibc is possible. Needs a few tricks: https://github.com/NixOS/bundlers/issues/18#issuecomment-2191644756

In that case I'm only adding a file, but something similar would help with your override.