From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Cl=C3=A9ment?= Lassieur Subject: bug#32190: [PATCH] database: Merge Derivations into Builds table. Date: Thu, 16 Aug 2018 23:00:17 +0200 Message-ID: <87y3d63u3i.fsf@lassieur.org> References: <87k1ptirr0.fsf@lassieur.org> <20180804160303.20451-1-clement@lassieur.org> <87h8jwvkg8.fsf@elephly.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:59742) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fqPOF-0005DU-OR for bug-guix@gnu.org; Thu, 16 Aug 2018 17:01:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fqPOC-0004Co-Hb for bug-guix@gnu.org; Thu, 16 Aug 2018 17:01:07 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:47849) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fqPOC-0004Cd-Cx for bug-guix@gnu.org; Thu, 16 Aug 2018 17:01:04 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1fqPOC-0001WN-9d for bug-guix@gnu.org; Thu, 16 Aug 2018 17:01:04 -0400 Sender: "Debbugs-submit" Resent-To: bug-guix@gnu.org Resent-Message-ID: In-reply-to: <87h8jwvkg8.fsf@elephly.net> List-Id: Bug reports for GNU Guix List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-guix-bounces+gcggb-bug-guix=m.gmane.org@gnu.org Sender: "bug-Guix" To: Ricardo Wurmus Cc: 32190-done@debbugs.gnu.org Pushed, thank you for the review! Cl=C3=A9ment Ricardo Wurmus writes: > Hi Cl=C3=A9ment, > >> Fixes . > > Woo! Thank you for this patch. > >> * src/cuirass/base.scm (evaluate): Don't add jobs to the Derivations tab= le. > > I see that you=E2=80=99ve mentioned your changes to =E2=80=9Cbuild-packag= es=E2=80=9D in a later > email. > > I have two general questions about this: why was the change from =E2=80= =9Cid=E2=80=9D to > =E2=80=9Crowid=E2=80=9D necessary? And: could you please also update the= documentation > so that is reflects the changes? > >> diff --git a/src/cuirass/database.scm b/src/cuirass/database.scm >> index b4b1652..7788ac9 100644 >> --- a/src/cuirass/database.scm >> +++ b/src/cuirass/database.scm > [=E2=80=A6] >> (define (db-add-evaluation db eval) >> (sqlite-exec db "\ >> INSERT INTO Evaluations (specification, commits) VALUES (" >> @@ -384,27 +356,39 @@ string." >> (define (db-add-build db build) >> "Store BUILD in database DB. BUILD eventual outputs are stored >> in the OUTPUTS table." >> - (let* ((build-exec >> - (sqlite-exec db "\ >> -INSERT INTO Builds (derivation, evaluation, log, status, timestamp, sta= rttime, stoptime)\ >> - VALUES (" >> - (assq-ref build #:derivation) ", " >> - (assq-ref build #:eval-id) ", " >> - (assq-ref build #:log) ", " >> - (or (assq-ref build #:status) >> - (build-status scheduled)) ", " >> - (or (assq-ref build #:timestamp) 0) ", " >> - (or (assq-ref build #:starttime) 0) ", " >> - (or (assq-ref build #:stoptime) 0) ");")) >> - (build-id (last-insert-rowid db))) >> - (for-each (lambda (output) >> - (match output >> - ((name . path) >> - (sqlite-exec db "\ >> -INSERT INTO Outputs (build, name, path) VALUES (" >> - build-id ", " name ", " path ");")))) >> - (assq-ref build #:outputs)) >> - build-id)) >> + (catch 'sqlite-error >> + (lambda () >> + (sqlite-exec db " >> +INSERT INTO Builds (derivation, evaluation, job_name, system, nix_name,= log, >> +status, timestamp, starttime, stoptime) >> +VALUES (" >> + (assq-ref build #:derivation) ", " >> + (assq-ref build #:eval-id) ", " >> + (assq-ref build #:job-name) ", " >> + (assq-ref build #:system) ", " >> + (assq-ref build #:nix-name) ", " >> + (assq-ref build #:log) ", " >> + (or (assq-ref build #:status) >> + (build-status scheduled)) ", " >> + (or (assq-ref build #:timestamp) 0) ", " >> + (or (assq-ref build #:starttime) 0) ", " >> + (or (assq-ref build #:stoptime) 0) ");") >> + (let ((derivation (assq-ref build #:derivation))) >> + (for-each (lambda (output) >> + (match output >> + ((name . path) >> + (sqlite-exec db "\ >> +INSERT INTO Outputs (derivation, name, path) VALUES (" >> + derivation ", " name ", " path ");"= )))) >> + (assq-ref build #:outputs)) >> + derivation)) > > This procedure is called when a build is scheduled, isn=E2=80=99t it? The > docstring says =E2=80=9CBUILD eventual outputs are stored in the OUTPUTS = table.=E2=80=9D > =E2=80=93 does this mean the names of the *expected* output directories a= re > recorded in Outputs? They don=E2=80=99t exist at this point, right? > >> + (lambda (key who code message . rest) >> + ;; If we get a unique-constraint-failed error, that means we have >> + ;; already inserted the same build. That happens when several jo= bs >> + ;; produce the same derivation, and we can ignore it. >> + (if (=3D code SQLITE_CONSTRAINT_PRIMARYKEY) >> + #f >> + (apply throw key who code rest))))) > > Okay. > > Can we prevent this from happening in the first place? I feel a little > uncomfortable about performing an operation that we expect to cause > primary key errors regularly. > >> diff --git a/src/schema.sql b/src/schema.sql >> index eb0f7e9..0452495 100644 >> --- a/src/schema.sql >> +++ b/src/schema.sql > [=E2=80=A6] >> --- Builds are not in a one to one relationship with derivations in orde= r to >> --- keep track of non deterministic compilations. > > Is this comment still correct considering that the derivation is now the > primary key of the Builds table? > >> CREATE TABLE Builds ( >> - id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, >> - derivation TEXT NOT NULL, >> + derivation TEXT NOT NULL PRIMARY KEY, >> evaluation INTEGER NOT NULL, >> + job_name TEXT NOT NULL, >> + system TEXT NOT NULL, >> + nix_name TEXT NOT NULL, >> log TEXT NOT NULL, >> status INTEGER NOT NULL, >> timestamp INTEGER NOT NULL, >> starttime INTEGER NOT NULL, >> stoptime INTEGER NOT NULL, >> - FOREIGN KEY (derivation) REFERENCES Derivations (derivation), >> FOREIGN KEY (evaluation) REFERENCES Evaluations (id) >> ); > > Thanks again!