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

nixos/defaults: init #101127

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

nixos/defaults: init #101127

wants to merge 9 commits into from

Conversation

@deviant
Copy link
Member

@deviant deviant commented Oct 20, 2020

Motivation for this change

This adds a new NixOS module for configuring the default editor and
pager of a system. It also deprecates users.defaultUserShell, opting
to handle all three in one place for consistency. As with #16189, it
makes use of passthru attributes on applicable packages: editorCommand
for $EDITOR, and pagerCommand for $PAGER.

This gives every text editor in nixpkgs first-class support for being
configured as the default editor, without needing to create a new module
for each and every one of them. This avoids the need for changes such as
#100132, which are now handled totally generically. Configuration can be
done via packages themselves (or wrappers), further alleviating the need
for additional modules.

Like with #97171, this allows for removing more things from the system
path: nano is no longer a hardcoded dependency, sitting around unused
by those with different editing tastes. Since it now has a dedicated
option, it's easy to find where to configure the default text editor.

Additionally, the default editor and pager packages are now added to the
system path in the same location that their corresponding variables are
set, simplifying the surrounding logic and making it easier to follow.

As noted in nixos/modules/programs/environment.nix:

Most of the stuff here should probably be moved elsewhere sometime.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@deviant
Copy link
Member Author

@deviant deviant commented Oct 20, 2020

(rebased)

@deviant
Copy link
Member Author

@deviant deviant commented Oct 26, 2020

(rebased)

nixos/modules/config/defaults.nix Outdated Show resolved Hide resolved
deviant added 7 commits Nov 16, 2020
This adds a new passthru to all text editor derivations, analogous to
the shellPath passthru, containing a string suitable to be used as the
$EDITOR environment variable. Editors that daemonise by default are
passed flags that stop them from doing so.

This covers every single text editor in nixpkgs. IDEs were not given
such treatment, with the rationale that they are intended to be used
for editing entire projects - and often can't be configured to open
single files or disable daemonisation. Similarly, task-specific text
editors (such as Markdown editors) were skipped.
This adds a new passthru to all pager derivations, containing a string
suitable to be used as the $PAGER environment variable.
This introduces a new set of options for configuring the default editor
and pager. users.defaults.editor can be set to any derivation with the
'editorCommand' attribute, which is a string suitable for being used as
the $EDITOR variable. Similarly, the option users.defaults.pager can be
set to any derivation with the 'pagerCommand' attribute, and controls
the $PAGER variable.

Of note: programs invoking $EDITOR and $PAGER expect the respective
program to not exit until the user has finished editing/viewing the
file, which means that any program that daemonises by default (such as
Atom or Sublime Text) needs an additional flag to be passed. Such flags
are specific to the program in question, and are usually documented in
manual pages and --help output.

This also means that the system path no longer implicitly contains nano
and less, which is probably welcome news for anyone with particularly
passionate opinions about text editors ;)
Since users.defaults is intended to hold options for selecting
default programs, it makes sense to deprecate users.defaultUserShell
and incorporate its functionality here.
This module is made redundant by the users.defaults.editor option.
This removes duplication of functionality provided by
users.defaults.editor, and sets that instead.
This removes duplication of functionality provided by
users.defaults.editor, and sets that instead. It also makes
defaultEditor actually do something, since it wasn't previously used.
@deviant deviant requested a review from teto as a code owner Nov 16, 2020
@deviant deviant requested a review from roberth Nov 16, 2020
@deviant
Copy link
Member Author

@deviant deviant commented Nov 16, 2020

(simplified how to configure different commands + rebased)

packageWithAttr = type: attr: package // {
name = "${type}Package";
description = "${type} package";
check = x: package.check x && hasAttr attr x;
Copy link
Member

@andir andir Nov 18, 2020

Can we generate a meaningful error message here? I know that this will complain about the invalid type but as it is likely that someone adds a new editor without knowing about the new passthru's it might be good to tell the user what is required.

Copy link
Member Author

@deviant deviant Nov 18, 2020

I think the most meaningful error message we're likely to end up with is "package with the passthru ${attr}", and I'm unsure as to whether that's any more clear than the error that's already produced.

Copy link
Member

@andir andir Nov 18, 2020

I was thinking about something like:

The package you provided as defaults.editor does not have the required attribute `editorCommand`. Please select the correct package or add the passthru to the package as documented in the option.

NIx is often already bad enough user experience for new users so it is probably worth being helpful in error messages.

@@ -26,6 +26,22 @@
<listitem>
<para>GNOME desktop environment was upgraded to 3.38, see its <link xlink:href="https://help.gnome.org/misc/release-notes/3.38/">release notes</link>.</para>
</listitem>
<listitem>
Copy link
Member

@andir andir Nov 18, 2020

