[bug#31813] [PATCH] evaluate: Use a generic key to identify Cuirass arguments.

[bug#31813] [PATCH] evaluate: Use a generic key to identify Cuirass arguments.

[Clément Lassieur]

So that Cuirass specifications used to build 'guix-modular' can be named
differently than "guix" and "guix-modular" (see Guix's
build-aux/hydra/guix-modular.scm).

The name is used as a primary key, so before that commit, it was also
impossible to have several such specifications.

* bin/evaluate.in (main): Replace custom NAME (passed to PROC) with 'guix'.

Co-authored-by: Mathieu Othacehe <m.othacehe@gmail.com>
---
 bin/evaluate.in | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/bin/evaluate.in b/bin/evaluate.in
index d973c44..86d0e83 100644
--- a/bin/evaluate.in
+++ b/bin/evaluate.in
@@ -6,7 +6,8 @@ exec ${GUILE:-@GUILE@} --no-auto-compile -e main -s "$0" "$@"
 ;;;; evaluate -- convert a specification to a job list
 ;;; Copyright © 2016, 2018 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2016, 2017 Mathieu Lirzin <mthl@gnu.org>
-;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
+;;; Copyright © 2017, 2018 Mathieu Othacehe <m.othacehe@gmail.com>
+;;; Copyright © 2018 Clément Lassieur <clement@lassieur.org>
 ;;;
 ;;; This file is part of Cuirass.
 ;;;
@@ -98,7 +99,7 @@ building things during evaluation~%")
                 (proc    (module-ref %user-module proc-name))
                 (commit  (assq-ref spec #:current-commit))
                 (name    (assq-ref spec #:name))
-                (args    `((,(string->symbol name)
+                (args    `((guix
                             (revision . ,commit)
                             (file-name . ,source))
                            ,@(or (assq-ref spec #:arguments) '())))
-- 
2.17.1

[bug#31813] [PATCH] evaluate: Use a generic key to identify Cuirass arguments.

[Mathieu Othacehe]

Hi,

Thanks to this patch, we are able to build on Cuirass guix package from
multiple source repositories (guix-modular-url1, guix-modular-url2, ...)

and then guix pull --url=url1 or guix pull --url=url2

Cuirass + new "guix pull" is becoming awesome, can't wait to have a
nice web interface :)

Mathieu

Clément Lassieur writes:

> So that Cuirass specifications used to build 'guix-modular' can be named
> differently than "guix" and "guix-modular" (see Guix's
> build-aux/hydra/guix-modular.scm).
>
> The name is used as a primary key, so before that commit, it was also
> impossible to have several such specifications.
>
> * bin/evaluate.in (main): Replace custom NAME (passed to PROC) with 'guix'.
>
> Co-authored-by: Mathieu Othacehe <m.othacehe@gmail.com>
> ---
>  bin/evaluate.in | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/bin/evaluate.in b/bin/evaluate.in
> index d973c44..86d0e83 100644
> --- a/bin/evaluate.in
> +++ b/bin/evaluate.in
> @@ -6,7 +6,8 @@ exec ${GUILE:-@GUILE@} --no-auto-compile -e main -s "$0" "$@"
>  ;;;; evaluate -- convert a specification to a job list
>  ;;; Copyright © 2016, 2018 Ludovic Courtès <ludo@gnu.org>
>  ;;; Copyright © 2016, 2017 Mathieu Lirzin <mthl@gnu.org>
> -;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
> +;;; Copyright © 2017, 2018 Mathieu Othacehe <m.othacehe@gmail.com>
> +;;; Copyright © 2018 Clément Lassieur <clement@lassieur.org>
>  ;;;
>  ;;; This file is part of Cuirass.
>  ;;;
> @@ -98,7 +99,7 @@ building things during evaluation~%")
>                  (proc    (module-ref %user-module proc-name))
>                  (commit  (assq-ref spec #:current-commit))
>                  (name    (assq-ref spec #:name))
> -                (args    `((,(string->symbol name)
> +                (args    `((guix
>                              (revision . ,commit)
>                              (file-name . ,source))
>                             ,@(or (assq-ref spec #:arguments) '())))

[bug#31813] [PATCH] evaluate: Use a generic key to identify Cuirass arguments.

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 1664 bytes --]

Heya!

Clément Lassieur <clement@lassieur.org> skribis:

> So that Cuirass specifications used to build 'guix-modular' can be named
> differently than "guix" and "guix-modular" (see Guix's
> build-aux/hydra/guix-modular.scm).
>
> The name is used as a primary key, so before that commit, it was also
> impossible to have several such specifications.

[...]

> diff --git a/bin/evaluate.in b/bin/evaluate.in
> index d973c44..86d0e83 100644
> --- a/bin/evaluate.in
> +++ b/bin/evaluate.in
> @@ -6,7 +6,8 @@ exec ${GUILE:-@GUILE@} --no-auto-compile -e main -s "$0" "$@"
>  ;;;; evaluate -- convert a specification to a job list
>  ;;; Copyright © 2016, 2018 Ludovic Courtès <ludo@gnu.org>
>  ;;; Copyright © 2016, 2017 Mathieu Lirzin <mthl@gnu.org>
> -;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
> +;;; Copyright © 2017, 2018 Mathieu Othacehe <m.othacehe@gmail.com>
> +;;; Copyright © 2018 Clément Lassieur <clement@lassieur.org>
>  ;;;
>  ;;; This file is part of Cuirass.
>  ;;;
> @@ -98,7 +99,7 @@ building things during evaluation~%")
>                  (proc    (module-ref %user-module proc-name))
>                  (commit  (assq-ref spec #:current-commit))
>                  (name    (assq-ref spec #:name))
> -                (args    `((,(string->symbol name)
> +                (args    `((guix
>                              (revision . ,commit)
>                              (file-name . ,source))
>                             ,@(or (assq-ref spec #:arguments) '())))

If we do that, then everything is called ‘guix’.

Shouldn’t we instead change the schema along these lines?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 590 bytes --]

diff --git a/src/schema.sql b/src/schema.sql
index 65aebbd..bad2f6d 100644
--- a/src/schema.sql
+++ b/src/schema.sql
@@ -1,7 +1,7 @@
 BEGIN TRANSACTION;
 
 CREATE TABLE Specifications (
-  repo_name     TEXT NOT NULL PRIMARY KEY,
+  repo_name     TEXT NOT NULL,
   url           TEXT NOT NULL,
   load_path     TEXT NOT NULL,
   file          TEXT NOT NULL,
@@ -11,7 +11,8 @@ CREATE TABLE Specifications (
   branch        TEXT,
   tag           TEXT,
   revision      TEXT,
-  no_compile_p  INTEGER
+  no_compile_p  INTEGER,
+  PRIMARY KEY (repo_name, branch)
 );
 
 CREATE TABLE Stamps (

[-- Attachment #3: Type: text/plain, Size: 678 bytes --]


?

That way we can have one ‘guix-modular’ job for each branch, for example.

Mathieu Othacehe <m.othacehe@gmail.com> skribis:

> Thanks to this patch, we are able to build on Cuirass guix package from
> multiple source repositories (guix-modular-url1, guix-modular-url2, ...)
>
> and then guix pull --url=url1 or guix pull --url=url2

Neat!  So you have a Cuirass setup that works well for you?  I’m asking
because I’m not fully satisfied with what we have on berlin, but part of
the issues come from offloading to 20+ machines.

> Cuirass + new "guix pull" is becoming awesome, can't wait to have a
> nice web interface :)

Same here!

Ludo’.

[bug#31813] [PATCH] evaluate: Use a generic key to identify Cuirass arguments.

[Clément Lassieur]

Heya :-)

Ludovic Courtès <ludo@gnu.org> writes:

>> @@ -98,7 +99,7 @@ building things during evaluation~%")
>>                  (proc    (module-ref %user-module proc-name))
>>                  (commit  (assq-ref spec #:current-commit))
>>                  (name    (assq-ref spec #:name))
>> -                (args    `((,(string->symbol name)
>> +                (args    `((guix
>>                              (revision . ,commit)
>>                              (file-name . ,source))
>>                             ,@(or (assq-ref spec #:arguments) '())))
>
> If we do that, then everything is called ‘guix’.

Why is it a problem?

We need a sure way to distinguish the 'guix-checkout' argument from the
other ones being appended ((systems "x86_64-linux") in our case).  We
thought about choosing a more descriptive name, like 'guix-checkout',
but that would require modifying Guix's build-aux/hydra/guix-modular.scm
like this: (which is probably fine)

--8<---------------cut here---------------start------------->8---
    (define guix-checkout
      (or (assq-ref arguments 'guix)                ;Hydra on hydra
  -       (assq-ref arguments 'guix-modular)))      ;Cuirass on berlin
  +       (assq-ref arguments 'guix-checkout)))     ;Cuirass on berlin
--8<---------------cut here---------------end--------------->8---

I don't think the current situation is good because:
  - a simple mistake from the user gets their build task to silently
    vanish,
  - it is inconvenient to use guix-modular.scm with several different
    specifications,
  - that #:name key is useless if users can't choose a custom name,
  - allowing custom names would make it way easier to understand
    /api/latestbuilds.  For an example with custom names, see
    https://cuirass.lassieur.org:8081/api/latestbuilds?nr=100.

> Shouldn’t we instead change the schema along these lines?
>
> diff --git a/src/schema.sql b/src/schema.sql
> index 65aebbd..bad2f6d 100644
> --- a/src/schema.sql
> +++ b/src/schema.sql
> @@ -1,7 +1,7 @@
>  BEGIN TRANSACTION;
>  
>  CREATE TABLE Specifications (
> -  repo_name     TEXT NOT NULL PRIMARY KEY,
> +  repo_name     TEXT NOT NULL,
>    url           TEXT NOT NULL,
>    load_path     TEXT NOT NULL,
>    file          TEXT NOT NULL,
> @@ -11,7 +11,8 @@ CREATE TABLE Specifications (
>    branch        TEXT,
>    tag           TEXT,
>    revision      TEXT,
> -  no_compile_p  INTEGER
> +  no_compile_p  INTEGER,
> +  PRIMARY KEY (repo_name, branch)
>  );
>  
>  CREATE TABLE Stamps (
>
> ?
>
> That way we can have one ‘guix-modular’ job for each branch, for example.

I don't think it would work because our Specifications table looks like
this:

--8<---------------cut here---------------start------------->8---
sqlite> select * from specifications;
guix-modular-savannah|git://git.savannah.gnu.org/guix.git|.|build-aux/cuirass/guix-modular.scm|cuirass-jobs|((systems "x86_64-linux"))|master|||1
guix-modular-clem|git://git.lassieur.org/guix.git|.|build-aux/cuirass/guix-modular.scm|cuirass-jobs|((systems "x86_64-linux"))|master|||1
guix-manifest-savannah|git://git.savannah.gnu.org/guix.git|.|/gnu/store/iv4p56ja708gdwvj85982srqnx2cz056-cuirass-derivations.scm|drv-list|()|master|||1
guix-manifest-clem|git://git.lassieur.org/guix.git|.|/gnu/store/iv4p56ja708gdwvj85982srqnx2cz056-cuirass-derivations.scm|drv-list|()|master|||1
--8<---------------cut here---------------end--------------->8---

and so we would have two (guix-modular, master) pairs, thus conflicting
with the PRIMARY KEY constraint again.  Using an auto-incrementing ID
column could work, but I don't like it for the reasons I explained
above.

> Mathieu Othacehe <m.othacehe@gmail.com> skribis:
>
>> Thanks to this patch, we are able to build on Cuirass guix package from
>> multiple source repositories (guix-modular-url1, guix-modular-url2, ...)
>>
>> and then guix pull --url=url1 or guix pull --url=url2
>
> Neat!  So you have a Cuirass setup that works well for you?  I’m asking
> because I’m not fully satisfied with what we have on berlin, but part of
> the issues come from offloading to 20+ machines.

:-)  Yes it works well, but we use it only for 5 machines.  And we only
build the packages in our manifests (and guix-modular), which isn't
much.

Clément

[bug#31813] [PATCH] evaluate: Use a generic key to identify Cuirass arguments.

[Ludovic Courtès]

Hey!

Clément Lassieur <clement@lassieur.org> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>>> @@ -98,7 +99,7 @@ building things during evaluation~%")
>>>                  (proc    (module-ref %user-module proc-name))
>>>                  (commit  (assq-ref spec #:current-commit))
>>>                  (name    (assq-ref spec #:name))
>>> -                (args    `((,(string->symbol name)
>>> +                (args    `((guix
>>>                              (revision . ,commit)
>>>                              (file-name . ,source))
>>>                             ,@(or (assq-ref spec #:arguments) '())))
>>
>> If we do that, then everything is called ‘guix’.
>
> Why is it a problem?

In theory you can have several inputs (checkouts) to a given jobset, and
they need to have different names so that you can distinguish among
them.

> I don't think the current situation is good because:
>   - a simple mistake from the user gets their build task to silently
>     vanish,
>   - it is inconvenient to use guix-modular.scm with several different
>     specifications,
>   - that #:name key is useless if users can't choose a custom name,
>   - allowing custom names would make it way easier to understand
>     /api/latestbuilds.  For an example with custom names, see
>     https://cuirass.lassieur.org:8081/api/latestbuilds?nr=100.

I agree with all this.  :-)  I think the custom name should appear in
the arguments passed to the build procedure, though.

>> diff --git a/src/schema.sql b/src/schema.sql
>> index 65aebbd..bad2f6d 100644
>> --- a/src/schema.sql
>> +++ b/src/schema.sql
>> @@ -1,7 +1,7 @@
>>  BEGIN TRANSACTION;
>>  
>>  CREATE TABLE Specifications (
>> -  repo_name     TEXT NOT NULL PRIMARY KEY,
>> +  repo_name     TEXT NOT NULL,
>>    url           TEXT NOT NULL,
>>    load_path     TEXT NOT NULL,
>>    file          TEXT NOT NULL,
>> @@ -11,7 +11,8 @@ CREATE TABLE Specifications (
>>    branch        TEXT,
>>    tag           TEXT,
>>    revision      TEXT,
>> -  no_compile_p  INTEGER
>> +  no_compile_p  INTEGER,
>> +  PRIMARY KEY (repo_name, branch)
>>  );
>>  
>>  CREATE TABLE Stamps (
>>
>> ?
>>
>> That way we can have one ‘guix-modular’ job for each branch, for example.
>
> I don't think it would work because our Specifications table looks like
> this:
>
> sqlite> select * from specifications;
> guix-modular-savannah|git://git.savannah.gnu.org/guix.git|.|build-aux/cuirass/guix-modular.scm|cuirass-jobs|((systems "x86_64-linux"))|master|||1
> guix-modular-clem|git://git.lassieur.org/guix.git|.|build-aux/cuirass/guix-modular.scm|cuirass-jobs|((systems "x86_64-linux"))|master|||1
> guix-manifest-savannah|git://git.savannah.gnu.org/guix.git|.|/gnu/store/iv4p56ja708gdwvj85982srqnx2cz056-cuirass-derivations.scm|drv-list|()|master|||1
> guix-manifest-clem|git://git.lassieur.org/guix.git|.|/gnu/store/iv4p56ja708gdwvj85982srqnx2cz056-cuirass-derivations.scm|drv-list|()|master|||1
>
> and so we would have two (guix-modular, master) pairs, thus conflicting
> with the PRIMARY KEY constraint again.

Good point.

> Using an auto-incrementing ID column could work, but I don't like it
> for the reasons I explained above.

You didn’t mention auto-incrementing ID above, did you?  It would seem
like a simple solution to the problem.

> :-)  Yes it works well, but we use it only for 5 machines.  And we only
> build the packages in our manifests (and guix-modular), which isn't
> much.

Heh, pretty cool.  :-)

Thanks,
Ludo’.

[bug#31813] [PATCH] evaluate: Use a generic key to identify Cuirass arguments.

[Clément Lassieur]

Heya!

Ludovic Courtès <ludo@gnu.org> writes:

>>>> @@ -98,7 +99,7 @@ building things during evaluation~%")
>>>>                  (proc    (module-ref %user-module proc-name))
>>>>                  (commit  (assq-ref spec #:current-commit))
>>>>                  (name    (assq-ref spec #:name))
>>>> -                (args    `((,(string->symbol name)
>>>> +                (args    `((guix
>>>>                              (revision . ,commit)
>>>>                              (file-name . ,source))
>>>>                             ,@(or (assq-ref spec #:arguments) '())))
>>>
>>> If we do that, then everything is called ‘guix’.
>>
>> Why is it a problem?
>
> In theory you can have several inputs (checkouts) to a given jobset, and
> they need to have different names so that you can distinguish among
> them.

Right now 'cuirass-jobs' can't handle several checkouts, and evaluate.in
sends only one 'checkout'.  So if we want to support several inputs we
would need to modify both Guix and Cuirass.

And anyway if we force the argument key to 'guix', 'guix-checkout' or
'guix-modular', it wouldn't prevent us to add other inputs (checkouts)
later, I think.

>> I don't think the current situation is good because:
>>   - a simple mistake from the user gets their build task to silently
>>     vanish,
>>   - it is inconvenient to use guix-modular.scm with several different
>>     specifications,
>>   - that #:name key is useless if users can't choose a custom name,
>>   - allowing custom names would make it way easier to understand
>>     /api/latestbuilds.  For an example with custom names, see
>>     https://cuirass.lassieur.org:8081/api/latestbuilds?nr=100.
>
> I agree with all this.  :-)  I think the custom name should appear in
> the arguments passed to the build procedure, though.

But with our patch it *does* appear in the build procedure, because it
is still in 'eval' which is read by 'evaluate' in src/cuirass/base.scm.

--8<---------------cut here---------------start------------->8---
                (eval    `((#:specification . ,name)
                           (#:revision . ,commit)))
           pretty-print
            `(evaluation ,eval
                         ,(map (lambda (thunk) (thunk))
                               thunks))
--8<---------------cut here---------------end--------------->8---

What we want to modify is the key passed to 'proc', and it is only used
to extract the checkout argument (with 'assq-ref').

>> Using an auto-incrementing ID column could work, but I don't like it
>> for the reasons I explained above.
>
> You didn’t mention auto-incrementing ID above, did you?  It would seem
> like a simple solution to the problem.

If we were to do this,
  - that would allow several specs to have the same name,
  - that would not change the fact that users are forced to use the
    'guix-modular' (or 'guix') name for each spec.

The former is nice but not necessary (it could be a further commit), the
latter is the bug that we want to fix.

Mathieu and Clément

[bug#31813] [PATCH] evaluate: Use a generic key to identify Cuirass arguments.

[Ludovic Courtès]

Hello Clément & Mathieu,

Trying to take a step back and look at how Hydra does things…

Clément Lassieur <clement@lassieur.org> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>>>>> @@ -98,7 +99,7 @@ building things during evaluation~%")
>>>>>                  (proc    (module-ref %user-module proc-name))
>>>>>                  (commit  (assq-ref spec #:current-commit))
>>>>>                  (name    (assq-ref spec #:name))
>>>>> -                (args    `((,(string->symbol name)
>>>>> +                (args    `((guix
>>>>>                              (revision . ,commit)
>>>>>                              (file-name . ,source))
>>>>>                             ,@(or (assq-ref spec #:arguments) '())))
>>>>
>>>> If we do that, then everything is called ‘guix’.
>>>
>>> Why is it a problem?
>>
>> In theory you can have several inputs (checkouts) to a given jobset, and
>> they need to have different names so that you can distinguish among
>> them.

For the record, this calling convention comes from Hydra.  In Hydra, a
“jobset” can have several “inputs”, and they all show up in this ‘args’
list, like:

  ((INPUT1 . PROPERTIES) (INPUT2 . PROPERTIES) …)

where INPUT1 is the famous name shown above.  (See
<https://github.com/NixOS/hydra/blob/master/src/script/hydra-eval-guile-jobs.in>.)

> And anyway if we force the argument key to 'guix', 'guix-checkout' or
> 'guix-modular', it wouldn't prevent us to add other inputs (checkouts)
> later, I think.

It would prevent us from distinguishing between different inputs.
Currently, at least with Hydra, we can do:

  (assoc-ref args 'some-input)

and get the file-name, revision, etc. properties corresponding to
‘some-input’.

So I think we should preserve this API.

The problem we have though is that Cuirass has no notion of “input”.  In
Hydra’s schema, there’s ‘JobsetInputs’ (that’s where we get the input
name from in the ‘args’ alist above), which is separate from ‘Jobset’
(roughly equivalent to ‘Specifications’ in Cuirass.)

  https://github.com/NixOS/hydra/blob/master/src/sql/hydra.sql

Perhaps what we should do is introduce an ‘Input’ table to begin with,
and have ‘Specifications’ refer to one or more of these.

How does that sound?

We can apply your patch in the meantime, so that we can effectively have
several ‘guix-modular’ jobs for example, but what I mean to say is that
it can only be a temporary workaround for a flaw that needs to be fixed.

Thanks for your patience.  :-)

Ludo’.

bug#31813: [PATCH] evaluate: Use a generic key to identify Cuirass arguments.

[Clément Lassieur]

Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

> For the record, this calling convention comes from Hydra.  In Hydra, a
> “jobset” can have several “inputs”, and they all show up in this ‘args’
> list, like:
>
>   ((INPUT1 . PROPERTIES) (INPUT2 . PROPERTIES) …)
>
> where INPUT1 is the famous name shown above.  (See
> <https://github.com/NixOS/hydra/blob/master/src/script/hydra-eval-guile-jobs.in>.)
>
>> And anyway if we force the argument key to 'guix', 'guix-checkout' or
>> 'guix-modular', it wouldn't prevent us to add other inputs (checkouts)
>> later, I think.
>
> It would prevent us from distinguishing between different inputs.
> Currently, at least with Hydra, we can do:
>
>   (assoc-ref args 'some-input)
>
> and get the file-name, revision, etc. properties corresponding to
> ‘some-input’.
>
> So I think we should preserve this API.
>
> The problem we have though is that Cuirass has no notion of “input”.  In
> Hydra’s schema, there’s ‘JobsetInputs’ (that’s where we get the input
> name from in the ‘args’ alist above), which is separate from ‘Jobset’
> (roughly equivalent to ‘Specifications’ in Cuirass.)
>
>   https://github.com/NixOS/hydra/blob/master/src/sql/hydra.sql
>
> Perhaps what we should do is introduce an ‘Input’ table to begin with,
> and have ‘Specifications’ refer to one or more of these.
>
> How does that sound?

Excellent!  That would be the fix for another issue we have: our
specifications also depend on our custom packages repository, which
would be another input (if my understanding is correct).  I'll start
working on it.

> We can apply your patch in the meantime, so that we can effectively have
> several ‘guix-modular’ jobs for example, but what I mean to say is that
> it can only be a temporary workaround for a flaw that needs to be fixed.

Sure, I understand this.  I pushed it.

> Thanks for your patience.  :-)

Thanks for yours :-)

I'm closing this ticket, and I'll open a new one for the 'real' patch.

Clément