Web interface pushed

Web interface pushed

[Clément Lassieur]

Hi Tatiana,

So, I did the pagination review and I pushed your work tonight.  :-)

I added checks to avoid crashes when the table is empty (FIRST and LAST
expect non-empty lists).

I modified a few minor things too:

- the commit message, so that it matches our convention
- the indentation
- I removed code comments, trailing '\' in SQL queries, useless
  newlines, useless exports
- I renamed a few things (e.g. thing-list with things, %pagesize with
  %page-size)
- I replaced (+ 1 x) with (1+ x)
- I used string-join to avoid long strings
- I used FIRST instead of CAR when used with LAST (for more consistency,
  but it's the exact same thing)
- I replaced FIRST and LAST with BUILD-ID and BUILD-STOPTIME, so to make
  it more furure-proof and easier to understand
- I used a format string for RESPOND-HTML (to avoid "\"\"")
- I finally opted for a non-parameter %page-size (yes, I changed my mind
  :-), I just didn't see any reason to use one)
- I removed ('page (string->number param)) from REQUEST-PARAMETERS (I
  think it was useless)
- I added a missing copyright header

And that's all!

Thanks for this work, it'll be very useful.  Don't hesitate send new
patches to improve it!

Best regards,
Clément

Re: Web interface pushed

[Ludovic Courtès]

Hello,

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

> So, I did the pagination review and I pushed your work tonight.  :-)

\o/  Thumbs up to Tatiana for all the great work and to Clément for the
review and final polishing!  Really cool (and not so common!) to have
code on ‘master’ before GSoC is over.

I hope we can deploy it soon on berlin.guixsd.org.

Ludo’.

Re: Web interface pushed

[amirouche]

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


Hi
On Monday, July 30, 2018 00:08 CEST, Clément Lassieur <clement@lassieur.org> wrote:
 Hi Tatiana,

So, I did the pagination review and I pushed your work tonight. :-)

I added checks to avoid crashes when the table is empty (FIRST and LAST
expect non-empty lists).

I modified a few minor things too:

- the commit message, so that it matches our convention
- the indentation
- I removed code comments, trailing '\' in SQL queries, useless
newlines, useless exports
- I renamed a few things (e.g. thing-list with things, %pagesize with
%page-size)
- I replaced (+ 1 x) with (1+ x)
- I used string-join to avoid long strings
- I used FIRST instead of CAR when used with LAST (for more consistency,
but it's the exact same thing)
- I replaced FIRST and LAST with BUILD-ID and BUILD-STOPTIME, so to make
it more furure-proof and easier to understand
- I used a format string for RESPOND-HTML (to avoid "\"\"")
- I finally opted for a non-parameter %page-size (yes, I changed my mind
:-), I just didn't see any reason to use one)
- I removed ('page (string->number param)) from REQUEST-PARAMETERS (I
think it was useless)
- I added a missing copyright header

And that's all!

Thanks for this work, it'll be very useful. Don't hesitate send new
patches to improve it!

Best regards,
Clément
 How can I test?


 

[-- Attachment #2: Type: text/html, Size: 1622 bytes --]

Re: Web interface pushed

[Clément Lassieur]

Hi,

amirouche@hypermove.net <amirouche@hypermove.net> writes:

> Hi
>
> How can I test?

On https://cuirass.lassieur.org:8081/ for now, and on
https://berlin.guixsd.org/ when Ricardo guix pulls and guix reconfigures
it :-)

Clément

Re: Web interface pushed

[Alex Sassmannshausen]

Wow, that looks beautiful! Congratulations to all involved, and
especially Tatiana :D

Alex


Clément Lassieur writes:

> Hi,
>
> amirouche@hypermove.net <amirouche@hypermove.net> writes:
>
>> Hi
>>
>> How can I test?
>
> On https://cuirass.lassieur.org:8081/ for now, and on
> https://berlin.guixsd.org/ when Ricardo guix pulls and guix reconfigures
> it :-)
>
> Clément

Re: Web interface pushed

[Nils Gillmann]

Wow, good work Tatiana!

Alex Sassmannshausen transcribed 384 bytes:
> Wow, that looks beautiful! Congratulations to all involved, and
> especially Tatiana :D
> 
> Alex
> 
> 
> Clément Lassieur writes:
> 
> > Hi,
> >
> > amirouche@hypermove.net <amirouche@hypermove.net> writes:
> >
> >> Hi
> >>
> >> How can I test?
> >
> > On https://cuirass.lassieur.org:8081/ for now, and on
> > https://berlin.guixsd.org/ when Ricardo guix pulls and guix reconfigures
> > it :-)
> >
> > Clément
> 
> 

Re: Web interface pushed

[Ricardo Wurmus]

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

> Hi,
>
> amirouche@hypermove.net <amirouche@hypermove.net> writes:
>
>> Hi
>>
>> How can I test?
>
> On https://cuirass.lassieur.org:8081/ for now, and on
> https://berlin.guixsd.org/ when Ricardo guix pulls and guix reconfigures
> it :-)

I just reconfigured berlin.guixsd.org.  The web interface is now
available there.

Thanks Tatiana and Clément!

--
Ricardo

Re: Web interface pushed

[Gábor Boskovits]

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

Ricardo Wurmus <rekado@elephly.net> ezt írta (időpont: 2018. júl. 30., H,
14:09):

>
> Clément Lassieur <clement@lassieur.org> writes:
>
> > Hi,
> >
> > amirouche@hypermove.net <amirouche@hypermove.net> writes:
> >
> >> Hi
> >>
> >> How can I test?
> >
> > On https://cuirass.lassieur.org:8081/ for now, and on
> > https://berlin.guixsd.org/ when Ricardo guix pulls and guix reconfigures
> > it :-)
>
> I just reconfigured berlin.guixsd.org.  The web interface is now
> available there.
>
> Thanks Tatiana and Clément!
>
--
> Ricardo
>
> Thanks for all! Nice work!

