environment.shellAliases: change default behavior #44441

mnacamura · 2018-08-04T03:53:37Z

Do not override environment.shellAliases in nixos/modules/programs/shell.nix.
programs.{bash,zsh,fish}.shellAliases override environment.shellAliases for each shell.

Motivation for this change

Hopefully improve the situation in #36282.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

FRidh · 2018-08-04T06:57:25Z

nixos/modules/config/shells-environment.nix

@@ -107,7 +107,11 @@ in
    };

    environment.shellAliases = mkOption {
-      default = {};


Are these defaults intentional?

I just moved these aliases that were in nixos/modules/programs/shell.nix to here without any specific reason. Empty default is fine or better I think though.

Made default environment.shellAliases empty.

Note it was just a remark. I have no opinion really on the matter.

Never mind. It just reflected my preference.

Reverted the default aliases. See #44441 (comment)

mnacamura · 2018-08-04T10:15:17Z

Made default environment.shellAliases empty.

infinisil · 2018-10-11T19:43:33Z

nixos/modules/programs/shell.nix

-        ll = "ls -l";
-        l  = "ls -alh";
-      };
-


Not entirely sure about removing those, they have been there for a long time. I don't personally use them, but maybe some people do. Do other distros have any default aliases?

Also, the cost of keeping these around with the changes in this PR isn't very high, so we might as well just leave them in for the sake of not annoying anybody.

Okay. Moved them to the default of environment.shellAliases.

infinisil · 2018-10-11T19:47:40Z

nixos/modules/programs/bash/bash.nix

