GNU bug report logs - #28487
[PATCH] cuirass: Add gnu-system build spec.

Previous Next

Full log

Message #23 received at 28487 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Jan Nieuwenhuizen <janneke <at> gnu.org>
Cc: Mathieu Othacehe <m.othacehe <at> gmail.com>, 28487 <at> debbugs.gnu.org
Subject: Re: [bug#28487] [PATCH] cuirass: Add gnu-system build spec.
Date: Wed, 27 Sep 2017 21:45:39 +0200

Hi!

Jan Nieuwenhuizen <janneke <at> gnu.org> skribis:

>>> +(define (symbol-alist-entry->keyword-alist-entry entry)
>>> +  (cons (symbol->keyword (car entry)) (cdr entry)))
>>
>> I *think* that’s correct, though we’ll need to double check.
>
> I tested today and there were minor problems.  Cuirass actually doesn't
> take an alist; instead takes a list that includes (#:job-name . "name").
>
> Also, Cuirass performs an sexp-read and thus #<<license> ...> needs to
> get sexp'ified.
>
> Anyway, what I proposed was close and attached is a tested, working
> version (that may need some work, see below).

Oh good points, thanks for testing!

BTW, there’s “make hydra-jobs.scm”, which I occasionally use to test
gnu-system.scm (it spits a raw alist, which is enough to make sure that
it works.)  We could similarly add “make cuirass-jobs.scm” eventually.

>>> --- a/build-aux/hydra/gnu-system.scm
>>> +++ b/build-aux/hydra/gnu-system.scm
>>> @@ -270,6 +270,8 @@ valid."
>>>    (define subset
>>>      (match (assoc-ref arguments 'subset)
>>>        ("core" 'core)                              ; only build core packages
>>> +      ("hello" 'hello)                            ; only build hello
>>> +      (((? string?) (? string?) ...) 'list)       ; only build selected list of packages
>>>        (_ 'all)))                                  ; build everything
>>
>> This part could be added separately.
>
> Yes...it could.  Do you mean a separate patch, or ...

Yes, separate patch for clarity: first patch does the Hydra/Cuirass
split, second patch adds the ability to select a list of packages.

Would that be OK?

>> (It’s not usuable via Hydra since its UIs does not support passing
>> list-of-strings arguments.)
>
> ...I don't quite understand what you propose here.  I appreciate that
> I'm adding functionality for Cuirass to the hydra file where in itself
> that does not make much sense...
>
> Otoh, I don't see how to move this functionality to
> cuirass/gnu-system.scm only without duplicating much of `hydra-jobs'; so
> that's probably not what you mean... / somewhat confused here. ;-)

Never mind, it *was* confusing.  :-)

Last nitpicking:

> +(define (entry->sexp-entry o)
> +  (match o
> +    (($ <license> name uri comment)
> +     `((name . ,name) (uri . ,uri) (comment . ,comment)))
> +    (_ o)))

[...]

> --- a/guix/licenses.scm
> +++ b/guix/licenses.scm
> @@ -31,7 +31,8 @@
>  
>  (define-module (guix licenses)
>    #:use-module (srfi srfi-9)
> -  #:export (license? license-name license-uri license-comment
> +  #:export (<license>

I prefer not to export record type descriptors in general, because that
exposes too much of the internals and makes it harder to change the code
afterwards.  For instance, if we change the layout of <license>, and if
<license> is exported, we have to carefully check all users and it’s
easy to get it wrong.  (There’s also the problem that exposing the RTD
makes records forgeable: we no longer control how <license> objects are
created, so we cannot make sure that <license> objects we get are
“genuine.”)

Anyway, with this fixed, OK to push.

After that we should update the config on berlin.guixsd.org to use this
file directly.

Thanks a lot!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.