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

Basename of current directory goes into derivation for filterSource on ./. #1305

Closed
nh2 opened this issue Apr 6, 2017 · 13 comments
Closed
Assignees

Comments

@nh2
Copy link
Contributor

nh2 commented Apr 6, 2017

This is a move-over of NixOS/nixpkgs#24575 from nixpkgs, which turned out to be a nix issue.

When you use filterSource (...) ./., nix puts the basename of the current dir, that is, the name of the parent directory, into the derivation.

That is problematic because that could be gnu-hello or gnu-hello-myclone or /tmp/asdf123 etc (see #904 for more such problems).

I think that filterSource should not rely on the basename here.

The easiest solution would be to just use the hash and not try to make up a nice extra name.

@domenkozar
Copy link
Member

domenkozar commented May 1, 2017

@edolstra what would be the implications of name not being part of the store path hash?

This would help with determinism for many Nix functions while keeping the UX.

@edolstra
Copy link
Member

edolstra commented May 1, 2017

Well, filterSource is consistent with the behaviour of regular (unfiltered) sources BTW. If you write builder = ./builder.sh, you get a store path name /nix/store/...-builder.sh.

@bjornfor
Copy link
Contributor

bjornfor commented May 1, 2017

If you write builder = ./builder.sh, you get a store path name /nix/store/...-builder.sh

That behaviour cause problems from time to time, due to the hash being prepended to the filename. I wonder how it would be if it produced /nix/store/...-builder.sh/builder.sh instead. It looks both ugly and pretty at the same time. Ugly because of the seemingly pointless duplicated name, and pretty because now the basename is the same inside and outside the Nix store.

@domenkozar
Copy link
Member

domenkozar commented May 1, 2017

This adds to "a list of obstacles that makes hash dependent based on how the source is retrieved" in #904

@nh2
Copy link
Contributor Author

nh2 commented May 1, 2017

Would anybody be opposed to changing filterSource so that it contains only the hash, and adding a separate function like filterSourceWithName where you can give a name explicitly if you want the ...hash...-name format?

@domenkozar
Copy link
Member

@nh2 I don't think so.

@nh2
Copy link
Contributor Author

nh2 commented Oct 29, 2017

For drive-by readers, the pain created by this bug is explained in detail in @domenkozar's talk at NixCon 2017.

@domenkozar
Copy link
Member

Ugly workaround for now might be fetchGit ./., but not sure how to get that working with filterSource.

@shlevy
Copy link
Member

shlevy commented Apr 19, 2018

You can use builtins.path for this now.

@shlevy shlevy closed this as completed Apr 19, 2018
@domenkozar
Copy link
Member

Note: if you want to do something like nix-env -if <url.tar.gz>, it will also have a different hash than locally doing just nix-build on that unpacked folder.

To circumvent that use builtins.path as mentioned by @shlevy , minimal example: https://gist.github.com/domenkozar/766c926b6a4427d3828a0ca9600ca6cb

$ nix-env -if https://gist.github.com/domenkozar/766c926b6a4427d3828a0ca9600ca6cb/archive/1ae25b20e8ffa1fe507990b1346f4acf82b127af.tar.gz

will have the same hash as nix-build on that file anywhere locally.

@nh2
Copy link
Contributor Author

nh2 commented Aug 24, 2018

Can somebody provide in here an example of how builtins.path should be used for avoiding the basename problem?

We should proably also mention that in the manuals.

@nh2
Copy link
Contributor Author

nh2 commented Aug 24, 2018

Can somebody provide in here an example of how builtins.path should be used for avoiding the basename problem?

Examples:

{
  src = builtins.path { name = "stack2nix"; path = ./.; };
}

With filter:

{
  src = builtins.path {
    name = "stack2nix";
    path = ./.;
    filter = path: type:
      !(pkgs.lib.hasPrefix "." (baseNameOf path));
  };
}

nomeata added a commit to dfinity/motoko that referenced this issue Mar 25, 2019
this way, we get consistent store paths independent of wether we have
the sources locally, or get them via `fetchGit`.

See NixOS/nix#1305
nomeata added a commit to dfinity/motoko that referenced this issue Mar 25, 2019
this way, we get consistent store paths independent of wether we have
the sources locally, or get them via `fetchGit`.

See NixOS/nix#1305
@roberth
Copy link
Member

roberth commented Sep 8, 2019

This functionality is now available in nixpkgs master via lib.cleanSourceWith. Example:

lib.cleanSourceWith {
  name = "source";
  src = ./.; # or a cleanSourceWith output
  filter = path: type:
    !(pkgs.lib.hasPrefix "." (baseNameOf path));
}

or to just rename an already cleaned source:

lib.cleanSourceWith { name = "source"; src = someSource; }

michaelpj added a commit to michaelpj/nixpkgs that referenced this issue Mar 23, 2020
Currently, not providing `name` to `cleanSourceWith` will use the name
of the imported directory. However, a common case is for this to be the
top level of some repository. In that case, the name will be the name of
the checkout on the current machine, which is not necessarily
reproducible across different settings, and can lead to e.g. cache
misses in CI.

This is documented in the comment on `cleanSourceWith`, but this does
not stop it being a subtle trap for users.

There are different tradeoffs in each case:

1. If `cleanSourceWith` defaults to `"source"`, then we may end up with a
user not knowing what directory a source store path corresponds to.
However, it being called "unnamed" may give them a clue that there is a
way for them to name it, and lead them to the definition of the
function, which has a clear `name` parameter.

2. If `cleanSoureWith` defaults to the directory name, then a user may face
occasional loss of caching, which is hard to notice, and hard to track
down. Tracking it down likely requires use of more advanced tools like
`nix-diff`, and reading the source of a lot of nix code.

I think the downside of the status quo is worse.

This is really another iteration of
NixOS/nix#1305: that led to adding the `name`
argument in the first place, this just makes us use a better default
`name`.
github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this issue Dec 4, 2022
Currently, not providing `name` to `cleanSourceWith` will use the name
of the imported directory. However, a common case is for this to be the
top level of some repository. In that case, the name will be the name of
the checkout on the current machine, which is not necessarily
reproducible across different settings, and can lead to e.g. cache
misses in CI.

This is documented in the comment on `cleanSourceWith`, but this does
not stop it being a subtle trap for users.

There are different tradeoffs in each case:

1. If `cleanSourceWith` defaults to `"source"`, then we may end up with a
user not knowing what directory a source store path corresponds to.
However, it being called "unnamed" may give them a clue that there is a
way for them to name it, and lead them to the definition of the
function, which has a clear `name` parameter.

2. If `cleanSoureWith` defaults to the directory name, then a user may face
occasional loss of caching, which is hard to notice, and hard to track
down. Tracking it down likely requires use of more advanced tools like
`nix-diff`, and reading the source of a lot of nix code.

I think the downside of the status quo is worse.

This is really another iteration of
NixOS/nix#1305: that led to adding the `name`
argument in the first place, this just makes us use a better default
`name`.
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

No branches or pull requests

7 participants