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.

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

Allow use of path and filterSource in flakes #5163

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

tomberek
Copy link
Contributor

@tomberek tomberek commented Aug 23, 2021

Using builtins.filterSource and/or builtins.path inside of flakes doesn't quite work as expected at the moment. Is this fundamentally a bad idea? There are a few issues about this, but nothing conclusive. This patch allows it, but i have no idea if this is breaking some invariant.

This part seems odd, but seems to be needed:

state.realiseContext({});

Discourse
Discourse2
Fixes #3732
Fixes #2538

As filterSource and path perform work, add paths to allowedPaths.
@Mazurel
Copy link
Member

Mazurel commented Aug 28, 2021

I have made a simple repo to test lib.cleanSource issue, and it seems that this patch have fixed it.

I do not really know Nix codebase well, although the code looks good to me. The state.realiseContext({}); should not do anything from what I understand, but based on my tests it is required indeed.

@ilkecan
Copy link
Member

ilkecan commented Sep 22, 2021

I think the problems in linked/mentioned issues/topics can be categorized into three:

1. Usage of builtins.filterSource doesn't prevent a rebuild when a file that didn't pass the filter is updated

  • This is because filterSource is inherently impure and I think it should be deprecated in favour of builtins.path.
  • The change in this PR is unrelated to this.
  • Both discourse topics belong to this category.

2. Using a filter (filterSource or path) on a string with a context result in string '<string>' cannot refer to other paths

  • Main implication of this is that the flake inputs can not be filtered, which I think is a huge problem. This can currently be avoided with builtins.unsafeDiscardStringContext but ideally it should be fixed within Nix if possible.
  • This PR doesn't have any effect on this.
  • builtins.filterSource does not work in flakes #3732 (I think) belongs to this category.

3. Trying to access a file in the filtered (using filterSource or path) directory when restrict-eval is enabled results in access to path '<path>' is forbidden in restricted mode

@edolstra edolstra added this to the nix-2.4 milestone Sep 22, 2021
@tomberek
Copy link
Contributor Author

There are several motivating use-cases. One significant one is when developing an "internal flake" (ie: one where the flake.nix is in the same repo as the source code, and someone is probably using src = ./.; or some variant of that) and modifying the flake.nix invalidates the cached package builds. It would be convenient to filter these cases to avoid repeated rebuilds when the relevant source for the packages is not changing.

Both path and filterSource use the same underlying mechanism:
https://github.com/NixOS/nix/blob/master/src/libexpr/primops.cc#L1915
https://github.com/NixOS/nix/blob/master/src/libexpr/primops.cc#L2002

Problem # 1

Some of the issues with filterSource is why path was introduced. But i'm not clear on exactly which impurity you (@ilkecan) are talking about.

The baseNameOf default is present in both, but path allows you to set it to remove that impurity. One possibility is to have the default for a directory to use something like "source" instead of baseNameOf?

Problem # 2

Likely the difference is between using a path and string-with-context, common idioms being:

  • ./. + "/some-dir"
  • "${some-src}/some-dir}"

Both use baseNameOf(path) by default. Both path and filterSource check if context is empty:
https://github.com/NixOS/nix/blob/master/src/libexpr/primops.cc#L1900
https://github.com/NixOS/nix/blob/master/src/libexpr/primops.cc#L1971

Should this check also check if the context's paths are in state.allowedPaths rather than just checking if it is empty? (@edolstra ?)

Problem # 3

If this fixes it: 🎉

@ilkecan
Copy link
Member

ilkecan commented Sep 22, 2021

The impurity of filterSource I was referring to was the one you are talking about in the next sentence. I created a PR to add a warning about the usage of filterSource to avoid # 1.

# 2 is about not being able to filter a string with contexts in general. Nix complains since the flake input contains a context. That check might normally be necessary for other cases but I think we should be able to filter a nix input. Your idea to check state.allowedPaths sounds interesting.

ilkecan added a commit to ngi-nix/studio that referenced this pull request Sep 26, 2021
Source filtering isn't possible right now, due to npmlock2nix reading
the lock file. Follow up on how NixOS/nix#5163
will be resolved.
ilkecan added a commit to ngi-nix/studio that referenced this pull request Sep 26, 2021
Source filtering isn't possible right now, due to npmlock2nix reading
the lock file. Follow up on how NixOS/nix#5163
will be resolved.
ilkecan added a commit to ngi-nix/studio that referenced this pull request Sep 26, 2021
Source filtering isn't possible right now, due to npmlock2nix reading
the lock file. Follow up on how NixOS/nix#5163
will be resolved.
@ilkecan
Copy link
Member

ilkecan commented Sep 30, 2021

Even with this PR, trying to import a file accessed through ${pkgs.path} produces access to path '<path>' is forbidden in restricted mode.

This can be tested by doing nix develop with the following flake:

{
  inputs = {
    nixpkgs.url = "nixpkgs/nixos-unstable";
  };

  outputs = { self, nixpkgs, ... }@inputs: {
    devShell.x86_64-linux =
      let
        pkgs = import nixpkgs { system = "x86_64-linux"; };
      in
      pkgs.mkShell {
        shellHook =''
          echo "${(pkgs.callPackage "${pkgs.path}/pkgs/development/misc/resholve" {}).resholveScript}"
        '';
      };
  };
}

Expected error
error: cannot coerce a function to a string
Actual error
error: access to path '/nix/store/jma6rgslg0pyx3h3r8viymf4l2zqp0xx-7jc0a6qq2w4xjj4p61mspp4s54yniz6m-source/pkgs/development/misc/resholve' is forbidden in restricted mode

Note that ${pkgs.path} should probably not be used in this case anyway since Nix will unnecessarily move the directory to another store path. Instead, one of the following should be used

  • ${toString pkgs.path}
  • ${nixpkgs}

@edolstra edolstra merged commit c497fce into NixOS:master Oct 6, 2021
@edolstra
Copy link
Member

edolstra commented Oct 6, 2021

Thanks, I've merged this without the realiseContext call which should be a no-op and it seems to work without it.

@edolstra
Copy link
Member

edolstra commented Oct 7, 2021

@ilkecan I've fixed the ${pkgs.path} case in 972405e. But you're right that it's better not to do it this way because it causes an unnecessary copy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

builtins.filterSource does not work in flakes filterSource is disallowed in restrict-eval
4 participants