[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#32121] [PATCH 5/5] Add support for multiple inputs.
From: |
Clément Lassieur |
Subject: |
[bug#32121] [PATCH 5/5] Add support for multiple inputs. |
Date: |
Sun, 15 Jul 2018 10:25:32 +0200 |
User-agent: |
mu4e 1.0; emacs 26.1 |
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
- [bug#32121] [PATCH 3/5] database: Add support for database upgrades., (continued)
[bug#32121] [PATCH 4/5] database: Call a specification 'jobset' instead of 'project'., Clément Lassieur, 2018/07/10
- [bug#32121] [PATCH 4/5] database: Call a specification 'jobset' instead of 'project'., Ludovic Courtès, 2018/07/13
- [bug#32121] [PATCH 4/5] database: Call a specification 'jobset' instead of 'project'., Clément Lassieur, 2018/07/13
- [bug#32121] [PATCH 4/5] database: Call a specification 'jobset' instead of 'project'., Clément Lassieur, 2018/07/13
- [bug#32121] [PATCH 4/5] database: Call a specification 'jobset' instead of 'project'., Ludovic Courtès, 2018/07/13
- [bug#32121] [PATCH 4/5] database: Call a specification 'jobset' instead of 'project'., Clément Lassieur, 2018/07/14
[bug#32121] [PATCH 5/5] Add support for multiple inputs., Clément Lassieur, 2018/07/10
[bug#32121] [PATCH 1/5] base: Compile CHECKOUT in the fiber., Ludovic Courtès, 2018/07/13