Use `subDir` not `src` to construct default names #506

hamishmack · 2020-03-21T05:11:30Z

When no name is provided to cleanSourceWith or cleanGit we
currently use baseNameOf src as a default.

This was cute, but it lead to cache misses. For instance if
x = cleanSourceWith { src = ./.; } then baseName src
will be different when src resolves to "/nix/store/X"
than when it is in a local directory. If people use
git worktrees they also may wind up with different
values for name. Anything that depends on x.name will
propagate the issue.

This change uses subDir if present or if it is not it encourages
adding a suitable name with:

A warning message.
A default name that gives a hint as to why there is no name.

When no name is provided to `cleanSourceWith` or `cleanGit` we currently use `baseNameOf src` as a default. This was cute, but it lead to cache misses. For instance if `x = cleanSourceWith { src = ./.; }` then `baseName src` will be different when `src` resolves to "/nix/store/X" than when it is in a local directory. If people use git worktrees they also may wind up with different values for `name`. Anything that depends on `x.name` will propagate the issue. This change uses `subDir` if present or if it is not it encourages adding a suitable `name` with: * A warning message. * A default name that gives a hint as to why there is no name.

michaelpj

I like it! I think we should consider making a PR like this to upstream nixpkgs. It'll probably be controversial, but I think it would be a clear improvement - this is a real new-user trap.

michaelpj · 2020-03-22T13:00:55Z

lib/clean-source-with.nix

+          if subDirName != ""
+            then if src ? name
+              then src.name + "-" + subDirName 
+              else "cleaned-" + subDirName


This is a bit circuitous: we have a default value for name (null), which we then test for in order to act as if it had a different value ("unnamed"), except sometimes when we use something else ("cleaned"). How about just changing the default value for name? i.e.

, name ? warnHere "unnamed"

I felt if you pass in subDir, but not name it should not warn you since it can derive what is likely to be a reasonable name from the subDir.

Eh, I'd be happy to just make people name it. But I can see the argument both ways.

I think it might soften the impact for people who liked the baseNameOf src default and used it carefully. They can go from:

cleanSourceWith { src = ./myDir; }

to:

cleanSourceWith { src = ./.; subDir = "myDir"; }

I prefer this to:

cleanSourceWith { src = ./myDir; name = "myDir"; }

I know at some point I would have a typo in the name or update the src and forget to update name as well.

Okay, I take your argument. But I would go for using "unnamed" as the prefix always just for consistency.

Good point. I've changed it so the prefix is "unnamed-${caller}-" (where caller will be "cleanSourceWith" or "cleanGit"). This matches the name given when we issue the warning. Now the only difference between:

cleanSourceWith { src = ./.; subDir = "myDir"; }

and

cleanSourceWith { src = cleanSourceWith { src = ./.; }; subDir = "myDir"; }

should be that the latter will issue a warning to the user about the lack of a name.

michaelpj · 2020-03-22T13:01:35Z

lib/clean-source-with.nix

@@ -44,6 +44,7 @@
  cleanSourceWith = { filter ? _path: _type: true, src, subDir ? "", name ? null }:
    let
      subDir' = if subDir == "" then "" else "/" + subDir;
+      subDirName = __replaceStrings ["/"] ["-"] subDir;


Nit, but I find it confusing that we use "" as the default value for subDir, but null as the default value for name...

Haskell types I was thinking of subDir :: FilePath and name :: Maybe String.

I wanted to avoid the use of null for subDir and since directory names cannot be empty it seems like a natural default. Since subDir works more or less like src + "/" + subDir I felt this was consistent with ls dir and ls dir/.

What I was going for with name is:

null : build a name from the subDir if it can (warning if it cannot).

"" : Use no name regardless of subDir and with no warning (might be useful if you always plan to filter sub directories out). For instance this should not produce a warning or include a "unnamed" in the name:

root = cleanSourceWith { src ./.; name = ""; filter = ... }; x = cleanSourceWith { src = root; subDir = "x"; }

I'd have thought subDir :: Maybe FilePath too, since it's optional, no? But yes, I guess the empty path is a reasonable default for subDir, I guess I'm just surprised that you then have a bunch of checks for subDir == "".

If we used null as the default I think we would still need the subDir == "" checks (or to transform "" to null). Otherwise passing "" would result in extra '/' characters in the path and the filter function would not work (we could canonicalise the path later, but I would prefer just to make it hard to have non canonical paths in the first place).

michaelpj · 2020-03-22T13:02:37Z

lib/clean-git.nix

@@ -102,7 +103,7 @@ then
    gitModules = builtins.path { name = "gitmodules"; path = gitModulesStr; };

    gitSubmoduleFiles = cleanSourceWith {
-      inherit src;
+      inherit name src;


I'd have thought you want different names for these, otherwise they're going to look confusingly like the main src. e.g. name = name + "-gitSubmoduleFiles"

michaelpj · 2020-03-23T09:54:00Z

I've opened an upstream PR for some of this: NixOS/nixpkgs#83201

michaelpj · 2020-03-23T09:55:24Z

Eelco points out that we should probably use "source" instead of "unnamed" in the case of a missing name.

NixOS/nix@65b5f17

hamishmack · 2020-03-24T22:29:09Z

bors try

iohk-bors · 2020-03-24T23:26:03Z

try

Build succeeded

hamishmack mentioned this pull request Mar 21, 2020

Add 'name' to cleanSource calls in tests #505

Merged

michaelpj reviewed Mar 22, 2020

View reviewed changes

hamishmack added 3 commits March 23, 2020 15:53

Improve drv names unnamed warning for cleanGit

0620593

Use unnamed when src has no name.

2dcf27e

Use unnamed-${caller} to be consistent

34a00cf

hamishmack added 2 commits March 23, 2020 23:07

s/unnamed/source

9d98d1c

Use just source as default name

66cfe7b

NixOS/nix@65b5f17

michaelpj approved these changes Mar 24, 2020

View reviewed changes

iohk-bors bot added a commit that referenced this pull request Mar 24, 2020

Try #506:

2daed10

hamishmack merged commit 0aa17d8 into master Mar 25, 2020

iohk-bors bot deleted the hkm/clean-names branch March 25, 2020 00:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `subDir` not `src` to construct default names #506

Use `subDir` not `src` to construct default names #506

hamishmack commented Mar 21, 2020

michaelpj left a comment

michaelpj Mar 22, 2020

hamishmack Mar 22, 2020

michaelpj Mar 22, 2020

hamishmack Mar 23, 2020

michaelpj Mar 23, 2020

hamishmack Mar 23, 2020

michaelpj Mar 22, 2020

hamishmack Mar 22, 2020

michaelpj Mar 22, 2020

hamishmack Mar 23, 2020

michaelpj Mar 22, 2020

hamishmack Mar 22, 2020

michaelpj commented Mar 23, 2020

michaelpj commented Mar 23, 2020

hamishmack commented Mar 24, 2020

iohk-bors bot commented Mar 24, 2020

Use subDir not src to construct default names #506

Use subDir not src to construct default names #506

Conversation

hamishmack commented Mar 21, 2020

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Mar 23, 2020

michaelpj commented Mar 23, 2020

hamishmack commented Mar 24, 2020

iohk-bors bot commented Mar 24, 2020

try

Build succeeded

Use `subDir` not `src` to construct default names #506

Use `subDir` not `src` to construct default names #506