ludo@gnu.org (Ludovic Courtès) writes:

> So, what speedup to you get compared to ‘make -jN’?

It should have the same improvement as 'guix pull' since it does
basically the same thing.  I measured to make things concrete, and on my
machine the run time of 'make -j4' decreases from ~26m to ~6m.

>> * Makefile.am: Call build-aux/compile-all.scm to compile many .scm files
>>   in a single process.
>
> Rather:
>
>   * Makefile.am (%.go, make-go): New rules.
>
> Also, the new script must be added to ‘EXTRA_DIST’.

Done and done.  Also added a part to the commit message for EXTRA_DIST.

>> +++ b/build-aux/compile-all.scm
>
> Please add a copyright/license header.

Whoops, done.

>> +(define (file->module file)
>> +  (map string->symbol
>> +       (string-split (string-drop-right file 4) #\/)))
>
> Does this work with out-of-tree builds?

Good catch.  There were a few reasons it failed, all fixed now.

(There were also existing problems with out-of-tree builds; see other
patch I sent.)

>> +(let* ((args (cdr (command-line)))
>> +       (target (car args))
>> +       (files (cdr args)))
>
> Please use ‘match’ for the win!  :-)

Done.  I really need to burn that into my coding habits.

>> +  (for-each
>> +   (lambda (file)
>> +     (let ((go (scm->go file)))
>> +       (unless (and (file-exists? go)
>> +                    (file-mtime<? file go))
>> +         (let ((module (file->module file)))
>> +           (format #t "  LOAD ~s~%" module)
>> +           (resolve-interface module)))))
>> +   files)
>
> Make the ‘lambda’ a named top-level procedure, for clarity.  Also add a
> reference to the evil bug that makes this hack necessary in the first
> place.
>
> Maybe it would be clearer to have:
>
>   (define (file-needs-compilation? file)
>     (file-mtime<? (scm->go file) file))
>
> and then to:
>
>   (let ((files (filter file-needs-compilation? files)))
>     ;; …
>     )

Indeed, done.

>> +  (with-target target
>> +    (lambda ()
>> +      (let ((mutex (make-mutex)))
>> +        (par-for-each
>> +         (lambda (file)
>> +           (let ((go (scm->go file)))
>> +             (unless (and (file-exists? go)
>> +                          (file-mtime<? file go))
>> +               (with-mutex mutex
>> +                 (format #t "  GUILEC ~s~%" file)
>> +                 (force-output))
>> +               (compile-file file #:output-file go #:opts compile-options)
>> +               (with-mutex mutex
>> +                 (format #t "  WROTE ~s~%" go)
>> +                 (force-output)))))
>> +         files)))))
>
> Ditto: name the lambda.

Here the lambda cannot entirely be factored out since the 'mutex' from
the lexical scope is needed.  I still factored out the actual code and
pass the mutex as an argument to the procedure in a minimal lambda, as
seen in the new patch below; I hope it's readable this way.

> I would use ~a instead of ~s, to match the current output, and remove
> the “WROTE” output.

Done.

> Could you send an updated patch?

Attached at the bottom.

> It would be awesome if you could check that ‘make distcheck’ still
> passes, and also make sure things behave correctly when modifying just
> one file and running ‘make’, things like that.

I'm having headaches with distcheck.  Currently it bails out because I'm
missing tex.  Debian's version is apparently too old, and Guix's version
is huge and has been downloading for many hours now.  I'll report back
again when I'm done with that.

I've ran 'make' on my branch containing this patch a couple times in the
past after rebasing on master, so that a subset of the .scm files would
be recompiled, and didn't have issues.  I can't imagine a way it would
break either, for any given subset of all the .scm files.  I hope I'm
not overseeing anything.

> Thank you!

Thanks for the review! :-)

Other notable changes in this version of the patch:

- I noticed guix/config.scm and guix/tests.scm are not in MODULES in
  Makefile.am (intentionally?), so I added them to the make-go rule.

- I removed the MKDIR_P loop in the make-go rule, and do the equivalent
  in the Scheme code now.

- The target host and top source directory are now passed to the script
  via lowercase environment variables, which makes the code a little
  simpler.  I hope this is stylistically fine.

> Ludo’.

Taylan