[bug#32121] [PATCH 5/5] Add support for multiple inputs.

[Clément Lassieur]

Ludovic Courtès <address@hidden> writes:

> Clément Lassieur <address@hidden> skribis:
>
>>  (define* (main #:optional (args (command-line)))
>>    (match args
>> -    ((command load-path guix-package-path source specstr)
>> -     ;; Load FILE, a Scheme file that defines Hydra jobs.
>> +    ((command static-guix-package-path specstr checkoutsstr)
>> +     ;; Load PROC-FILE, a Scheme file that defines Hydra jobs.
>
> There’s no “proc-file”; should it be “proc-source”?

It was #:PROC-PATH, but I'll bind it to FILE and use FILE in the
comment, as it was initially.

> Do I get it write that inputs do not necessarily contribute to
> GUIX_PACKAGE_PATH?

Yes!  Only inputs in package-path-inputs contribute to
GUIX_PACKAGE_PATH.

> Some inputs may provide code (to be in %load-path) while not provide any
> package definition (so nothing to add to GUIX_PACKAGE_PATH.)

Indeed.  And some inputs can contribute to both %load-path and
GUIX_PACKAGE_PATH.  It's flexible.

>>         ;; Since we have relative file name canonicalization by default, 
>> better
>> -       ;; change to SOURCE to make sure things like 'include' with relative
>> -       ;; file names work as expected.
>> -       (chdir source)
>> +       ;; change to PROC-SOURCE to make sure things like 'include' with
>> +       ;; relative file names work as expected.
>> +       (chdir proc-source)
>
> As a rule of thumb, identifiers for local variables should, IMO, almost
> always be a single word or at most two words.  Long names like
> ‘static-guix-package-path’ in local scope tend to make code harder to
> read; ‘proc-source’ here should probably be ‘source’ because we know
> what it is we’re talking about.

Okay!  Well I'll just remove static-guix-package-path (you know, the
--load-path argument to the cuirass command), because it's better to use
inputs instead.  And it'll simplify the code.

I'll also rename my GET-something procedures.

>>         (save-module-excursion
>>          (lambda ()
>>            (set-current-module %user-module)
>> -          (primitive-load (assq-ref spec #:file))))
>> +          (primitive-load (assq-ref spec #:proc-path))))
>
> Nitpick: in GNU “path” means “search path” (a list of directories), so
> here I think it should be “file” or “file name”, not “path”.

Ok I'll change it everywhere else too.

>>  @command{cuirass} acts as a daemon polling @acronym{VCS, version control
>> -system} repositories for changes, and evaluating a derivation when
>> -something has changed (@pxref{Derivations, Derivations,, guix, Guix}).
>> -As a final step the derivation is realized and the result of that build
>> -allows you to know if the job succeeded or not.
>> +system} repositories (called @code{inputs}) for changes, and evaluating a
>
> s/@code/@dfn/
>
>> +derivation when an @code{input} has changed (@pxref{Derivations, 
>> Derivations,,
>
> s/@code//
>
> @code is to refer to identifiers in the code, things like that.

Got it :-)

>> +There are three @code{inputs}: one tracking the Guix repository, one 
>> tracking
>
> s/@code//
>
>> +(define (compile-checkouts spec all-checkouts)
>> +  (let* ((checkouts (filter compile? all-checkouts))
>> +         (thunks
>> +          (map
>> +           (lambda (checkout)
>> +             (lambda ()
>> +               (log-message "compiling input '~a' of spec '~a' (commit ~s)"
>> +                            (assq-ref checkout #:name)
>> +                            (assq-ref spec #:name)
>> +                            (assq-ref checkout #:commit))
>> +               (compile checkout)))
>> +           checkouts))
>> +         (results (par-map %non-blocking thunks)))
>> +    (map (lambda (checkout)
>> +           (log-message "compiled input '~a' of spec '~a' (commit ~s)"
>> +                        (assq-ref checkout #:name)
>> +                        (assq-ref spec #:name)
>> +                        (assq-ref checkout #:commit))
>> +           checkout)
>> +         results)))
>
> Since the return value is unused, we could perhaps make it:
>
>   (define (compile-checkouts spec checkouts)
>     (for-each (lambda (checkout)
>                 (log-message …)
>                 (non-blocking (compile checkout)))
>               checkouts))

I use par-map because it's better to build them in parallel.  Also, the
return value is used to display a log message.

> and move the ‘filter’ call to the call site (the job of
> ‘compile-checkouts’, one might think, is to compile what it’s given, not
> to filter things.)

Right!

Thank you for the review, I'm learning a lot ;-).
Clément