Re: [PATCH v2 01/13] build-system: Add asdf-build-system.

[ludo@gnu.org (Ludovic Courtès)]

Hello,

Adding my 2¢ as 宋文武 suggested.  :-)

Andy Patterson <ajpatter@uwaterloo.ca> skribis:

> From 31dea60d8f4d876c24352b3279c8bef7d5a1ffc4 Mon Sep 17 00:00:00 2001
> From: Andy Patterson <ajpatter@uwaterloo.ca>
> Date: Mon, 26 Sep 2016 20:11:54 -0400
> Subject: [PATCH v3 01/12] build-system: Add asdf-build-system.
>
> * guix/build-system/asdf.scm: New file.
> * guix/build/asdf-build-system.scm: New file.
> * guix/build/lisp-utils.scm: New file.
> * Makefile.am (MODULES): Add them.
> * doc/guix.texi (Build Systems): Document 'asdf-build-system'.

Woohoo, nice stuff!

> +@defvr {Scheme Variable} asdf-build-system/source
> +@defvrx {Scheme Variable} asdf-build-system/sbcl
> +@defvrx {Scheme Variable} asdf-build-system/ecl

And great doc.

> +(define* (package-with-build-system from-build-system to-build-system
> +                                    from-prefix to-prefix
> +                                    #:key variant-property
> +                                    phases-transformer)
> +  "Return a precedure which takes a package PKG which uses FROM-BUILD-SYSTEM,
> +and returns one using TO-BUILD-SYSTEM. If PKG was prefixed by FROM-PREFIX, the
> +resulting package will be prefixed by TO-PREFIX. Inputs of PKG are recursively
> +transformed using the same rule. The result's #:phases argument will be
> +modified by PHASES-TRANSFORMER, an S-expression which evaluates on the build
> +side to a procedure of one argument.

This code seems to be adapted from ‘package-with-python2’.  It seems
that ‘package-input-rewriting’ is too specific to be used here, but at
any rate, we should keep an eye towards factorizing this and keep it as
simple as possible to facilitate that.

Is #:variant-property necessary here?  It was necessary in
‘package-with-python2’ due to python-2 and python-3 packages sometimes
having a different set of dependencies.  If it can be avoided here, it’s
better.  Otherwise that’s fine.

(I haven’t followed closely, so apologies if this has already been
discussed.)

Note: Two spaces after end-of-sentence period please.  :-)

> +  (define target-is-source? (eq? 'asdf/source
> +                                 (build-system-name to-build-system)))

Rather:

  (eq? ast-build-system/source to-build-system)

The name is purely for debugging purposes.

> +(define asdf-build-system/sbcl
> +  (build-system
> +    (name 'asdf/sbcl)
> +    (description "The build system for asdf binary packages using sbcl")
> +    (lower (lower "sbcl"))))
> +
> +(define asdf-build-system/ecl
> +  (build-system
> +    (name 'asdf/ecl)
> +    (description "The build system for asdf binary packages using ecl")
> +    (lower (lower "ecl"))))
> +
> +(define asdf-build-system/source
> +  (build-system
> +    (name 'asdf/source)
> +    (description "The build system for asdf source packages")
> +    (lower lower/source)))

Probably uppercase: SBCL, ECL, ASDF.

> +(define* (strip #:key lisp #:allow-other-keys #:rest args)
> +  ;; stripping sbcl binaries removes their entry program and extra systems
> +  (unless (string=? lisp "sbcl")
> +    (apply (assoc-ref gnu:%standard-phases 'strip) args))
> +  #t)

Shouldn’t it be:

  (or (string=? lisp "sbcl")
      (apply …))

?  Otherwise the real return value is discarded.

> +(define %lisp
> +  (make-parameter "lisp"))

Add a comment like “File name of the Lisp compiler.” (?).

> +(define %install-prefix "/share/common-lisp")

What about “lib/common-lisp” for architecture-dependent files
(binaries)?  What do other distros do?

That’s it.

Thank you!

Ludo’.