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

builtins.filterSource does not work in flakes #3732

Closed
pasqui23 opened this issue Jun 23, 2020 · 14 comments · Fixed by #5163
Closed

builtins.filterSource does not work in flakes #3732

pasqui23 opened this issue Jun 23, 2020 · 14 comments · Fixed by #5163

Comments

@pasqui23
Copy link
Contributor

Is your feature request related to a problem? Please describe.
As discussed https://discourse.nixos.org/t/flakes-and-filtersource/7787
I have a flake with 2 outputs,a depends on b and b depends on ./.
In theory if I change the nix expression for a the b expression should not rebuild,but it still does as flake.nix is included in ./.
filterSource could remove flake from ./. and avoid rebuilding b,but evaluating filterSource in a flake keepgiving weird errors.

Describe the solution you'd like
For builtins.filterSource to work inside flake like outside them.

Describe alternatives you've considered
Just keeping the depency on ./. is not as bad as the flake still only includes the files tracked by the git repo,but still keep doing more rebuilds than I'd like when editing flake.nix

Additional context
Add any other context or screenshots about the feature request here.

@maisiliym
Copy link

With this bug, everyone can just forget trying to use a cargo-workspace flake. Li is beginning to get lost in the dozens of git repos he uses to avoid rebuilds everywhere. But nobody is going to split their big git projects in 7 different repos to use nix. This bug deserves top-priority.

@Kha
Copy link
Contributor

Kha commented Nov 4, 2020

Works for me.

$ git st
A  a
A  flake.lock
A  flake.nix

$ cat flake.nix
{
  description = "A very basic flake";

  outputs = { self, nixpkgs }: {

    packages.x86_64-linux.hello =
      with import nixpkgs { system = "x86_64-linux"; };
      stdenv.mkDerivation {
        name = "hello";
        src = builtins.filterSource (p: t: builtins.match ".*/a" p != null) ./.;
        installPhase = ''ls > $out'';
      };

    defaultPackage.x86_64-linux = self.packages.x86_64-linux.hello;

  };
}

$ nix build
warning: Git tree '/tmp/foo' is dirty

$ cat result
a

$ nix --version
nix (Nix) 3.0pre20200829_f156513

@maisiliym
Copy link

maisiliym commented Nov 17, 2020

The bug is easily reproducible. Here, systemdSors points directly to a flake. both systemdSors and systemdSors.outPath were tried, with same result;

error: --- EvalError ------------------------------------------------------------------------------------------------------------------------------------------------- nix
at: (63:9) in file: /nix/store/iqrvryccvj5ax4az2ch848yvmq332syn-source/default.nix

    62|   # This has proven to be less error-prone than the previous systemd fork.
    63|   src = builtins.filterSource (path: type:
      |         ^
    64|   builtins.baseNameOf path != "README.md") systemdSors.outPath;

string '/nix/store/6vpq5zbrkilpwjkfhcwgw52793nvcx67-source' cannot refer to other paths
❯ nix --version
nix (Nix) 3.0

@Kha 's exemple is not using the flake in the same manner, or perhapes, from nix's point of view, not at all.

Observation: Nix has strong build sandboxing, so it really should not matter if X 'refers to other paths'

@Kha
Copy link
Contributor

Kha commented Nov 17, 2020

If it's "easily reproducible", could you please give a self-contained example? It's not clear to me what you're doing here - filtering the output of a flake in another flake...?

@Kha 's exemple is not using the flake in the same manner, or perhapes, from nix's point of view, not at all.

Oh, it absolutely does. Try using ../. in a flake; Nix will stop you from escaping the flake.

Note that the original issue was about ./. as well, so your example might be out of scope for this issue. That filterSource cannot be used on store paths is not a new restriction of flakes, I believe.

@zimbatm
Copy link
Member

zimbatm commented Nov 17, 2020

I just wanted to leave a note that builtins.filterSource is deprecated by builtins.path. Maybe that works better for you, as a workaround.

@maisiliym
Copy link

@zimbatm Thanks, but it resulted in the same error.
@Kha ./. is not a flake, that's just a path. A flake is a full attribute set, which has an outPath attribute which is a path.

So it seems the problem is that nix can only filter the flake inside flake.nix. Said otherwise, the flake seems to only be considered a flake in a very limited context. A very common pattern in Li's flow now is to create a flake which wraps a 'non-flake flake' (input with attribute flake = false), which allows to 'flake-up' a repo without getting a PR accepted to merge it directly, especially since Li has an unconventional nix setup.

See the systemd wrapper flake he made if curious: https://github.com/li-maisiliym-uniks/systemd
Newer, unfinished exemple (qt/gtk deps undone), using crate2nix to wrap a cargo-workspace repo: https://github.com/li-maisiliym-uniks/ripasso/blob/main/flake.nix

Decomposing sources into smaller parts is very useful in a world where people routinely jam dozens of build targets in a single repo...

@Kha
Copy link
Contributor

Kha commented Nov 23, 2020

I see, turning non-flake repos into flakes while allowing for filterSource/path would be a feature I'd like to see as well. Perhaps #3920 (comment) could be used to patch flake.nix etc. into the repo...?

But again, this issue is about using filterSource in, not on a flake, i.e. on files below ./.

@maisiliym
Copy link

A very useful interface would be a filterTree builtin, to filter any (store) directory. If it could bind mount the files to get the hash or build, it could offer efficent storage and eval caching.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/builtins-filtersource-causes-rebuilds/12564/3

@tomberek
Copy link
Contributor

Possibly?
tomberek@d90582b

@ilkecan
Copy link
Member

ilkecan commented Sep 21, 2021

There might be some confusion about what this issue is about. There are two different problems regarding the usage of filterSource:

  1. Using filterSource filterFn ./. as the value of src in a flake doesn't work as expected, in that a rebuild is still triggered even when a filtered out file is changed. This is because how the directory created by filterSource is named is impure. Instead builtins.path should be used and the name should be explicitly defined. Even thought this is what is described in the linked Discourse, I don't think this issue is opened for this.
  2. Doing filterSource filterFn someFlakeInput (e.g. filterSource filterFn self) complains about string <nix store path of someFlakeInput> cannot refer to other paths. This is also true for builtins.path. Both check if the path the filter will be applied on contains any context and throw that error if it does.

While it could have been named better, I believe this issue is created for the second problem. It could be avoided by using builtins.unsafeDiscardStringContext on the directory before filtering it, but ideally this should be fixed within Nix unless there is a reason for this behaviour.

@pasqui23
Copy link
Contributor Author

@ilkecan it's for both actually, as both behaviors are surprising and undocumented.

@ilkecan
Copy link
Member

ilkecan commented Sep 22, 2021

I didn't really dive into Nix codebase but from my extensive testing the way filterSource names the new directory is impure and there is no way to change that. I am not sure if builtins.path is introduced to deprecate filterSource but the latter shouldn't be used in any case because of its impurity.

@ilkecan
Copy link
Member

ilkecan commented Sep 22, 2021

@pasqui23 I created a PR to document the unexpected behaviour in the first case.

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 a pull request may close this issue.

7 participants