Package: guix-patches;
Reported by: Clément Lassieur <clement <at> lassieur.org>
Date: Tue, 10 Jul 2018 22:59:02 UTC
Severity: normal
Done: Clément Lassieur <clement <at> lassieur.org>
Bug is archived. No further changes may be made.
Message #38 received at 32121 <at> debbugs.gnu.org (full text, mbox):
From: ludo <at> gnu.org (Ludovic Courtès) To: Clément Lassieur <clement <at> lassieur.org> Cc: 32121 <at> debbugs.gnu.org Subject: Re: [bug#32121] [PATCH 5/5] Add support for multiple inputs. Date: Fri, 13 Jul 2018 11:28:52 +0200
Clément Lassieur <clement <at> lassieur.org> 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’.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.