Re: [PATCH] build: pull: Compile .scm files in one process.

[Taylan Ulrich Bayırlı/Kammer]

address@hidden (Ludovic Courtès) writes:

> address@hidden (Taylan Ulrich "Bayırlı/Kammer") skribis:
>
>> address@hidden (Ludovic Courtès) writes:
>
> [...]
>
>>>    ?: 2 [primitive-load 
>>> "/gnu/store/d51z2xkwp1vh0dh6gqadyyzv21m0b772-guix-latest/guix/scripts/import/hackage.scm"]
>>> In ice-9/eval.scm:
>>>  453: 1 Exception thrown while printing backtrace:
>>> ERROR: In procedure package-location: Wrong type argument: Error while 
>>> printing exception.
>>>
>>> ice-9/eval.scm:387:11: In procedure eval:
>>> ice-9/eval.scm:387:11: In procedure package-version: Wrong type argument: 
>>> Error while printing exception.
>>> builder for `/gnu/store/pc1i5s6vx9yx97prhskx178gj5swxw4k-guix-latest.drv' 
>>> failed with exit code 1
>>> guix pull: error: build failed: build of 
>>> `/gnu/store/pc1i5s6vx9yx97prhskx178gj5swxw4k-guix-latest.drv' failed
>>>
>>> Any idea?
>>>
>>> To me it sounds like there are two <package> record type descriptors in
>>> the wild, which is why ‘package-location’ in the package record printer
>>> bails out.
>>
>> That's one of the errors that result from a "bad" order of compiling the
>> files and when the 'load' hack isn't used to work around it, which isn't
>> the case in that patch...  Indeed I can't seem to reproduce the issue.
>>
>> The attached patch below also builds on the quoted patch, removes the
>> thread-safe-port procedure, and just sets the warning port to a void
>> port.  Applied on top of the current master, it works for me.
>
> On top of current master, it fails for me in the same way as above.
>
> To be clear, I applied the patch, ran ‘make dist’, and then:
>
>   time ./pre-inst-env guix pull --url=file://$PWD/guix-0.9.0.tar.gz
>
> Are you doing the same?  The “loading” part is done sequentially, so it
> should be deterministic.

Whoops, I had not rerun the whole 'make dist' after rebasing on master,
only copied the new guix/build/pull.scm into an existing tarball (I had
gotten used to doing that because it saves time while working on a
single file), so changes in other files were missing.

After some tinkering around I realized that the problem is that our
workaround of loading files explicitly causes the package record type to
be redefined after some packages have already been defined.  More
generally, we force the top-level of many files to be re-executed after
they've already been executed as a result of a module import...

It would be great if the whole circular import problem could somehow be
solved by Guile (no idea how feasible it is).  On the meanwhile, we'll
have to work around problems introduced by workarounds. :-) Moving the
loading of guix/package.scm to the very front seems to solve the issue.
Other record types could still cause the same issue, but their relative
rarity of use hopefully makes this a non-issue.  I also moved the
loading of guix/ files before gnu/ files again, which might also help
with that.  (For package.scm it wasn't sufficient, probably because some
modules under guix/ import some gnu package modules, before package.scm
is loaded explicitly.)

One can imagine a wholly more robust version of the workaround, which
avoids the re-execution of top-levels.  A variant of load[-primitive]
that doesn't load a file again if it was already loaded would do.
That's basically what importing a module does, so scanning for module
definitions in files and importing them might work, but seems somewhat
hacky...  For now, here's the patch that just loads package.scm first.

>From dcb563f611c4fbd6e3e22106c60626f32c04f9e5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Taylan=20Ulrich=20Bay=C4=B1rl=C4=B1/Kammer?=
 <address@hidden>
Date: Fri, 27 Nov 2015 09:27:55 +0100
Subject: [PATCH] build: pull: Compile .scm files in one process.

* guix/build/pull.scm (call-with-process, report-build-progress)
(p-for-each): Remove.
(build-guix): Load and compile files in one process.
---
 guix/build/pull.scm | 149 +++++++++++++++++++---------------------------------
 1 file changed, 55 insertions(+), 94 deletions(-)

diff --git a/guix/build/pull.scm b/guix/build/pull.scm
index 281be23..3025442 100644
--- a/guix/build/pull.scm
+++ b/guix/build/pull.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013, 2014 Ludovic Courtès <address@hidden>
+;;; Copyright © 2015 Taylan Ulrich Bayırlı/Kammer <address@hidden>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -22,6 +23,7 @@
   #:use-module (ice-9 ftw)
   #:use-module (ice-9 match)
   #:use-module (ice-9 format)
+  #:use-module (ice-9 threads)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-26)
@@ -33,75 +35,10 @@
 ;;;
 ;;; Code:
 
