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
Merged

Conversation

@Gabriel439
Copy link
Contributor

@Gabriel439 Gabriel439 commented Dec 12, 2019

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

@edolstra edolstra commented Dec 12, 2019

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 ...)).

@Gabriel439
Copy link
Contributor Author

@Gabriel439 Gabriel439 commented Dec 12, 2019

@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: ...
@Gabriel439
Copy link
Contributor Author

@Gabriel439 Gabriel439 commented Dec 14, 2019

@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
@Gabriel439
Copy link
Contributor Author

@Gabriel439 Gabriel439 commented Jan 3, 2020

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

@jtojnar
Copy link
Contributor

@jtojnar 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

@Gabriel439
Copy link
Contributor Author

@Gabriel439 Gabriel439 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
Contributor

@roberth 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 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
Contributor

@roberth roberth commented Jan 5, 2020

return a list

precisely

... as suggested by @roberth
@Gabriel439 Gabriel439 requested a review from Infinisil as a code owner Jan 5, 2020
lib/default.nix Outdated Show resolved Hide resolved
... as suggested by @roberth

Co-Authored-By: Robert Hensing <roberth@users.noreply.github.com>
@roberth
roberth approved these changes Jan 5, 2020
Copy link
Member

@Infinisil Infinisil left a comment

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
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Jan 22, 2020

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

@Profpatsch Profpatsch commented Jan 22, 2020

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

@Profpatsch Profpatsch commented Jan 22, 2020

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

@Profpatsch Profpatsch commented Jan 22, 2020

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

@roberth
Copy link
Contributor

@roberth 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

@Profpatsch Profpatsch commented Jan 23, 2020

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

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Feb 5, 2020

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
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.