[PATCH 0/2] Make 'guix help' helpful

> On Tue, Sep 01, 2020 at 10:35:20PM +0200, Ludovic Courtès wrote:

>> Hey Guix!

>>

>> Here’s another long-overdue change that was brought to my

>> attention: making ‘guix help’ show a synopsis of each command.

>> While at it, it also groups them in categories (I more or

>> less followed the manual, but there’s prolly room for

>> improvement, like a ‘packaging’ category maybe?):

>>

>

> I dislike the idea that some commands are only for developers and some

> commands are "too advanced for the common user" (my words, not yours).

> Before bikeshedding about where the options should be, how about:

> commands for developers -> commands related to building

> advanced usage -> commands related to verification

> This way they're still "commands for everybody" and they're more

> descriptive than just "advanced" "expert" and "debug" style headers.

> Hi,

>

> Efraim Flashner <efraim@flashner.co.il> skribis:

>

> > On Tue, Sep 01, 2020 at 10:35:20PM +0200, Ludovic Courtès wrote:

> >> Hey Guix!

> >>

> >> Here’s another long-overdue change that was brought to my

> >> attention: making ‘guix help’ show a synopsis of each command.

> >> While at it, it also groups them in categories (I more or

> >> less followed the manual, but there’s prolly room for

> >> improvement, like a ‘packaging’ category maybe?):

> >>

> >

> > I dislike the idea that some commands are only for developers and some

> > commands are "too advanced for the common user" (my words, not yours).

>

> Yeah, I’m ambivalent about this as well.

>

> > Before bikeshedding about where the options should be, how about:

> > commands for developers -> commands related to building

>

> Rather, “commands for development“, because that’s really what ‘guix

> environment’ is about, for instance, but saying ‘development’ rather

> than ‘developers’ is probably wise.

>

> > advanced usage -> commands related to verification

>

> Well that only works for ‘challenge’ (which I’d like to put in the hands

> of ‘normal’ users, so I’m not comfortable with the ‘advanced’ label, but

> OTOH, it’s clearly not one of the commands you’d begin with.)

>

> What about archive, copy, download, etc.?

>

> > This way they're still "commands for everybody" and they're more

> > descriptive than just "advanced" "expert" and "debug" style headers.

>

> Understood.

>

> Thanks for your feedback!

>

> Ludo’.

> This changes 'guix help' to print a short synopsis for each command and

> to group commands by category.

> diff --git a/guix/scripts.scm b/guix/scripts.scm

> index 8534948892..013b775818 100644

> --- a/guix/scripts.scm

> +++ b/guix/scripts.scm

> @@ -34,7 +34,10 @@

> #:use-module (srfi srfi-19)

> #:use-module (srfi srfi-37)

> #:use-module (ice-9 match)

> - #:export (args-fold*

> + #:export (synopsis

> + category

> + define-command

> + args-fold*

> parse-command-line

> maybe-build

> build-package

> @@ -50,6 +53,30 @@

> ;;;

> ;;; Code:

>

> +;; Syntactic keywords.