[-- Attachment #2: Type: text/html, Size: 1467 bytes --]

Re: Web interface pushed

[Pjotr Prins]

On Mon, Jul 30, 2018 at 01:46:10AM +0200, Ludovic Courtès wrote:
> Hello,
> 
> Clément Lassieur <clement@lassieur.org> skribis:
> 
> > So, I did the pagination review and I pushed your work tonight.  :-)
> 
> \o/  Thumbs up to Tatiana for all the great work and to Clément for the
> review and final polishing!  Really cool (and not so common!) to have
> code on ‘master’ before GSoC is over.

+1.

One comment: I think we should strive to design GSoC programmes in a
way that students get to push earlier to trunk. I know Guix can be
complex, but even so, I think we would have less failure if we make it
small pieces and better gratification. Maybe this year's students can
say something about that here.

Pj.

Re: Web interface pushed

[Ricardo Wurmus]

Hi Pjotr,

> One comment: I think we should strive to design GSoC programmes in a
> way that students get to push earlier to trunk. I know Guix can be
> complex, but even so, I think we would have less failure if we make it
> small pieces and better gratification.

This has always been our goal.  The projects can usually be implemented
in independent chunks that could be merged into the “master” branch at
various stages.

--
Ricardo

Re: Web interface pushed

[Hartmut Goebel]

Am 30.07.2018 um 11:11 schrieb Clément Lassieur:
> On https://cuirass.lassieur.org:8081/ for now, and on
> https://berlin.guixsd.org/ when Ricardo guix pulls and guix reconfigures
> it :-)

Wow, this is much, much quicker then the old UI. I'm just curious how I
could get the build log. Anything I misses?

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: Web interface pushed

[Clément Lassieur]

Hartmut Goebel <h.goebel@crazy-compilers.com> writes:

> Am 30.07.2018 um 11:11 schrieb Clément Lassieur:
>> On https://cuirass.lassieur.org:8081/ for now, and on
>> https://berlin.guixsd.org/ when Ricardo guix pulls and guix reconfigures
>> it :-)
>
> Wow, this is much, much quicker then the old UI. I'm just curious how I
> could get the build log. Anything I misses?

It's a todo :-)

Re: Web interface pushed

[Amirouche Boubekki]

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

Le lun. 30 juil. 2018 à 18:03, Clément Lassieur <clement@lassieur.org> a
écrit :

> Hartmut Goebel <h.goebel@crazy-compilers.com> writes:
>
> > Am 30.07.2018 um 11:11 schrieb Clément Lassieur:
> >> On https://cuirass.lassieur.org:8081/ for now, and on
> >> https://berlin.guixsd.org/ when Ricardo guix pulls and guix
> reconfigures
> >> it :-)
> >
> > Wow, this is much, much quicker then the old UI. I'm just curious how I
> > could get the build log. Anything I misses?
>
> It's a todo :-)
>

(display "win\n")

