Pushed, thank you for the review! Clément
Ricardo Wurmus <[email protected]> writes: > Hi Clément, > >> Fixes <https://bugs.gnu.org/32190>. > > Woo! Thank you for this patch. > >> * src/cuirass/base.scm (evaluate): Don't add jobs to the Derivations table. > > I see that you’ve mentioned your changes to “build-packages” in a later > email. > > I have two general questions about this: why was the change from “id” to > “rowid” 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 > […] >> (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, >> starttime, 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’t it? The > docstring says “BUILD eventual outputs are stored in the OUTPUTS table.” > – does this mean the names of the *expected* output directories are > recorded in Outputs? They don’t 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 jobs >> + ;; produce the same derivation, and we can ignore it. >> + (if (= 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 > […] >> --- Builds are not in a one to one relationship with derivations in order 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!
