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

[Ludovic Courtès]

Clément Lassieur <address@hidden> skribis:

> * bin/evaluate.in (absolutize, find-checkout, get-proc-source, get-load-path,
> get-guix-package-path, format-checkouts, append-paths): New procedures.
> (%not-colon): Remove variable.
> (main): Take the load path, package path and PROC from the checkouts that
> result from the inputs.  Format the checkouts before sending them to the
> procedure.
> * doc/cuirass.texi (Overview, Database schema): Document the changes.
> * examples/{guix-jobs.scm, hello-git.scm, hello-singleton.scm,
> hello-subset.scm, random.scm}: Adapt to the new specification format.
> * examples/guix-track-git.scm (package->spec): Rename to PACKAGE->INPUT.
> (package->git-tracked): Replace FETCH-REPOSITORY with FETCH-INPUT and handle
> the new format of its return value.
> * examples/random-jobs.scm (make-random-jobs): Rename RANDOM to CHECKOUT.
> Rename the checkout from 'random (which is a specification) to 'cuirass (which
> is a checkout resulting from an input).
> * src/cuirass/base.scm (fetch-repository): Rename to fetch-input.  Rename SPEC
> to INPUT.  Return a checkout object instead of returning two values.
> (evaluate): Take a list of CHECKOUTS and COMMITS as arguments, instead of
> SOURCE.  Remove TOKENIZE and LOAD-PATH.  Pass the CHECKOUTS instead of the
> SOURCE to "evaluate".  Build the EVAL object instead of getting it from
> "evaluate".
> (compile?, fetch-inputs, compile-checkouts): New procedures.
> (process-specs): Fetch all inputs instead of only fetching one repository.
> The result of that fetching operation is a list of CHECKOUTS whose COMMITS are
> used as a STAMP.
> * src/cuirass/database.scm (db-add-input, db-get-inputs): New procedures.
> (db-add-specification, db-get-specifications): Adapt to the new specification
> format.  Add/get all inputs as well.
> (db-add-evaluation): Rename REVISION to COMMITS.  Store COMMITS as space
> separated commit hashes.
> (db-get-builds): Rename REPO_NAME to NAME.
> (db-get-stamp): Rename COMMIT to STAMP.  Return #f when there is no STAMP.
> (db-add-stamp): Rename COMMIT to STAMP.  Deal with DB-GET-STAMP's new return
> value.
> (db-get-evaluations): Rename REVISION to COMMITS.  Tokenize COMMITS.
> * src/cuirass/utils.scm (%non-blocking): Export it.
> * src/schema.sql (Inputs): New table that refers to the Specifications table.
> (Specifications): Move input related fields to the Inputs table.  Rename
> REPO_NAME to NAME.  Rename ARGUMENTS to PROC_ARGS.  Rename FILE to PROC_PATH.
> Add LOAD_PATH_INPUTS, PACKAGE_PATH_INPUTS and PROC_INPUT fields that refer to
> the Inputs table.
> (Stamps): Rename REPO_NAME to NAME.
> (Evaluations): Rename REPO_NAME to NAME.  Rename REVISION to COMMITS.
> (Specifications_index): Replace with Inputs_index.
> * src/sql/upgrade-2.sql: New file.
> * tests/database.scm (example-spec, make-dummy-eval, sqlite-exec): Adapt to
> the new specifications format.  Rename REVISION to COMMITS.
> * tests/http.scm (evaluations-query-result, fill-db): Idem.

Wow, that’s intimidating.  :-)

>  (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”?

> -     ;; Until FILE is loaded, we must *not* load any Guix module because
> -     ;; SOURCE may be providing its own, which could differ from ours--this 
> is
> -     ;; the case when SOURCE is a Guix checkout.  The 'ref' procedure helps 
> us
> -     ;; achieve this.
> -     (let ((%user-module (make-fresh-user-module))
> -           (spec         (with-input-from-string specstr read))
> -           (stdout       (current-output-port))
> -           (stderr       (current-error-port))
> -           (load-path    (string-tokenize load-path %not-colon)))
> -       (unless (string-null? guix-package-path)
> -         (setenv "GUIX_PACKAGE_PATH" guix-package-path))
> +     ;; Until PROC-FILE is loaded, we must *not* load any Guix module because
> +     ;; the user may be providing its own with #:LOAD-PATH-INPUTS, which 
> could
> +     ;; differ from ours.  The 'ref' procedure helps us achieve this.
> +     (let* ((%user-module (make-fresh-user-module))
> +            (spec (with-input-from-string specstr read))
> +            (checkouts (with-input-from-string checkoutsstr read))
> +            (proc-source (get-proc-source spec checkouts))
> +            (load-path (get-load-path spec checkouts))
> +            (guix-package-path (get-guix-package-path spec checkouts))
> +            (stdout (current-output-port))
> +            (stderr (current-error-port)))
> +       (setenv "GUIX_PACKAGE_PATH"
> +               (append-paths static-guix-package-path guix-package-path))

Do I get it write that inputs do not necessarily 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.)

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

>         (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”.

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

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

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

I think that’s about it.

The size of reviews is often inversely proportional to the size of the
change, and I think this one is no exception.  :-)

I’m not fully up-to-speed on all the changes but I’ll guess we’ll see it
live when we upgrade Cuirass on berlin.

Thank you!

Ludo’.