@abbradar, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @ericsagnes and @nbp to be potential reviewers.

@abbradar , I am not sure to understand the use case, can you provide an example.

@nbp shell = toString (writeScript "foo" "....");. toString is needed because writeScript is not a "shell" derivation (it doesn't have shellPath attribute), so I need to indicate that I know what I'm doing. Without the patch even this is not possible, although type of users.extraUsers.*.shell (either shellPackage path) indicates as if it should be.

More elaborated, it doesn't work now because:

• shellPackage.check = x: (package.check x) && (hasAttr "shellPath" x) doesn't behave well with strings-paths from Nix store: it gets true from package.check and then throws on hasAttr. Instead I now only allow proper derivations as shellPackages;
• Similarly, toShellPath won't work because shellPath returns false but package returns true for such string so we get an exception again.

Would the following work:
shell = writeScriptBin "foo" "...." // { shellPath = "/bin/foo"; };

It should but either type still feels broken there (one can specify non-Nix store string "path" but not a Nix-store "path").

I actually have a similar use case in one of my configuration files:

  users.extraUsers.git = {
    uid = config.ids.uids.git;
    group = "git";
    description = "Git User";
    home = "/home/git";
    createHome = true;
    shell = pkgs.git + "/bin/git-shell";
    openssh.authorizedKeys.keyFiles = [ ...
      ./nicolas.computer.pub
    ];
  };

I think the proper reviewer for these changes should be @zimbatm

Hm, it's interesting that it works for your configuration without those patches. I'll look into this, maybe there's something deeper going on...

The shell option has the type

types.either types.shellPackage types.path;

Thus in my case, I get a nix-store path which is validated by the types.path.check function, while without the toString in front of the writeScript function call, this is not a valid path.

I see that multiple locations have the same type with either a shellPackage or a path. 2 things we can do:

• writeScript should add a shellPath attribute instead.
• we should include the path.check in the shellPackage.check function, and get rid of all these either cases.

the path type was to preserve legacy usage. if you have a better approach I'm all for it

@nbp Ah, I get it now, isStorePath is false for files/subdirectories in Nix path -- so it works for you with /bin but not for me with a plain path.

I'm for option (2) -- I'll look at implementing it somewhere this week.

Option (2) is just a clean-up, and not a solution, it would not solve the problem you are facing. Option (1) is from what I understand the proper way to fix this issue.

Hm, this seems a wrong place to fix the problem to me because I feel that either shellPackage path should, by its definition, allow any path-like string, and instead it just throws an exception in shellPackage.check.

having a shellPath attribute provided by writeScript makes total sense to me, as you are making a script, having its destination known by the user through the shellPath attribute or any other sounds reasonable.

@nbp That's okay, what I mean is that either the type should be shellPackage and shellPath should be required everywhere, or (if the type is left as is) arbitrary paths should be allowed, as in the type. And under no circumstances should check throw!

So, do we agree that shellPackage.check shouldn't throw? I propose fixing it so that it accepts only derivations with shellPath (that's the first patch) and then decide if shell option should accept arbitrary paths.

do we agree that shellPackage.check shouldn't throw?

Yes the check fucntion should in not throw, because we have other mechanism to report proper error messages on type failures.

I've made a new version; we now have a separate type stringPath which accepts only strings -- this way the code and my intent is much more clear.

I'm working on a third version of this patch and wonder -- why do we need to install shells globally? What is the downside of using direct Nix store paths?

It comes from /etc/shells (man 5 shells) that is supposed to contain the list of all authorised shells. Historically I think that it was introduced for chsh but it's possible other tools are also depending on it.

Ah, then it means that using shells which are not installed in system environment is a bad idea anyway! Then this PR is a bad idea as it is too -- let's close it. I'll document that shells need stable paths somewhere in comments later.