[-- Attachment #2: Type: text/html, Size: 1102 bytes --]

Re: Web interface pushed

[Tatiana Sholokhova]

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

Hi all,

Thank you for the congratulations! I am excited to have the web-interface
pushed into the master branch. I would like to thank you all for your great
support. Special thanks to Clément for helpful final reviews and merging
all the changes together.

Now I would like to add some more features to the interface to enhance it
further. Am I right that now I should again organize my changes as a new
separate branch?

Best regards,
Tatiana

On Mon, 30 Jul 2018 at 15:27, Ricardo Wurmus <rekado@elephly.net> wrote:

>
> Hi Pjotr,
>
> > One comment: I think we should strive to design GSoC programmes in a
> > way that students get to push earlier to trunk. I know Guix can be
> > complex, but even so, I think we would have less failure if we make it
> > small pieces and better gratification.
>
> This has always been our goal.  The projects can usually be implemented
> in independent chunks that could be merged into the “master” branch at
> various stages.
>
> --
> Ricardo
>
>

[-- Attachment #2: Type: text/html, Size: 1362 bytes --]

Re: Web interface pushed

[Clément Lassieur]

Hi Tatiana,

Tatiana Sholokhova <tanja201396@gmail.com> writes:

> Hi all,
>
> Thank you for the congratulations! I am excited to have the web-interface
> pushed into the master branch. I would like to thank you all for your great
> support. Special thanks to Clément for helpful final reviews and merging
> all the changes together.

:-)

> Now I would like to add some more features to the interface to enhance it
> further.

That would be great!

> Am I right that now I should again organize my changes as a new
> separate branch?

It's worth doing a separate branch if the change is large and
complicated, but I think the best is to add things little by little in a
consistent way.  Then you can use 'git format-patch' to generate a patch
file, and send it to the mailing list.  Or you can use 'git send-email'.
Don't hesitate to ask help about those commands.  Most people send
patches to the patch mailing list (guix-patches@gnu.org) because it's
easier to keep track of them (there is an interface:
https://debbugs.gnu.org/db/pa/lguix-patches.html), but in the end, it
doesn't really matter :-) so do as you wish.

Clément

Re: Web interface pushed

[Gábor Boskovits]

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

Clément Lassieur <clement@lassieur.org> ezt írta (időpont: 2018. aug. 2.,
Cs, 8:54):

> Hi Tatiana,
>
> Tatiana Sholokhova <tanja201396@gmail.com> writes:
>
> > Hi all,
> >
> > Thank you for the congratulations! I am excited to have the web-interface
> > pushed into the master branch. I would like to thank you all for your
> great
> > support. Special thanks to Clément for helpful final reviews and merging
> > all the changes together.
>
> :-)
>
> > Now I would like to add some more features to the interface to enhance it
> > further.
>
> >Hello Tatiana, did you meet my writeup?
>I believe that a filtering function would be a very useful addition.
>Also most of the time the results differing from the results in the
previous evaluations are the most interesting.
>Can we get this kind of information somehow?
>Also, do you think, that a page with a possibility to download the build
log is possible?


> That would be great!
>
> Am I right that now I should again organize my changes as a new
> > separate branch?
>
> It's worth doing a separate branch if the change is large and
> complicated, but I think the best is to add things little by little in a
> consistent way.  Then you can use 'git format-patch' to generate a patch
> file, and send it to the mailing list.  Or you can use 'git send-email'.
> Don't hesitate to ask help about those commands.  Most people send
> patches to the patch mailing list (guix-patches@gnu.org) because it's
> easier to keep track of them (there is an interface:
> https://debbugs.gnu.org/db/pa/lguix-patches.html), but in the end, it
> doesn't really matter :-) so do as you wish.
>
> Clément
>
>

[-- Attachment #2: Type: text/html, Size: 2578 bytes --]

Re: Web interface pushed

[Clément Lassieur]

Hi Gábor,

Gábor Boskovits <boskovits@gmail.com> writes:

> Also most of the time the results differing from the results in the
> previous evaluations are the most interesting.
> Can we get this kind of information somehow?

About that, I am preparing a commit that would get Cuirass to stop
building all derivations, but only the ones that don't exist yet.
That's https://bugs.gnu.org/32190.  For example, Cuirass wouldn't build
anything when there is a new commit that just updates the documentation,
and the green red and grey buttons would show 0, 0, 0.  Is that what you
meant?

Clément

Re: Web interface pushed

[Amirouche Boubekki]

[-- Attachment #1.1: Type: text/plain, Size: 9042 bytes --]

I read some of the code, I like it :)

diff --git a/src/cuirass/base.scm b/src/cuirass/base.scm
index 82f49a4..af24a28 100644
--- a/src/cuirass/base.scm
+++ b/src/cuirass/base.scm
@@ -20,6 +20,10 @@
 ;;; You should have received a copy of the GNU General Public License
 ;;; along with Cuirass.  If not, see <http://www.gnu.org/licenses/>.

+;; Comment:
+
+;; This is called base because it interface with guix daemon.
+
 (define-module (cuirass base)
   #:use-module (fibers)
   #:use-module (cuirass logging)
diff --git a/src/cuirass/http.scm b/src/cuirass/http.scm
index 2d66ff9..0899cd1 100644
--- a/src/cuirass/http.scm
+++ b/src/cuirass/http.scm
@@ -117,6 +117,13 @@
     (map build->hydra-build builds)))

 (define (request-parameters request)
+  ;; REVIEW: this 'parameters' is ambigious in aiohttp. It's called
+  ;; request.query because the part after ? in the URL is called the "query
+  ;; string". Also parameters can be passed via the path part of the URL.
+
+  ;; here is what I use
+  ;;
https://github.com/a-guile-mind/guile-web/blob/master/src/web/decode.scm
+
   "Parse the REQUEST query parameters and return them under the form
   '((parameter . value) ...)."
   (let* ((uri (request-uri request))
@@ -125,11 +132,14 @@
         (map (lambda (param)
                (match (string-split param #\=)
                  ((key param)
-                  (let ((key-symbol (string->symbol key)))
+                  (let ((key-symbol (string->symbol (uri-decode key))))
                     (cons key-symbol
                           (match key-symbol
-                            ('id (string->number param))
-                            ('nr (string->number param))
+                                 ('id (string->number (uri-decode param)))
+                                 ;; I don't think this will scale in terms
of
+                                 ;; kind query pairs where the key part
can be
+                                 ;; many different things
+                            ('nr (string->number (uri-decode param)))
                             (_   param)))))))
              (string-split query #\&))
         '())))
@@ -138,9 +148,10 @@
 ;;;
 ;;; Web server.
 ;;;
-;;; The api is derived from the hydra one. It is partially described here :
+;;; The exposed api is derived from the hydra one. It is partially
described
+;;; here :
 ;;;
-;;; https://github.com/NixOS/hydra/blob/master/doc/manual/api.xml
+;;;   https://github.com/NixOS/hydra/blob/master/doc/manual/api.xml
 ;;;

 (define (request-path-components request)
@@ -217,6 +228,8 @@
                     (with-critical-section db-channel (db)
                       (db-get-specifications db)))))
     (("build" build-id)
+     ;; REVIEW: don't inline that much code in same procedure aka.
procedures
+     ;; for the win1
      (let ((hydra-build
             (with-critical-section db-channel (db)
               (handle-build-request db (string->number build-id)))))
@@ -234,6 +247,7 @@
               (let ((uri (string->uri-reference
                           (string-append "/log/"
                                          (basename output)))))
+                ;; create a procedure for that when you will have forms.
                 (respond (build-response #:code 302
                                          #:headers `((location . ,uri)))
                          #:body "")))
@@ -241,7 +255,10 @@
               ;; Not entry for BUILD-ID in the 'Outputs' table.
               (respond-json-with-error
                500
+               ;; 500 is very strong error, it's must not be used for
expected failure
+               ;; by the way this is rendering logic, it should be in it's
own procedure.
                (format #f "Outputs of build ~a are unknown." build-id)))
+
              (#f
               (respond-build-not-found build-id)))
            (respond-build-not-found build-id))))
@@ -251,13 +268,25 @@
             (nr (assq-ref params 'nr)))
        (if nr
            (respond-json (object->json-string
+                          ;; don't nest here the code. Because http
handlers should follow
+                          ;; the following pattern:
+                          ;;
+                          ;;   (define (handler request body)
+                          ;;      (let ((input (snarf-input request body)))
+                          ;;          (shallow-validate-with-exception
input)
+                          ;;          (deep-validate-with-exception input
(db))
+                          ;;          (let ((out (domain-code-goes-here)))
+                          ;;             (sxml->response (handler-template
out))))
+                          ;;
+                          ;; Basically, the current code requires top down
and bottom up
+                          ;; reading. Declaring intermediate variables
helps readbility.
                           (with-critical-section db-channel (db)
                             (db-get-evaluations db nr))))
-           (respond-json-with-error 500 "Parameter not defined!"))))
+           (respond-json-with-error 400 "Parameter not defined!"))))  ;;
aka. bad request
     (("api" "latestbuilds")
      (let* ((params (request-parameters request))
             ;; 'nr parameter is mandatory to limit query size.
-            (valid-params? (assq-ref params 'nr)))
+            (valid-params? (assq-ref params 'nr)))  ;; or exception?
        (if valid-params?
            ;; Limit results to builds that are "done".
            (respond-json
@@ -359,7 +388,10 @@
     ;; process each client request and then directly go back waiting for
the
     ;; next client (conversely, Guile's 'run-server' loop processes clients
     ;; one after another, sequentially.)  We can do that because we don't
-    ;; maintain any state across connections.
+    ;; maintain any state across connections.  I don't understand that
+    ;; comment. Why no state across connections? isn't sqlite that stores
+    ;; states? Does it mean there is no global mutable in memory data in
+    ;; cuirass?
     ;;
     ;; XXX: We don't do 'call-with-sigint' like 'run-server' does.
     (let* ((impl (lookup-server-impl 'fiberized))
diff --git a/src/schema.sql b/src/schema.sql
index eb0f7e9..769360f 100644
--- a/src/schema.sql
+++ b/src/schema.sql
@@ -1,5 +1,8 @@
 BEGIN TRANSACTION;

+-- REVIEW: The name of the table must be all small caps.
+-- COMMENT: Some people argue table name must be singular
+--          but I disagree, both might work.
 CREATE TABLE Specifications (
   name          TEXT NOT NULL PRIMARY KEY,
   load_path_inputs TEXT NOT NULL, -- list of input names whose load path
will be in Guile's %load-path
@@ -64,7 +67,7 @@ CREATE TABLE Builds (
   log           TEXT NOT NULL,
   status        INTEGER NOT NULL,
   timestamp     INTEGER NOT NULL,
-  starttime     INTEGER NOT NULL,
+  starttime     INTEGER NOT NULL,  -- REVIEW: why not start_time?
   stoptime      INTEGER NOT NULL,
   FOREIGN KEY (derivation) REFERENCES Derivations (derivation),
   FOREIGN KEY (evaluation) REFERENCES Evaluations (id)
@@ -75,5 +78,12 @@ CREATE TABLE Builds (
 CREATE INDEX Builds_Derivations_index ON Builds(status ASC, timestamp ASC,
id, derivation, evaluation, stoptime DESC);
 CREATE INDEX Inputs_index ON Inputs(specification, name, branch);
 CREATE INDEX Derivations_index ON Derivations(derivation, evaluation,
job_name, system);
+-- COMMENT: Indices make the following trade off: slow down writes and take
+--          more space (disk and memory) and in prize you get faster reads.
+-- COMMENT: you'd better add CREATE INDEX just below the table that they
+--          speed up queries for.
+

 COMMIT;
+
+--


Le mer. 1 août 2018 à 21:47, Tatiana Sholokhova <tanja201396@gmail.com> a
écrit :

> Hi all,
>
> Thank you for the congratulations! I am excited to have the web-interface
> pushed into the master branch. I would like to thank you all for your great
> support. Special thanks to Clément for helpful final reviews and merging
> all the changes together.
>
> Now I would like to add some more features to the interface to enhance it
> further. Am I right that now I should again organize my changes as a new
> separate branch?
>
> Best regards,
> Tatiana
>
> On Mon, 30 Jul 2018 at 15:27, Ricardo Wurmus <rekado@elephly.net> wrote:
>
>>
>> Hi Pjotr,
>>
>> > One comment: I think we should strive to design GSoC programmes in a
>> > way that students get to push earlier to trunk. I know Guix can be
>> > complex, but even so, I think we would have less failure if we make it
>> > small pieces and better gratification.
>>
>> This has always been our goal.  The projects can usually be implemented
>> in independent chunks that could be merged into the “master” branch at
>> various stages.
>>
>> --
>> Ricardo
>>
>>

[-- Attachment #1.2: Type: text/html, Size: 11987 bytes --]

[-- Attachment #2: cuirass.amz3.20180802.diff --]
[-- Type: text/x-patch, Size: 8332 bytes --]

diff --git a/src/cuirass/base.scm b/src/cuirass/base.scm
index 82f49a4..af24a28 100644
--- a/src/cuirass/base.scm
+++ b/src/cuirass/base.scm
@@ -20,6 +20,10 @@
 ;;; You should have received a copy of the GNU General Public License
 ;;; along with Cuirass.  If not, see <http://www.gnu.org/licenses/>.
 
+;; Comment:
+
+;; This is called base because it interface with guix daemon.
+
 (define-module (cuirass base)
   #:use-module (fibers)
   #:use-module (cuirass logging)
diff --git a/src/cuirass/database.scm b/src/cuirass/database.scm
index 56f421d..ccb8309 100644
--- a/src/cuirass/database.scm
+++ b/src/cuirass/database.scm
@@ -547,6 +547,7 @@ Assumes that if group id stays the same the group headers stay the same."
                    ;; before those in 'scheduled' state (-2).
                    "status DESC, timestamp DESC")
                   (_ "id DESC")))
+         ;; TODO: create macro to read the following SQL query from something-get-builds.sql
          (stmt-text (format #f "SELECT * FROM (
 SELECT Builds.id, Outputs.name, Outputs.path, Builds.timestamp,
 Builds.starttime, Builds.stoptime, Builds.log, Builds.status,
diff --git a/src/cuirass/http.scm b/src/cuirass/http.scm
index 2d66ff9..0899cd1 100644
--- a/src/cuirass/http.scm
+++ b/src/cuirass/http.scm
@@ -117,6 +117,13 @@
     (map build->hydra-build builds)))
 
 (define (request-parameters request)
+  ;; REVIEW: this 'parameters' is ambigious in aiohttp. It's called
+  ;; request.query because the part after ? in the URL is called the "query
+  ;; string". Also parameters can be passed via the path part of the URL.
+
+  ;; here is what I use
+  ;; https://github.com/a-guile-mind/guile-web/blob/master/src/web/decode.scm
+
   "Parse the REQUEST query parameters and return them under the form
   '((parameter . value) ...)."
   (let* ((uri (request-uri request))
@@ -125,11 +132,14 @@
         (map (lambda (param)
                (match (string-split param #\=)
                  ((key param)
-                  (let ((key-symbol (string->symbol key)))
+                  (let ((key-symbol (string->symbol (uri-decode key))))
                     (cons key-symbol
                           (match key-symbol
-                            ('id (string->number param))
-                            ('nr (string->number param))
+                                 ('id (string->number (uri-decode param)))
+                                 ;; I don't think this will scale in terms of
+                                 ;; kind query pairs where the key part can be
+                                 ;; many different things
+                            ('nr (string->number (uri-decode param)))
                             (_   param)))))))
              (string-split query #\&))
         '())))
@@ -138,9 +148,10 @@
 ;;;
 ;;; Web server.
 ;;;
-;;; The api is derived from the hydra one. It is partially described here :
+;;; The exposed api is derived from the hydra one. It is partially described
+;;; here :
 ;;;
-;;; https://github.com/NixOS/hydra/blob/master/doc/manual/api.xml
+;;;   https://github.com/NixOS/hydra/blob/master/doc/manual/api.xml
 ;;;
 
 (define (request-path-components request)
@@ -217,6 +228,8 @@
                     (with-critical-section db-channel (db)
                       (db-get-specifications db)))))
     (("build" build-id)
+     ;; REVIEW: don't inline that much code in same procedure aka. procedures
+     ;; for the win1
      (let ((hydra-build
             (with-critical-section db-channel (db)
               (handle-build-request db (string->number build-id)))))
@@ -234,6 +247,7 @@
               (let ((uri (string->uri-reference
                           (string-append "/log/"
                                          (basename output)))))
+                ;; create a procedure for that when you will have forms.
                 (respond (build-response #:code 302
                                          #:headers `((location . ,uri)))
                          #:body "")))
@@ -241,7 +255,10 @@
               ;; Not entry for BUILD-ID in the 'Outputs' table.
               (respond-json-with-error
                500
+               ;; 500 is very strong error, it's must not be used for expected failure
+               ;; by the way this is rendering logic, it should be in it's own procedure.
                (format #f "Outputs of build ~a are unknown." build-id)))
+
              (#f
               (respond-build-not-found build-id)))
            (respond-build-not-found build-id))))
@@ -251,13 +268,25 @@
             (nr (assq-ref params 'nr)))
        (if nr
            (respond-json (object->json-string
+                          ;; don't nest here the code. Because http handlers should follow
+                          ;; the following pattern:
+                          ;;
+                          ;;   (define (handler request body)
+                          ;;      (let ((input (snarf-input request body)))
+                          ;;          (shallow-validate-with-exception input)
+                          ;;          (deep-validate-with-exception input (db))
+                          ;;          (let ((out (domain-code-goes-here)))
+                          ;;             (sxml->response (handler-template out))))
+                          ;;
+                          ;; Basically, the current code requires top down and bottom up
+                          ;; reading. Declaring intermediate variables helps readbility.
                           (with-critical-section db-channel (db)
                             (db-get-evaluations db nr))))
-           (respond-json-with-error 500 "Parameter not defined!"))))
+           (respond-json-with-error 400 "Parameter not defined!"))))  ;; aka. bad request
     (("api" "latestbuilds")
      (let* ((params (request-parameters request))
             ;; 'nr parameter is mandatory to limit query size.
-            (valid-params? (assq-ref params 'nr)))
+            (valid-params? (assq-ref params 'nr)))  ;; or exception?
        (if valid-params?
            ;; Limit results to builds that are "done".
            (respond-json
@@ -359,7 +388,10 @@
     ;; process each client request and then directly go back waiting for the
     ;; next client (conversely, Guile's 'run-server' loop processes clients
     ;; one after another, sequentially.)  We can do that because we don't
-    ;; maintain any state across connections.
+    ;; maintain any state across connections.  I don't understand that
+    ;; comment. Why no state across connections? isn't sqlite that stores
+    ;; states? Does it mean there is no global mutable in memory data in
+    ;; cuirass?
     ;;
     ;; XXX: We don't do 'call-with-sigint' like 'run-server' does.
     (let* ((impl (lookup-server-impl 'fiberized))
diff --git a/src/schema.sql b/src/schema.sql
index eb0f7e9..769360f 100644
--- a/src/schema.sql
+++ b/src/schema.sql
@@ -1,5 +1,8 @@
 BEGIN TRANSACTION;
 
+-- REVIEW: The name of the table must be all small caps.
+-- COMMENT: Some people argue table name must be singular
+--          but I disagree, both might work.
 CREATE TABLE Specifications (
   name          TEXT NOT NULL PRIMARY KEY,
   load_path_inputs TEXT NOT NULL, -- list of input names whose load path will be in Guile's %load-path
@@ -64,7 +67,7 @@ CREATE TABLE Builds (
   log           TEXT NOT NULL,
   status        INTEGER NOT NULL,
   timestamp     INTEGER NOT NULL,
-  starttime     INTEGER NOT NULL,
+  starttime     INTEGER NOT NULL,  -- REVIEW: why not start_time?
   stoptime      INTEGER NOT NULL,
   FOREIGN KEY (derivation) REFERENCES Derivations (derivation),
   FOREIGN KEY (evaluation) REFERENCES Evaluations (id)
@@ -75,5 +78,12 @@ CREATE TABLE Builds (
 CREATE INDEX Builds_Derivations_index ON Builds(status ASC, timestamp ASC, id, derivation, evaluation, stoptime DESC);
 CREATE INDEX Inputs_index ON Inputs(specification, name, branch);
 CREATE INDEX Derivations_index ON Derivations(derivation, evaluation, job_name, system);
+-- COMMENT: Indices make the following trade off: slow down writes and take
+--          more space (disk and memory) and in prize you get faster reads.
+-- COMMENT: you'd better add CREATE INDEX just below the table that they
+--          speed up queries for.
+
 
 COMMIT;
+
+--

Re: Web interface pushed

[Gábor Boskovits]

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

Clément Lassieur <clement@lassieur.org> ezt írta (időpont: 2018. aug. 2.,
Cs, 18:49):

> Hi Gábor,
>
> Gábor Boskovits <boskovits@gmail.com> writes:
>
> > Also most of the time the results differing from the results in the
> > previous evaluations are the most interesting.
> > Can we get this kind of information somehow?
>
> About that, I am preparing a commit that would get Cuirass to stop
> building all derivations, but only the ones that don't exist yet.
> That's https://bugs.gnu.org/32190.  For example, Cuirass wouldn't build
> anything when there is a new commit that just updates the documentation,
> and the green red and grey buttons would show 0, 0, 0.  Is that what you
> meant?
>
> > Not exactly. What I would be interested is to get a list of packages that
> previous succeeded, and now they fail, and also a list of packages that
were
> failing, but now build.


> Clément
>

[-- Attachment #2: Type: text/html, Size: 1513 bytes --]

Re: Web interface pushed

[Amirouche Boubekki]

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

Le jeu. 2 août 2018 à 20:49, Gábor Boskovits <boskovits@gmail.com> a écrit :

> Clément Lassieur <clement@lassieur.org> ezt írta (időpont: 2018. aug. 2.,
> Cs, 18:49):
>
>> Hi Gábor,
>>
>> Gábor Boskovits <boskovits@gmail.com> writes:
>>
>> > Also most of the time the results differing from the results in the
>> > previous evaluations are the most interesting.
>> > Can we get this kind of information somehow?
>>
>> About that, I am preparing a commit that would get Cuirass to stop
>> building all derivations, but only the ones that don't exist yet.
>> That's https://bugs.gnu.org/32190.  For example, Cuirass wouldn't build
>> anything when there is a new commit that just updates the documentation,
>> and the green red and grey buttons would show 0, 0, 0.  Is that what you
>> meant?
>>
>> > Not exactly. What I would be interested is to get a list of packages
> that
> > previous succeeded, and now they fail, and also a list of packages that
> were
> > failing, but now build.
>
>
>> Clément
>>
>
You do link the diff of each commit with the files that were impacted by
the change and must be re-evaluated?

[-- Attachment #2: Type: text/html, Size: 1997 bytes --]

Re: Web interface pushed

[Clément Lassieur]

Amirouche Boubekki <amirouche.boubekki@gmail.com> writes:

> You do link the diff of each commit with the files that were impacted by
> the change and must be re-evaluated?

You mean on the web interface?  We can actually see the commit that
triggered a new evaluation, and displaying the diff could be a nice
improvement!

Re: Web interface pushed

[Clément Lassieur]

Amirouche Boubekki <amirouche.boubekki@gmail.com> writes:

> I read some of the code, I like it :)

Thank you four that review Amirouche!  I'll have a look when I find the
time, and reply your questions if I can, but in the meantime, don't
hesitate to send patches. :-)

Re: Web interface pushed

[Ricardo Wurmus]

Hi Tatiana,

> Now I would like to add some more features to the interface to enhance it
> further. Am I right that now I should again organize my changes as a new
> separate branch?

You are welcome to use a new branch that is based on the current
“master” branch.  While it is not necessary to have a remote branch (as
Clément wrote before), I think it might be useful for GSoC, as they
require posting a link to your final work.

Could you please let us know what you plan to work on during the final
weeks?  I’m very excited about getting to play with the features you
will be adding.

--
Ricardo

Re: Web interface pushed

[Tatiana Sholokhova]

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

Hi Ricardo,

The next features I am thinking of are

   - make red, green and grey links on the evaluations page actually
   working and usable for filtering builds of the evaluation with a given
   status;
   - add some navigation tools to improve usability, especially for the
   case when a user wants to go back to the previous level of the interface
   (e.g. from evaluation page to specifications list);
   - add a page with some basic info about a build, containing a link to
   the corresponding build log;

I am going to make a new branch and push these changes in a few days, so
you could try them.

I have a question regarding the link for the final GSOC evaluation.
According to the GSOC requirements
<https://developers.google.com/open-source/gsoc/help/work-product#requirement>,
from the provided URL it should demonstrate the result of the project. I
suppose that it would be nice to include the example of the working
interface running on https://berlin.guixsd.org/ to the final document which
I will prepare for the evaluation (and which also will contain a link to
the list of changes made to the codebase). Would it be appropriate to do
so?

Best regards,
Tatiana

пт, 3 авг. 2018 г. в 15:53, Ricardo Wurmus <rekado@elephly.net>:

>
> Hi Tatiana,
>
> > Now I would like to add some more features to the interface to enhance it
> > further. Am I right that now I should again organize my changes as a new
> > separate branch?
>
> You are welcome to use a new branch that is based on the current
> “master” branch.  While it is not necessary to have a remote branch (as
> Clément wrote before), I think it might be useful for GSoC, as they
> require posting a link to your final work.
>
> Could you please let us know what you plan to work on during the final
> weeks?  I’m very excited about getting to play with the features you
> will be adding.
>
> --
> Ricardo
>
>

[-- Attachment #2: Type: text/html, Size: 2416 bytes --]

Re: Web interface pushed

[Ricardo Wurmus]

Hi Tatiana,

> The next features I am thinking of are
>
>    - make red, green and grey links on the evaluations page actually
>    working and usable for filtering builds of the evaluation with a given
>    status;
>    - add some navigation tools to improve usability, especially for the
>    case when a user wants to go back to the previous level of the interface
>    (e.g. from evaluation page to specifications list);
>    - add a page with some basic info about a build, containing a link to
>    the corresponding build log;

This all sounds very good and manageable.  I think this also matches
some of the open items that Gábor identified earlier.

> I am going to make a new branch and push these changes in a few days, so
> you could try them.

Excellent!

> I have a question regarding the link for the final GSOC evaluation.
> According to the GSOC requirements
> <https://developers.google.com/open-source/gsoc/help/work-product#requirement>,
> from the provided URL it should demonstrate the result of the project. I
> suppose that it would be nice to include the example of the working
> interface running on https://berlin.guixsd.org/ to the final document which
> I will prepare for the evaluation (and which also will contain a link to
> the list of changes made to the codebase). Would it be appropriate to do
> so?

Yes, this would be fine, but note that the document should really stand
on its own.  Whatever is shown on https://berlin.guixsd.org/ will change
over time.  You can add a link to it as an example, but make sure that
your “work product” page would not feel incomplete without it.
(E.g. take screenshots if you want to show specific features instead of
linking to pages on https://berlin.guixsd.org/)

If you want to we could also aim to publish this as a blog post on the
Guix website, but that’s really up to you.  The Guix blog is a good way
to make your work known in the community and to people who are following
the project.

--
Ricardo

Re: Web interface pushed

[Tatiana Sholokhova]

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

Hi Ricardo,

I pushed commits implementing the features we discussed before. Could you
please take a look at them? I have reconsidered the idea of having a
separate page with build information, as I do not see what additional
information we can display on this page except the link to the log file.
So, for now I have added a link to the log file in the newly added column
of the table with information about builds of an evaluation. What do you
think about this?

I thought about writing the summary of and sending it to the mailing list,
but now I think that a blog post would be more convenient for a reader.
What is the source format for guix blog markdown and is where something
special I need to know about preparing the blog post? Also, I would like to
create a draft version of the final document and share it with you in order
to discuss it before publishing. What do you think is the best way to
maintain this draft? I would suggest google doc, but it may be not
compatible with the blog post format.

Best regards,
Tatiana

On Sun, 5 Aug 2018 at 00:03, Ricardo Wurmus <rekado@elephly.net> wrote:

>
> Hi Tatiana,
>
> > The next features I am thinking of are
> >
> >    - make red, green and grey links on the evaluations page actually
> >    working and usable for filtering builds of the evaluation with a given
> >    status;
> >    - add some navigation tools to improve usability, especially for the
> >    case when a user wants to go back to the previous level of the
> interface
> >    (e.g. from evaluation page to specifications list);
> >    - add a page with some basic info about a build, containing a link to
> >    the corresponding build log;
>
> This all sounds very good and manageable.  I think this also matches
> some of the open items that Gábor identified earlier.
>
> > I am going to make a new branch and push these changes in a few days, so
> > you could try them.
>
> Excellent!
>
> > I have a question regarding the link for the final GSOC evaluation.
> > According to the GSOC requirements
> > <
> https://developers.google.com/open-source/gsoc/help/work-product#requirement
> >,
> > from the provided URL it should demonstrate the result of the project. I
> > suppose that it would be nice to include the example of the working
> > interface running on https://berlin.guixsd.org/ to the final document
> which
> > I will prepare for the evaluation (and which also will contain a link to
> > the list of changes made to the codebase). Would it be appropriate to do
> > so?
>
> Yes, this would be fine, but note that the document should really stand
> on its own.  Whatever is shown on https://berlin.guixsd.org/ will change
> over time.  You can add a link to it as an example, but make sure that
> your “work product” page would not feel incomplete without it.
> (E.g. take screenshots if you want to show specific features instead of
> linking to pages on https://berlin.guixsd.org/)
>
> If you want to we could also aim to publish this as a blog post on the
> Guix website, but that’s really up to you.  The Guix blog is a good way
> to make your work known in the community and to people who are following
> the project.
>
> --
> Ricardo
>
>

[-- Attachment #2: Type: text/html, Size: 4031 bytes --]

Re: Web interface pushed

[Clément Lassieur]

Hello Tatiana,

Tatiana Sholokhova <tanja201396@gmail.com> writes:

> I pushed commits implementing the features we discussed before. Could you
> please take a look at them? I have reconsidered the idea of having a
> separate page with build information, as I do not see what additional
> information we can display on this page except the link to the log file.
> So, for now I have added a link to the log file in the newly added column
> of the table with information about builds of an evaluation. What do you
> think about this?

Thank you!  I'll review these patches today or tomorrow.

Another idea would be to implement a 'search' feature that would allow
to search by job name.  Clicking on the job name would take us to a page
that would display all the builds for that job name.  Also, in the
Builds view, the job names could be clickable too.

WDYT?
Clément

Re: Web interface pushed

[amirouche]

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


On Tuesday, August 07, 2018 23:33 CEST, Tatiana Sholokhova <tanja201396@gmail.com> wrote:
 
I thought about writing the summary of and sending it to the mailing list, but now I think that a blog post would be more convenient for a reader. What is the source format for guix blog markdown and is where something special I need to know about preparing the blog post? Also, I would like to create a draft version of the final document and share it with you in order to discuss it before publishing.$ git clone git://git.savannah.gnu.org/guix/guix-artwork.git guix/artwork
$ cd guix/artwork/website/
$ guix package -i git glibc-locales gnutls guile guile-json guile-syntax-highlight guix haunt

Then

$ GUIX_WEB_SITE_LOCAL=yes haunt build
$ haunt serve

The blog post are sxml or markdown inside posts/ directory inside website/.

Send a patch with your blog post using: git format-patch -p1.

[-- Attachment #2: Type: text/html, Size: 1117 bytes --]

Re: Web interface pushed

[Ricardo Wurmus]

Hi Tatiana,

> I pushed commits implementing the features we discussed before. Could you
> please take a look at them?

I’ll leave the review to Clément who volunteered to take a look at your
commits soon.  (He’s more familiar with Cuirass than I am.)

> I have reconsidered the idea of having a
> separate page with build information, as I do not see what additional
> information we can display on this page except the link to the log file.
> So, for now I have added a link to the log file in the newly added column
> of the table with information about builds of an evaluation. What do you
> think about this?

I think that’s fine for now.

I’d like to show more information in the future, such as build time, git
change that triggered the build, links to packages that depend on this
build (and packages that affect this build in case of a propagated
failure), etc.

> I thought about writing the summary of and sending it to the mailing list,
> but now I think that a blog post would be more convenient for a reader.
> What is the source format for guix blog markdown and is where something
> special I need to know about preparing the blog post?

You don’t need to know anything about the blog to prepare the post.  The
format is markdown.  Amirouche sent instructions on how to build the
website (including the blog), but you don’t need to play with this if
you don’t want to.  It would be sufficient to send the blog post draft
to Clément, Ludovic, and myself via email.  (Please don’t use Google
Docs.)  We would comment inline if any changes are needed.

Once approved I would publish it on your behalf.

Does this sound okay?

--
Ricardo

Re: Web interface pushed

[Gábor Boskovits]

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

Ricardo Wurmus <rekado@elephly.net> ezt írta (időpont: 2018. aug. 9., Cs,
22:46):

>
> Hi Tatiana,
>
> > I pushed commits implementing the features we discussed before. Could you
> > please take a look at them?
>
> I’ll leave the review to Clément who volunteered to take a look at your
> commits soon.  (He’s more familiar with Cuirass than I am.)
>
> > I have reconsidered the idea of having a
> > separate page with build information, as I do not see what additional
> > information we can display on this page except the link to the log file.
> > So, for now I have added a link to the log file in the newly added column
> > of the table with information about builds of an evaluation. What do you
> > think about this?
>
> I think that’s fine for now.
>
> I’d like to show more information in the future, such as build time, git
> change that triggered the build, links to packages that depend on this
> build (and packages that affect this build in case of a propagated
> failure), etc.
>
>
Here we could also find a place for things like previous builds, last
successful build, last failing build later.


> > I thought about writing the summary of and sending it to the mailing
> list,
> > but now I think that a blog post would be more convenient for a reader.
> > What is the source format for guix blog markdown and is where something
> > special I need to know about preparing the blog post?
>
> You don’t need to know anything about the blog to prepare the post.  The
> format is markdown.  Amirouche sent instructions on how to build the
> website (including the blog), but you don’t need to play with this if
> you don’t want to.  It would be sufficient to send the blog post draft
> to Clément, Ludovic, and myself via email.  (Please don’t use Google
> Docs.)  We would comment inline if any changes are needed.
>
> Once approved I would publish it on your behalf.
>
> Does this sound okay?
>
> --
> Ricardo
>
>

[-- Attachment #2: Type: text/html, Size: 2512 bytes --]

Re: Web interface pushed

[Clément Lassieur]

Hello!

Gábor Boskovits <boskovits@gmail.com> writes:

>> > I have reconsidered the idea of having a
>> > separate page with build information, as I do not see what additional
>> > information we can display on this page except the link to the log file.
>> > So, for now I have added a link to the log file in the newly added column
>> > of the table with information about builds of an evaluation. What do you
>> > think about this?
>>
>> I think that’s fine for now.
>>
>> I’d like to show more information in the future, such as build time, git
>> change that triggered the build, links to packages that depend on this
>> build (and packages that affect this build in case of a propagated
>> failure), etc.

Note that there is no notion of 'package' in Cuirass.  Most of the
things you said would work with 'derivations' though.  But to add a link
to the package, we would need a way to find out if a derivation refers
to a package.  I don't know how to do this.

> Here we could also find a place for things like previous builds, last
> successful build, last failing build later.

The 'job name' page I was talking about[1] is what you want, isn't it?

Clément

[1]: https://lists.gnu.org/archive/html/guix-devel/2018-08/msg00039.html

Re: Web interface pushed

[Ricardo Wurmus]

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

>>> I’d like to show more information in the future, such as build time, git
>>> change that triggered the build, links to packages that depend on this
>>> build (and packages that affect this build in case of a propagated
>>> failure), etc.
>
> Note that there is no notion of 'package' in Cuirass.  Most of the
> things you said would work with 'derivations' though.  But to add a link
> to the package, we would need a way to find out if a derivation refers
> to a package.  I don't know how to do this.

Yeah, I used “package” as a shorthand for “derivation for a package”.
Links to related derivations would be enough.

We may want to consider taking a closer look at the format for
derivations and think about whether it would make sense to extend it, so
that it is easier to understand the derivation’s context.

>> Here we could also find a place for things like previous builds, last
>> successful build, last failing build later.
>
> The 'job name' page I was talking about[1] is what you want, isn't it?
>
> Clément
>
> [1]:
> https://lists.gnu.org/archive/html/guix-devel/2018-08/msg00039.html

Yes, that’s what I’d like.


--
Ricardo