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

Add pkgs.lib.encodeGNUCommandLine #75539

Merged
merged 6 commits into from Jan 14, 2020

Conversation

Gabriella439
Copy link
Contributor

Motivation for this change

This adds a new utility to intelligently convert Nix records to
command line options to reduce boilerplate for simple use cases and to
also reduce the likelihood of malformed command lines

Things done

I ran nix-instantiate --eval --strict lib/tests/misc.nix

This adds a new utility to intelligently convert Nix records to
command line options to reduce boilerplate for simple use cases and to
also reduce the likelihood of malformed command lines
@edolstra
Copy link
Member

I'm hesitant about this because it puts a highly specific policy of how command line arguments are encoded in the standard library. It's not at all clear how universally applicable this is. For example, v = false is encoded as an empty string, but maybe in some CLI tools you want it to be encoded as --no-v.

The name renderOptions is rather vague. At first I thought this had something to do with the NixOS module system. Maybe something like encodeGNUCommandLine is more descriptive.

Escaping shell arguments should be handled elsewhere since you could also pass arguments as an array. So escaping could be done separately (e.g. concatMap escapeShellArg (renderOptions ...)).

@Gabriella439
Copy link
Contributor Author

@edolstra: I would be open to naming this in such a way that it does not imply that it is the only way to render command-line options and/or making the utility's behavior customizable (e.g. customizing how it handles lists or boolean-valued fields). For example, conceptually this utility could take an additional record argument specifying how to handle each behavior (with default values so that the user can supply { } for the common case):

encodeGNUCommandLine =
    { renderBool ? key: value: if value then " ${hyphenate key}" else ""
    , renderList ? key: values: ...
    , renderOption ? key: value: ...
    , ...
    }
    options: ...

@Gabriella439
Copy link
Contributor Author

@edolstra: Alright, I believe this is ready for re-review now. I incorporated your feedback

@roberth roberth changed the title Add pkgs.lib.renderOptions Add pkgs.lib.encodeGNUCommandLine Dec 15, 2019
... as suggested by @roberth

This also caught a bug in rendering lists, which this change also fixes
@Gabriella439
Copy link
Contributor Author

@roberth: I believe I addressed your feedback and this is ready for another review

@jtojnar
Copy link
Contributor

jtojnar commented Jan 4, 2020

Where would this be used seeing it returns string? We are moving towards __structuredAttrs, where things like configureFlags will be lists: #72074

@Gabriella439
Copy link
Contributor Author

Gabriella439 commented Jan 4, 2020

@jtojnar: A systemd service specification is one example of where you can use this:

{ config, lib, pkgs, ... }:

let
  cfg = config.services.example;

  options = { inherit (cfg) host port; };

in
  { options.services.example = {
      enable = lib.mkEnableOption "example";

      host = lib.mkOption { ... };

      port = lib.mkOption { ... };
    };

    config.systemd.services.example = lib.mkIf cfg.enable {
      wantedBy = [ "multi-user.target" ];

      script = ''
        ${pkgs}/bin/example${lib.toGNUCommandLine options}
      '';
    };
  }

@roberth
Copy link
Member

roberth commented Jan 5, 2020

You could extract a function toGNUCommandLine from this: encodeGNUCommandLine = a: escapeShellArgs (toGNUCommandLine a)

toGNUCommandLine will then be usable with structured attrs.

This will require a preceding space as in the-cmd ${encodeGNUCommandLine attrs}, but that's good because it's consistent with escapeShellArgs and the shell itself.

@jtojnar
Copy link
Contributor

jtojnar commented Jan 5, 2020

Well, the function returns a string. To be useful for structured attrs, it would have to return a list.

@roberth
Copy link
Member

roberth commented Jan 5, 2020

return a list

precisely

lib/default.nix Outdated Show resolved Hide resolved
... as suggested by @roberth

Co-Authored-By: Robert Hensing <roberth@users.noreply.github.com>
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, this could be used to provide something like NixOS/rfcs#42 for CLI configs.

@roberth roberth merged commit 8da8146 into NixOS:master Jan 14, 2020
@Profpatsch
Copy link
Member

Just seeing this, it would have been good if it had followed the same interface as lib/generators.nix;
that is naming the options mkX instead of renderX. The fn {} val interface is the same, luckily.

Not entirely sure what the separation in renderList/Option/Bool is for, one argument with pattern match would suffice I think. As long as it’s new like this, can we still change the interface?

@Profpatsch
Copy link
Member

Oh, I’d also rename the functions to use toX for the string output (which is how generators.nix names its functions) and toXList for the list version.

@Profpatsch
Copy link
Member

Ah, no, the string is explicitely POSIX shell-formatted, so we should call it that. toXShell. See escapeShellArg in strings.nix.

@Profpatsch
Copy link
Member

Let’s also not expose it into the global namespace, similar to generators. cli is only three letters.

@roberth
Copy link
Member

roberth commented Jan 22, 2020

@Profpatsch feel free to make a PR for the rename/move.

Not entirely sure what the separation in renderList/Option/Bool is for

It's for overriding the convention, for example to allow x = false to be rendered as --no-x.

Re POSIX: it does seem to be GNU, at least according to GNU.

GNU adds long options to these conventions. Long options consist of ‘--’ followed by a name made of alphanumeric characters and dashes.

-- https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html

I'd welcome a more generic term than GNU, but I don't know of any that implies long options.

@Profpatsch
Copy link
Member

I created a PR with my suggested breaking changes: #78337

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/january-2020-in-nixos/5771/1

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

Successfully merging this pull request may close these issues.

None yet

7 participants