In addition to the nixos release notes we should probably mention it in the stdenv docs so new contributors will be able to read up on it. We already document passthur.updateScript there so this wouldn't be too weird.

Copy link
Member Author

@deviant deviant Nov 18, 2020

Is there a place in the documentation that guides people through how to add a new editor/pager/shell/specific type of package, already? I don't remember seeing anything of that sort, and it'd be more easily discoverable there.

Copy link
Member

@andir andir Nov 18, 2020

I don't think there is how to package XYZ in the documentation but rather documentation on our build system where we already document other attributes of the passthru's. Due to a lack of a better section I'd stick it in there so it isn't undocumented. If, at some point, we have more specific documentation that could be moved.

the editorCommand passthru of the package. For example:
<programlisting>
users.defaults.editor = pkgs.vim // { editorCommand = "gvim -f"; };
Copy link
Member

@andir andir Nov 18, 2020

Should we encourage users to open a bug report if that attribute is missing?

Copy link
Member Author

@deviant deviant Nov 18, 2020

In the options documentation?

Copy link
Member

@andir andir Nov 18, 2020

I am not sure! I was just thinking about that when I saw the suggested ad-hoc set merging,.

Copy link
Member Author

@deviant deviant Nov 18, 2020

Do you feel the description isn't clear enough about that being the way to override the attribute instead of merely adding it? I could perhaps reword that a bit if you feel it's ambiguous.

Copy link
Contributor

@birlorg birlorg Nov 23, 2020

maybe something like this:

   description = ''
        The package to use as the default editor.

       To change the <varname>$EDITOR</varname> variable, most
       editors will be configured enough that, for example:
 <programlisting>
        users.defaults.editor = pkgs.vim;
</programlisting>
will be enough, but if you want to override what the package provides as editorCommand:

         set the editorCommand passthru of the package. For example:
        <programlisting>
        users.defaults.editor = pkgs.vim // { editorCommand = "gvim -f"; };
       </programlisting>
'';

pkgs.less
pkgs.libcap
pkgs.nano
Copy link
Member

@samueldr samueldr Nov 18, 2020

To anyone else reviewing: This is fine since <nixpkgs/nixos/modules/config/defaults.nix> sets those as defaults for the new configuration options.

attributes, respectively. For instance:
<programlisting>
users.defaults = {
editor = pkgs.vim // { editorCommand = "gvim -f"; };
Copy link
Member

@samueldr samueldr Nov 18, 2020

Please add a note that this example overrides the default editorCommand set in vim. That most editors will have a good default configured.

Otherwise I'll bet we'll see pkgs.vim // {editorCommand = "vim"; }; in use :)

Copy link
Member

@samueldr samueldr left a comment

Minor, but required, changes, see in-line comments.

nixos/modules/config/shells-environment.nix Outdated Show resolved Hide resolved

pager = mkOption {
type = types.pagerPackage;
default = pkgs.less;
Copy link
Member

@samueldr samueldr Nov 18, 2020

The previous default used less -R:

        PAGER = mkDefault "less -R";

The current configuration for pkgs.less is simply less.

Shouldn't the default for pkgs.less be less -R and your description instead describe how to override it to the -R-less version? (Stating as the note above, that this overrides the default value)

@@ -21,4 +21,6 @@ stdenv.mkDerivation rec {
license = licenses.gpl3;
maintainers = with maintainers; [ eelco dtzWill ];
};

passthru.pagerCommand = "less";
Copy link
Member

@samueldr samueldr Nov 18, 2020

(See other comment)

Shouldn't this be less -R?

<para>
<option>users.defaultUserShell</option> has been renamed to
<link linkend="opt-users.defaults.shell">users.defaults.shell</link>.
</para>
Copy link
Member

@samueldr samueldr Nov 18, 2020

(Unless I'm mistaken, not tested yet)

In addition, the value must now be set to a package.


The previous implementation used:

      type = types.either types.path types.shellPackage;

While now it only uses types.shellPackage.

The documentation stated:

This option defines the default shell assigned to user accounts. This can be either a full system path or a shell package.

This must not be a store path, since the path is used outside the store (in particular in /etc/passwd).

Copy link
Member Author

@deviant deviant Nov 18, 2020

This one's intentional. I added some fairly elaborate backcompat code to take care of the most common cases, and the description explains how to change the path if that isn't enough. The reason why this was even an either in the first place was actually due to further backcompat — hopefully 4 years is enough time for it to be reasonable to remove after. Additionally, it's less of a problem restricting the type slightly here, since people will have to update their configs after this change regardless.

deviant added 2 commits Nov 18, 2020
Avoiding mentioning $EDITOR anywhere other than in the documentation
for users.defaults.editorCommand will help people find the option more
easily, and decrease the chances of it being overlooked.
@stale
Copy link

@stale stale bot commented Jul 19, 2021

I marked this as stale due to inactivity. → More info

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment