From: "Clément Lassieur" <clement@lassieur.org>
To: "Ludovic Courtès" <ludo@gnu.org>
Cc: 32121@debbugs.gnu.org
Subject: [bug#32121] [PATCH 5/5] Add support for multiple inputs.
Date: Sun, 15 Jul 2018 10:25:32 +0200 [thread overview]
Message-ID: <87efg4gb2b.fsf@lassieur.org> (raw)
In-Reply-To: <87va9jjxgr.fsf@gnu.org>
Ludovic Courtès <ludo@gnu.org> writes:
> Clément Lassieur <clement@lassieur.org> 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
next prev parent reply other threads:[~2018-07-15 8:26 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-10 22:58 [bug#32121] Cuirass: add support for multiple inputs Clément Lassieur
2018-07-10 23:02 ` [bug#32121] [PATCH 1/5] base: Compile CHECKOUT in the fiber Clément Lassieur
2018-07-10 23:02 ` [bug#32121] [PATCH 2/5] utils: Reset the Fiber dynamic environment in %NON-BLOCKING Clément Lassieur
2018-07-13 8:35 ` Ludovic Courtès
2018-07-14 12:13 ` Clément Lassieur
2018-07-14 13:45 ` Ludovic Courtès
2018-07-10 23:02 ` [bug#32121] [PATCH 3/5] database: Add support for database upgrades Clément Lassieur
2018-07-13 8:47 ` Ludovic Courtès
2018-07-14 15:00 ` Clément Lassieur
2018-07-14 15:32 ` Clément Lassieur
2018-07-16 13:17 ` Ludovic Courtès
2018-07-10 23:02 ` [bug#32121] [PATCH 4/5] database: Call a specification 'jobset' instead of 'project' Clément Lassieur
2018-07-13 8:51 ` Ludovic Courtès
2018-07-13 9:35 ` Clément Lassieur
2018-07-13 9:43 ` Clément Lassieur
2018-07-13 11:56 ` Ludovic Courtès
2018-07-14 19:57 ` Clément Lassieur
2018-07-10 23:02 ` [bug#32121] [PATCH 5/5] Add support for multiple inputs Clément Lassieur
2018-07-13 9:28 ` Ludovic Courtès
2018-07-15 8:25 ` Clément Lassieur [this message]
2018-07-16 20:13 ` bug#32121: " Clément Lassieur
2018-07-13 8:32 ` [bug#32121] [PATCH 1/5] base: Compile CHECKOUT in the fiber Ludovic Courtès
2018-07-13 8:55 ` Clément Lassieur
2018-07-13 11:50 ` Ludovic Courtès
2018-07-13 11:57 ` Clément Lassieur
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87efg4gb2b.fsf@lassieur.org \
--to=clement@lassieur.org \
--cc=32121@debbugs.gnu.org \
--cc=ludo@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.