@@ -33,7 +33,7 @@ let
  '';

  bashAliases = concatStringsSep "\n" (
-    mapAttrsFlatten (k: v: "alias ${k}=${escapeShellArg v}") cfg.shellAliases
+    mapAttrsFlatten (k: v: "alias ${k}=${escapeShellArg v}") (cfge.shellAliases // cfg.shellAliases)


I think a better way to implement this is to have

{ config.programs.bash.shellAliases = mapAttrs (name: mkDefault) cfge.shellAliases; }

This should then have the // override behavior automatically through the module system. Also this allows the user to query all bash aliases by evaluating the option.

Hmm... By your change, although defined in /etc/nixos/configuration.nix, bash aliases that have the same names as defined in environment.shellAliases seem not appearing in programs.bash.shellAliases.

infinisil · 2018-10-12T11:58:35Z

Ah yeah, there's a problem with the original type. This works:

diff --git a/nixos/modules/config/shells-environment.nix b/nixos/modules/config/shells-environment.nix
index 5ecc3cfd207..a279120e125 100644
--- a/nixos/modules/config/shells-environment.nix
+++ b/nixos/modules/config/shells-environment.nix
@@ -108,18 +108,13 @@ in
     };
 
     environment.shellAliases = mkOption {
-      default = {
-        ls = "ls --color=tty";
-        ll = "ls -l";
-        l  = "ls -alh";
-      };
       example = { ll = "ls -l"; };
       description = ''
         An attribute set that maps aliases (the top level attribute names in
         this option) to command strings or directly to build outputs. The
         aliases are added to all users' shells.
       '';
-      type = types.attrs; # types.attrsOf types.stringOrPath;
+      type = with types; attrsOf (either str path);
     };
 
     environment.binsh = mkOption {
@@ -161,6 +156,12 @@ in
     # terminal instead of logging out of X11).
     environment.variables = config.environment.sessionVariables;
 
+    environment.shellAliases = mapAttrs (name: mkDefault) {
+      ls = "ls --color=tty";
+      ll = "ls -l";
+      l  = "ls -alh";
+    };
+
     environment.etc."shells".text =
       ''
         ${concatStringsSep "\n" (map utils.toShellPath cfg.shells)}
diff --git a/nixos/modules/programs/bash/bash.nix b/nixos/modules/programs/bash/bash.nix
index 62c63f2aeb3..aa524a333ee 100644
--- a/nixos/modules/programs/bash/bash.nix
+++ b/nixos/modules/programs/bash/bash.nix
@@ -33,7 +33,7 @@ let
   '';
 
   bashAliases = concatStringsSep "\n" (
-    mapAttrsFlatten (k: v: "alias ${k}=${escapeShellArg v}") (cfge.shellAliases // cfg.shellAliases)
+    mapAttrsFlatten (k: v: "alias ${k}=${escapeShellArg v}") cfg.shellAliases
   );
 
 in
@@ -64,7 +64,7 @@ in
           Set of aliases for bash shell, which overrides <option>environment.shellAliases</option>.
           See <option>environment.shellAliases</option> for an option format description.
         '';
-        type = types.attrs; # types.attrsOf types.stringOrPath;
+        type = with types; attrsOf (either str path);
       };
 
       shellInit = mkOption {
@@ -125,6 +125,8 @@ in
 
     programs.bash = {
 
+      shellAliases = mapAttrs (name: mkDefault) cfge.shellAliases;
+
       shellInit = ''
         if [ -z "$__NIXOS_SET_ENVIRONMENT_DONE" ]; then
             . ${config.system.build.setEnvironment}

(the same for fish and zsh)

mnacamura · 2018-10-12T13:47:23Z

The above patch works. Thank you!

Another problem that concerns me is how to remove the default aliases defined in environment.shellAliases. I mean, for example, how to remove l="ls -alh" if it is not necessary.

infinisil · 2018-10-12T13:49:52Z

Yeah, usually that's done by allowing null values (prepending the type with nullOr) and handling that as non-presence when generating. Feel free to add support for that as well.

mnacamura · 2018-10-13T15:00:38Z

All set.

infinisil · 2018-10-13T15:03:05Z

nixos/modules/config/shells-environment.nix

-      default = {};
+      default = mapAttrs (name: mkDefault) {
+        ls = "ls --color=tty";
+        ll = "ls -l";


I don't think this should work like that. It needs to be in the config section. How it's currently will disable all default aliases if a user adds any of their own.

Oh, right. Sorry.

…ases

mnacamura · 2018-10-13T15:23:46Z

This time for sure.

infinisil · 2018-10-13T15:42:33Z

This changes the following:

programs.{bash,zsh,fish}.shellAliases take defaults from environment.shellAliases, which won't disappear if you set any custom alias.
Defaults for environment.shellAliases are l, ll and ls, you can override these by setting enviromnent.shellAliases.{l,ll,ls} for all shells, or programs.{bash,zsh,fish}.shellAliases.{l,ll,ls} for just a specific one.
You can unset aliases by setting the value to null

Solves all issues mentioned in #36282

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Aug 4, 2018

FRidh reviewed Aug 4, 2018

View reviewed changes

mnacamura force-pushed the shell-aliases branch from a69e3db to b59e790 Compare August 4, 2018 08:38

mnacamura force-pushed the shell-aliases branch from b59e790 to 0be12c9 Compare August 12, 2018 13:21

mnacamura force-pushed the shell-aliases branch from 0be12c9 to 0e5358f Compare October 8, 2018 00:24

infinisil requested changes Oct 11, 2018

View reviewed changes

mnacamura force-pushed the shell-aliases branch from 0e5358f to e604566 Compare October 12, 2018 03:51

mnacamura force-pushed the shell-aliases branch 3 times, most recently from c75c0be to 8c1092b Compare October 13, 2018 14:53

infinisil reviewed Oct 13, 2018

View reviewed changes

mnacamura added 3 commits October 14, 2018 00:13

nixos/shells: do not override user-defined shell aliases

e4e160c

nixos/shells: programs.*sh.shellAliases override environment.shellAli…

3b5449b

…ases

nixos/shells: enable to nullify already defined aliases

c941577

mnacamura force-pushed the shell-aliases branch from 8c1092b to c941577 Compare October 13, 2018 15:16

infinisil approved these changes Oct 13, 2018

View reviewed changes

infinisil merged commit d4f2f4c into NixOS:master Oct 13, 2018

infinisil mentioned this pull request Oct 13, 2018

environment.shellAliases not honored for ls #36282

Closed

mnacamura deleted the shell-aliases branch October 14, 2018 05:34

TomSmeets mentioned this pull request Oct 14, 2018

nixos/environment: make default shell aliases overridable #28072

Closed

8 tasks

TomSmeets mentioned this pull request Oct 14, 2018

Some user-set aliases get ignored #24560

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

environment.shellAliases: change default behavior #44441

environment.shellAliases: change default behavior #44441

mnacamura commented Aug 4, 2018 •

edited

FRidh Aug 4, 2018

mnacamura Aug 4, 2018

FRidh Aug 4, 2018

mnacamura Aug 4, 2018

mnacamura Oct 12, 2018

mnacamura commented Aug 4, 2018

infinisil Oct 11, 2018

mnacamura Oct 12, 2018 •

edited

infinisil Oct 11, 2018

mnacamura Oct 12, 2018

infinisil commented Oct 12, 2018

mnacamura commented Oct 12, 2018

infinisil commented Oct 12, 2018

mnacamura commented Oct 13, 2018

infinisil Oct 13, 2018

mnacamura Oct 13, 2018

mnacamura commented Oct 13, 2018

infinisil commented Oct 13, 2018 •

edited

environment.shellAliases: change default behavior #44441

environment.shellAliases: change default behavior #44441

Conversation

mnacamura commented Aug 4, 2018 • edited

Motivation for this change

Things done

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

mnacamura commented Aug 4, 2018

Choose a reason for hiding this comment

mnacamura Oct 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Oct 12, 2018

mnacamura commented Oct 12, 2018

infinisil commented Oct 12, 2018

mnacamura commented Oct 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnacamura commented Oct 13, 2018

infinisil commented Oct 13, 2018 • edited

mnacamura commented Aug 4, 2018 •

edited

mnacamura Oct 12, 2018 •

edited

infinisil commented Oct 13, 2018 •

edited