> +(define synopsis 'command-synopsis)

> +(define category 'command-category)

> +(define-syntax define-command

> + (syntax-rules (category synopsis)

> + "Define the given command as a procedure along with its synopsis and,

> +optionally, its category. The synopsis becomes the docstring of the

> +procedure, but both the category and synopsis are meant to be read (parsed) by

> +'guix help'."

> + ;; The (synopsis ...) form is here so that xgettext sees those strings as

> + ;; translatable.

> + ((_ (name . args)

> + (synopsis doc) body ...)

> + (define (name . args)

> + doc

> + body ...))

> + ((_ (name . args)

> + (category _)

> + (synopsis doc) body ...)

> + (define (name . args)

> + doc

> + body ...))))

> +

> (define (args-fold* args options unrecognized-option-proc operand-proc . seeds)

> "A wrapper on top of `args-fold' that does proper user-facing error

> reporting."

> diff --git a/guix/scripts/archive.scm b/guix/scripts/archive.scm

> index f3b86fba14..8796774a01 100644

> --- a/guix/scripts/archive.scm

> +++ b/guix/scripts/archive.scm

> @@ -355,7 +355,10 @@ output port."

> ;;; Entry point.

> ;;;

>

> -(define (guix-archive . args)

> +(define-command (guix-archive . args)

> + (category advanced)

> --- a/guix/scripts/weather.scm

> +++ b/guix/scripts/weather.scm

> @@ -495,7 +495,9 @@ SERVER. Display information for packages with at least THRESHOLD dependents."

> ;;; Entry point.

> ;;;

>

> -(define (guix-weather . args)

> +(define-command (guix-weather . args)

> + (synopsis "report on the available of pre-built package binaries")

> +(define (source-file-command file)

> + "Read FILE, a Scheme source file, and return either a <command> object based

> +on the 'define-command' top-level form found therein, or #f if FILE does not

> +contain a 'define-command' form."

> + (define command-name

> + (match (string-split file #\/)

> + ((_ ... "guix" "scripts" name)

> + (list (file-sans-extension name)))

> + ((_ ... "guix" "scripts" first second)

> + (list first (file-sans-extension second)))))

> + ;; The strategy here is to parse FILE. This is much cheaper than a

> + ;; technique based on run-time introspection where we'd load FILE and all

> + ;; the modules it depends on.

> + (call-with-input-file file

> + (lambda (port)

> + (let loop ()

> + (match (read port)

> + (('define-command _ ('synopsis synopsis)

> + _ ...)

> + (command command-name synopsis 'main))

> + (('define-command _

> + ('category category) ('synopsis synopsis)

> + _ ...)

> + (command command-name synopsis category))

> + ((? eof-object?)

> + #f)

> + (_

> + (loop)))))))

> +

> + (define (display-commands commands)

> + (let* ((names (map (lambda (command)

> + (string-join (command-name command)))

> + commands))

> + (max-width (reduce max 0 (map string-length names))))

> * guix/ui.scm (show-bug-report-information): Link to

> <https://guix.gnu.org/help/> instead of <https://www.gnu.org/gethelp/>.

> The former is much more useful and includes links to GNU manuals.

> ---

> guix/ui.scm | 5 +++--

> 1 file changed, 3 insertions(+), 2 deletions(-)

>

> diff --git a/guix/ui.scm b/guix/ui.scm

> index 4d90a47bb9..87a1925a4b 100644

> --- a/guix/ui.scm

> +++ b/guix/ui.scm

> @@ -542,8 +542,9 @@ There is NO WARRANTY, to the extent permitted by law.

> Report bugs to: ~a.") %guix-bug-report-address)

> (format #t (G_ "

> ~a home page: <~a>") %guix-package-name %guix-home-page-url)

> - (display (G_ "

> -General help using GNU software: <http://www.gnu.org/gethelp/>"))

> + (format #t (G_ "

> +General help using Guix and GNU software: <~a>")

> + "https://guix.gnu.org/help/")

> (newline))

> Ludovic Courtès <ludo@gnu.org> writes:

>> +;; Syntactic keywords.

>> +(define synopsis 'command-synopsis)

>> +(define category 'command-category)

>

> Are these definition really necessary/useful? I would have thought

> having category and synopsis understood as literals in the

> define-command syntax was enough?

>> +(define-syntax define-command

>> + (syntax-rules (category synopsis)

>> -(define (guix-archive . args)

>> +(define-command (guix-archive . args)

>> + (category advanced)

>

> It'd be helpful if the category was an enum to keep the set of

> categories focused and helpful.

>> + ;; The strategy here is to parse FILE. This is much cheaper than a

>> + ;; technique based on run-time introspection where we'd load FILE and all

>> + ;; the modules it depends on.

>

> Interesting! Have you measure it? I would have thought loading a couple

> optimized byte code modules could have been nearly as fast as parsing

> files manually. If so, I think it'd be preferable to use introspection

> rather than implement a custom parser.

>> + (define (display-commands commands)

>> + (let* ((names (map (lambda (command)

>> + (string-join (command-name command)))

>> + commands))

>> + (max-width (reduce max 0 (map string-length names))))

>

> You can drop reduce and use (max (map string-length names)) instead.

> remove removed installed packages

> environment spawn one-off software environments

> PS: We can talk about categories, but make sure to turn on the

> bikeshedding limitation mode of your mail client. :-)

>> environment spawn one-off software environments

>

> spawn one-off?

> Here’s a v2 of the patches, where I tried to take your feedback into

> account. Changes:

>

> • Categories that appear in uses of ‘define-command’ are now validated

> at macro-expansion time.

>

> • (guix scripts) contains an alist of the known categories and

> ‘show-help’ traverses it.

>

> • Changed the label for development commands to “commands for

> development”, and changed “advanced” to “plumbing”.

>

> • Added a “packaging” category.

>

> • Fixed the typos you reported.

> Ludovic Courtès <ludo@gnu.org> skribis:

>

>> Here’s a v2 of the patches, where I tried to take your feedback into

>> account. Changes:

>>

>> • Categories that appear in uses of ‘define-command’ are now validated

>> at macro-expansion time.

>> • (guix scripts) contains an alist of the known categories and

>> ‘show-help’ traverses it.

>> • Changed the label for development commands to “commands for

>> development”, and changed “advanced” to “plumbing”.

>>

>> • Added a “packaging” category.

>>

>> • Fixed the typos you reported.

>

> I pushed this v2 as 3794ce93be8216d8378df7b808ce7f53b1e05a53.

>>> +;; Syntactic keywords.

>>> +(define synopsis 'command-synopsis)

>>> +(define category 'command-category)

>>

>> Are these definition really necessary/useful? I would have thought

>> having category and synopsis understood as literals in the

>> define-command syntax was enough?

>

> It’s not strictly necessary but it’s been considered “good practice”.

> That allows users to detect name clashes, to rename/hide/etc. syntactic

> keywords and so on.

>>> + ;; The strategy here is to parse FILE. This is much cheaper than a

>>> + ;; technique based on run-time introspection where we'd load FILE and all

>>> + ;; the modules it depends on.

>>

>> Interesting! Have you measure it? I would have thought loading a couple

>> optimized byte code modules could have been nearly as fast as parsing

>> files manually. If so, I think it'd be preferable to use introspection

>> rather than implement a custom parser.

>

> On a fast recent laptop with an SSD, a load of 0, hot cache, etc., we’d

> still be below 1s. But see:

>

> $ strace -c guix help >/dev/null

> % time seconds usecs/call calls errors syscall

> ------ ----------- ----------- --------- --------- ------------------

> 62.69 0.002698 1 2266 2043 stat

> 10.94 0.000471 2 161 2 lstat

> 4.55 0.000196 0 246 mmap

> 4.51 0.000194 0 330 172 openat

>

> [...]

>

> ------ ----------- ----------- --------- --------- ------------------

> 100.00 0.004304 1 3748 2235 total

> $ strace -c guile -c '(use-modules (guix scripts system) (guix scripts authenticate))'

> % time seconds usecs/call calls errors syscall

> ------ ----------- ----------- --------- --------- ------------------

> 54.27 0.007799 1 5735 4518 stat

> 12.00 0.001724 11 149 27 futex

> 9.06 0.001302 0 1328 651 openat

> 7.24 0.001040 1 822 mmap

>

> [...]

>

> ------ ----------- ----------- --------- --------- ------------------

> 100.00 0.014371 1 10334 5202 total

>

>

> (The 1st run is the current ‘guix help’; the 2nd run +/- emulates what

> you propose.)

>

> Loading all the modules translates into a lot more I/O, roughly an order

> of magnitude. We’re talking about loading tens of modules just to get

> at that synopsis:

>

> scheme@(guile-user)> ,use(guix modules)

> scheme@(guile-user)> (length (source-module-closure '((guix scripts system) (guix scripts authenticate))))

> $10 = 439

> scheme@(guile-user)> (length (source-module-closure '((guix scripts) (guix ui))))

> $11 = 31

>

> Memory usage would also be very different:

>

> $ \time guix help >/dev/null

> 0.07user 0.01system 0:00.06elapsed 128%CPU (0avgtext+0avgdata 35348maxresident)k

> 0inputs+0outputs (0major+3906minor)pagefaults 0swaps

> $ \time guile -c '(use-modules (guix scripts system) (guix scripts authenticate))'

> 0.42user 0.05system 0:00.37elapsed 128%CPU (0avgtext+0avgdata 166916maxresident)k

> 0inputs+0outputs (0major+15148minor)pagefaults 0swaps

> In summary, while this approach undoubtedly looks awkward to any Lisper,

> I think it’s a good way to not contribute to the general impression of

> sluggishness and resource-hungriness of ‘guix’ commands. :-)

>

>>> + (define (display-commands commands)

>>> + (let* ((names (map (lambda (command)

>>> + (string-join (command-name command)))

>>> + commands))

>>> + (max-width (reduce max 0 (map string-length names))))

>>

>> You can drop reduce and use (max (map string-length names)) instead.

>

> I could do (apply max (map …)) but I don’t like the idea of abusing

> variadic argument lists in that way—I know, it’s very subjective. ;-)

> Thanks for your feedback, I’ll send a v2!

> Sorry I couldn't reply faster.

> Thanks for the detailed measurements! It does indeed seem your approach

> is better, especially considering memory usage. Perhaps the commands

> could have been moved to dedicated modules not using much dependency at

> all so that their closure would have been small hence fast to load, but

> keeping the commands definitions local to where they are useful is

> definitely a nice property.

>> In summary, while this approach undoubtedly looks awkward to any Lisper,

>> I think it’s a good way to not contribute to the general impression of

>> sluggishness and resource-hungriness of ‘guix’ commands. :-)

>>

>>>> + (define (display-commands commands)

>>>> + (let* ((names (map (lambda (command)

>>>> + (string-join (command-name command)))

>>>> + commands))

>>>> + (max-width (reduce max 0 (map string-length names))))

>>>

>>> You can drop reduce and use (max (map string-length names)) instead.

>>

>> I could do (apply max (map …)) but I don’t like the idea of abusing

>> variadic argument lists in that way—I know, it’s very subjective. ;-)

>

> Eh, I wonder why? I may be missing something, but if max allows it,

> doesn't it mean it's a valid use? Anyway, just curious to know what are

> the grounds for this personal preference :-).

> Hi,

>

> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

>

>> Sorry I couldn't reply faster.

>

> No problem. I went ahead with this patch series, but nothing’s set in

> stone and I’m open to further changes.

>> Thanks for the detailed measurements! It does indeed seem your approach

>> is better, especially considering memory usage. Perhaps the commands

>> could have been moved to dedicated modules not using much dependency at

>> all so that their closure would have been small hence fast to load, but

>> keeping the commands definitions local to where they are useful is

>> definitely a nice property.

>

> We can’t really reduce the closure of commands. In particular, (guix

> scripts system) has to load pretty much “everything”.

>

> What we could do is use #:autoload aggressively in the (guix scripts …)

> modules, such that startup time would be as small as possible—e.g.,

> ‘--help’ would not trigger loading of a zillion modules.

>

> It’s a bit tedious though and not necessarily helpful in the general

> case where one is doing something non-trivial with the command. Well

> dunno, we could try!

>>> In summary, while this approach undoubtedly looks awkward to any Lisper,

>>> I think it’s a good way to not contribute to the general impression of

>>> sluggishness and resource-hungriness of ‘guix’ commands. :-)

>>>

>>>>> + (define (display-commands commands)

>>>>> + (let* ((names (map (lambda (command)

>>>>> + (string-join (command-name command)))

>>>>> + commands))

>>>>> + (max-width (reduce max 0 (map string-length names))))

>>>>

>>>> You can drop reduce and use (max (map string-length names)) instead.

>>>

>>> I could do (apply max (map …)) but I don’t like the idea of abusing

>>> variadic argument lists in that way—I know, it’s very subjective. ;-)

>>

>> Eh, I wonder why? I may be missing something, but if max allows it,

>> doesn't it mean it's a valid use? Anyway, just curious to know what are

>> the grounds for this personal preference :-).

>

> It’s mostly aesthetic, but it comes from the idea that there could be

> limitations on the maximum number of arguments a procedure can take, or

> inefficiencies with dealing with many arguments. Now, in today’s Guile,

> there are no such issues… (And now I look really silly!) :-)