-(define (call-with-process thunk)
-  "Run THUNK in a separate process that will return 0 if THUNK terminates
-normally, and 1 if an exception is raised."
-  (match (primitive-fork)
-    (0
-     (catch #t
-       (lambda ()
-         (thunk)
-         (primitive-exit 0))
-       (lambda (key . args)
-         (print-exception (current-error-port) #f key args)
-         (primitive-exit 1))))
-    (pid
-     #t)))
-
-(define* (report-build-progress total completed cont
-                                #:optional (log-port (current-error-port)))
-  "Report that COMPLETED out of TOTAL files have been completed, and call
-CONT."
-  (display #\cr log-port)
-  (format log-port "compiling...\t~5,1f% of ~d files" ;FIXME: i18n
-          (* 100. (/ completed total)) total)
-  (force-output log-port)
-  (cont))
-
-(define* (p-for-each proc lst
-                     #:optional (max-processes (current-processor-count))
-                     #:key (progress report-build-progress))
-  "Invoke PROC for each element of LST in a separate process, using up to
-MAX-PROCESSES processes in parallel.  Call PROGRESS at each step, passing it
-the continuation.  Raise an error if one of the processes exit with non-zero."
-  (define total
-    (length lst))
-
-  (define (wait-for-one-process)
-    (match (waitpid WAIT_ANY)
-      ((_ . status)
-       (unless (zero? (status:exit-val status))
-         (error "process failed" proc status)))))
-
-  (let loop ((lst       lst)
-             (running   0)
-             (completed 0))
-    (match lst
-      (()
-       (or (zero? running)
-           (let ((running   (- running 1))
-                 (completed (+ completed 1)))
-             (wait-for-one-process)
-             (progress total completed
-                       (lambda ()
-                         (loop lst running completed))))))
-      ((head . tail)
-       (if (< running max-processes)
-           (let ((running (+ 1 running)))
-             (call-with-process (cut proc head))
-             (progress total completed
-                       (lambda ()
-                         (loop tail running completed))))
-           (let ((running   (- running 1))
-                 (completed (+ completed 1)))
-             (wait-for-one-process)
-             (progress total completed
-                       (lambda ()
-                         (loop lst running completed)))))))))
-
 (define* (build-guix out source
                      #:key gcrypt
-                     (debug-port (%make-void-port "w")))
+                     (debug-port (%make-void-port "w"))
+                     (log-port (current-error-port)))
   "Build and install Guix in directory OUT using SOURCE, a directory
 containing the source code.  Write any debugging output to DEBUG-PORT."
   (setvbuf (current-output-port) _IOLBF)
@@ -130,33 +67,57 @@ containing the source code.  Write any debugging output to 
DEBUG-PORT."
     (set! %load-path (cons out %load-path))
     (set! %load-compiled-path (cons out %load-compiled-path))
 
-    ;; Compile the .scm files.  Do that in independent processes, à la
-    ;; 'make -j', to work around <http://bugs.gnu.org/15602> (FIXME).
-    ;; This ensures correctness, but is overly conservative and slow.
-    ;; The solution initially implemented (and described in the bug
-    ;; above) was slightly faster but consumed memory proportional to the
-    ;; number of modules, which quickly became unacceptable.
-    (p-for-each (lambda (file)
-                  (let ((go (string-append (string-drop-right file 4)
-                                           ".go")))
-                    (format debug-port "~%compiling '~a'...~%" file)
-                    (parameterize ((current-warning-port debug-port))
-                      (compile-file file
-                                    #:output-file go
-                                    #:opts
-                                    %auto-compilation-options))))
-
-                (filter (cut string-suffix? ".scm" <>)
-
-                        ;; Build guix/*.scm before gnu/*.scm to speed
-                        ;; things up.
-                        (sort (find-files out "\\.scm")
-                              (let ((guix (string-append out "/guix"))
-                                    (gnu  (string-append out "/gnu")))
-                                (lambda (a b)
-                                  (or (and (string-prefix? guix a)
-                                           (string-prefix? gnu b))
-                                      (string<? a b))))))))
+    ;; Compile the .scm files.  Load all the files before compiling them to
+    ;; work around <http://bugs.gnu.org/15602> (FIXME).
+    (let* ((files
+            ;; The above mentioned workaround means the top-level of many
+            ;; files will be executed twice.  Load guix/packages.scm first,
+            ;; because it's crucial that the package data type doesn't get
+            ;; redefined halfway through.  Also load guix/ modules before gnu/
+            ;; modules to get steadier progress reporting.
+            (sort (filter (cut string-suffix? ".scm" <>)
+                          (find-files out "\\.scm"))
+                  (let ((guix (string-append out "/guix"))
+                        (gnu  (string-append out "/gnu")))
+                    (lambda (a b)
+                      (or (string-suffix? "/guix/packages.scm" a)
+                          (and (string-prefix? guix a)
+                               (string-prefix? gnu  b))
+                          (string<? a b))))))
+           (total (length files)))
+      (let loop ((files files)
+                 (completed 0))
+        (match files
+          (() *unspecified*)
+          ((file . files)
+           (display #\cr log-port)
+           (format log-port "loading...\t~5,1f% of ~d files" ;FIXME: i18n
+                   (* 100. (/ completed total)) total)
+           (force-output log-port)
+           (format debug-port "~%loading '~a'...~%" file)
+           (parameterize ((current-warning-port debug-port))
+             (save-module-excursion
+              (lambda () (primitive-load file))))
+           (loop files (+ 1 completed)))))
+      (newline)
+      (let ((mutex (make-mutex))
+            (completed 0))
+        (par-for-each
+         (lambda (file)
+           (with-mutex mutex
+             (display #\cr log-port)
+             (format log-port "compiling...\t~5,1f% of ~d files" ;FIXME: i18n
+                     (* 100. (/ completed total)) total)
+             (force-output log-port)
+             (format debug-port "~%compiling '~a'...~%" file))
+           (let ((go (string-append (string-drop-right file 4) ".go")))
+             (parameterize ((current-warning-port (%make-void-port "w")))
+               (compile-file file
+                             #:output-file go
+                             #:opts %auto-compilation-options)))
+           (with-mutex mutex
+             (set! completed (+ 1 completed))))
+         files))))
 
   ;; Remove the "fake" (guix config).
   (delete-file (string-append out "/guix/config.scm"))
-- 
2.6.3