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

[Andy Patterson]

On Wed, 05 Oct 2016 12:55:51 +0800
address@hidden (宋文武) wrote:

> Hi!  I have spent more time wondering on this patch and ASDF, so here
> are some questions, opinions and ideas (roughly).
> 

Hi. Thanks again for your comments.

> > * Makefile.am: Add them.  
> 
> Should be: Makefile.am (MODULES): Add them.
> 

Ok.

> > * doc/guix.texi: Add section on 'asdf-build-system/source'.  
> 
> Well, it dosen't create a new info section, I think this can be:
> 
> * doc/guix.texi (Build Systems): Document 'asdf-build-system'.
>

Ok.

> > +These variables, exported by @code{(guix build-system sbcl)},
> > implement  
> 
> Typo, sbcl -> asdf.
> 

Right.

> > +build procedures for Common Lisp packages using the
> > address@hidden://common-lisp.net/project/asdf/, ``ASDF''} system.  
> 
> How about expand it a bit to: @url{..., ``ASDF''}, a system definition
> facility for Common Lisp programs and libraries.
>

Sure.

> > +The build system uses conventions to determine the roles of inputs
> > in +the build system.  
> 
> … uses naming conventions …  What’s the “roles of inputs” for?
> 

I'll explain what is meant a bit further.

> > + For example,
> > +
> > address@hidden
> > +(define-public sbcl-bordeaux-threads
> > +  (package
> > +    ...
> > +    (native-inputs `(("tests:cl-fiveam" ,sbcl-fiveam)))
> > +    ...))
> > address@hidden example  
> 
> This is a bit confusing, so every input starts with ‘sbcl-’ will be
> propagated?  I wonder why not just use ‘propgated-inputs’ for that.
>

Packages aren't propagated in the binary systems. The naming
convention is what tells 'patch-asd-file' which systems to wrap the
library with (so that it can find its dependencies). Changing the
prefix will cause that phase to ignore that library, so doing so has
the same effect as '#:test-only-systems' used to have. I'll explain
that in the doc.

> > +
> > +Additionally, the corresponding source package should be labelled
> > using +the same convention as python packages (see @ref{Python
> > Modules}), using +the @code{cl-} prefix.
> > +
> > +One package should be defined for each ASDF system.  
> 
> This is for binary packages right?  (It’s obviously not convenient for
> source packages which usually have an extra system for test only.)
> ++ I seems wrong here, new ideas below ‘package-with-build-system’. ++
>

This is correct, actually. This is essentially what I did for slynk,
which contains quite a few systems.
 
> For binary packages, this will be perfect if they’re 1-to-1 mapped to
> a CL system, but then their names are inconsistent with the ’cl-’
> ones, whose names are from projects instead of the systems they
> contain.
> 
> Consider the ‘cl-autowrap’ (https://github.com/rpav/cl-autowrap)
> project.  It has 3 systems: ‘cl-autowrap’, ‘cl-autowrap-test’ and
> ‘cl-plus-c’.  IIUC, follow this one package per system way, we will
> package it as:
> 
> - cl-autowrap, contains the 3 systems in source form.
> - sbcl-autowrap (or maybe sbcl-cl-autowrap?).
> - sbcl-plus-c (or ‘sbcl-cl-plus-c?).
> - sbcl-autowrap-test (for testing).
> 

That's right (and it would be sbcl-cl-autowrap).

> It’s hard to know that ‘cl-autowrap’ has ‘cl-plus-c’ in it…
> 

In that case, we could add the extra systems to the description of the
source package. Does that sound reasonable?

> > +
> > +The package outputs control whether or not executable programs and
> > +images are built alongside the package's usual output, using the
> > address@hidden and @code{image} outputs, respectively.
> > +
> > +Packages can also be built which combine other packages into an
> > +executable program or image only, without building another system.
> > +Specifying one of the @code{#:binary?} or @code{#:image?}
> > parameters +will produce this behaviour.
> > +
> > +When building an executable program, the @code{#:entry-program}
> > +parameter, which should be a list of Common Lisp expressions, must
> > be +used to specify what program should be run.  In this program,
> > address@hidden will be bound to the command-line arguments
> > passed. +
> > +The @code{#:image-dependencies} parameter can be used to add
> > packages to +the pre-loaded systems included in the executable
> > program or image. address@hidden:compile-dependencies} specifies a list
> > of additional systems +which should be loaded before a system is
> > compiled.  If the package +depends on special systems exported by
> > the implementation itself, the address@hidden:special-dependencies}
> > parameter should be used to specify them.  
> 
> I’d like to make the build action of ‘program’ or ‘image’ more
> explicit instead of coding them in the build system.  eg:
> 
> --8<---------------cut here---------------start------------->8---
> (define-public sbcl-stumpwm
>   (package
>     …
>     (arguments
>      '(#:phases
>        (modify-phases %standard-phases
>          (add-after 'install 'install-program
>            (lambda* (#:key outputs #:allow-other-keys)
>              ((let* ((bin  (assoc-ref outputs "bin"))
>                      (prog (string-append bin "/bin/stumpwm")))
>                 (asdf:build-program prog
>                   #:entry-program '((stumpwm:stumpwm))
>                   #:special-dependencies '("sb-posix")))))))
>         …))))
> --8<---------------cut here---------------end--------------->8---
> 
> I think this way show what #:entry-program and other parameters are
> used for more clearly, as they have nothing to do with other phases
> (compile the system and install a bundle asd file).  Most CL packages
> are libraries, so I feel that having phases (generate-binary and
> generate-image) which do nothing most times is overkill :-)
> 

That sounds good.

> What’s the ‘compile-dependencies’ used for?  I think when compile a
> system, ASDF will load all its depends (from the :depends-on) first.
> Sometimes we need load extra systems manually?
> 

It's for packages like slynk, where many systems are defined within the
same file. When telling asdf to find a system, it will only search for
a file with the same name unless it already knows about that system.
Therefore, in these cases the system which contains the definitions is
loaded first. This mechanism is a bit clunky so suggestions are
welcome. For now I'll document the reason it's done that way.

> > […]
> > diff --git a/guix/build-system/asdf.scm b/guix/build-system/asdf.scm
> > new file mode 100644
> > index 0000000..eb8b7d9
> > --- /dev/null
> > +++ b/guix/build-system/asdf.scm
> > @@ -0,0 +1,385 @@
> > […]
> > +
> > +(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.  
> 
> Oops, so we should use one package per system for source packages too,
> otherwise there is no obvious way to transform between them (I’d like
> it be one way, just ‘cl-package->sbcl-package’) and binary packages.

There doesn't need to be a translation for each package; in the case of
slynk one translation is done for sbcl->cl-source, and then the other
systems provided have binary packages created just using the usual
inherit mechanism, and a translation is made for sbcl->ecl.

I decided to use the sbcl package a the source for all translations,
because it is likely that a binary package will contain more
information (in the form of customizations to the arguments field),
than for source packages, which simplifies the translation process.

> If that’s true, for the ‘cl-autowrap’ project, its source packages
> should be ‘cl-autowrap’, ’cl-plus-c’, and ‘cl-autowrap-test’.  And I
> don’t think we should copy the whole project source for each of
> them.  So, how about:
> 

I agree, that seems unnecessary.

> - For a CL project, define a (non-public) project package, which just
>   unpacks the source to its ‘$out’.
> 
> - For each system the project contains, define a
> ‘cl-SYSTEM-NAME-W/O-CL’ package, which takes the project package as
> ‘source’ and symlink the system’s asd file to its
> ‘$out/share/common-lisp/systems’.
>

The source package already copies all asd files to that directory, so
I think it makes sense to have just one source package (I'll fix the
documentation for that). So for the case of cl-autowrap, that
directory would contain cl-{autowrap-test,autowrap,plus-c}.asd. 

In the guix source, you'd have a sbcl-cl-autowrap package, an
automatically translated cl-autowrap package containing all of the
above, and a manually inherited sbcl-cl-plus-c package. There wouldn't
be a -test package. You'd then also have automatically translated ecl-
packages.

> - Then for each ‘cl-’ package, we can transform it to a binary
> package, which should compile and install a bundle asd file for one
> system.
> 

As described earlier, I think it's easier to go binary->source and
binary->binary.

> > +The result's #:phases argument will be
> > +modified by PHASES-TRANSFORMER, a list which evaluates on the
> > build side to a +procedure of one argument.  
> 
> A symbol?
> 

Right, it could be any quoted sexp. I'll fix that.

> To make a binary package from a source package, my plan looks like:
> 
> - remove all inputs not start with ‘cl-’.  Ideally, we can do all the
>   things (eg: patch the file names of ffi libraries to absolute paths)
>   in source packages to make them ready to use for CLs.  All a binary
>   package needs are CL systems and an CL compiler.
>

Correct; that's currently exactly what CL systems depend on. I'm hoping
that patching ffi files could be done in a mostly automated way once
that's required, so that copying build phases would be unnecessary.
 
> - rewrite ‘cl-’ inputs to ‘sbcl-’ inputs recursively.
> 
> - source package becomes the ‘source’ of the binary package, if
> in-tree build is needed (I hope not very often), we can follow the
> asd file link to copy its real source.
> 

I think it's more flexible to just always build in tree (the system
worked as you describe it here in the first version of the series).
It's not too outlandish to think that systems might want to create
files during the test phase, which is exactly what one package was
doing.

> > +
> > +       (define base-arguments
> > +         (if target-is-source?
> > +             (strip-keyword-arguments
> > +              '(#:tests? #:special-dependencies #:entry-program
> > +                #:image-dependencies #:compile-dependencies
> > #:image?
> > +                #:binary? #:test-only-systems #:lisp)
> > +              (package-arguments pkg))
> > +             (package-arguments pkg)))  
> 
> If we don’t allow ‘sbcl-package->cl-package’, arguments don’t need to
> be stripped.
> 

That's true, but it's easier to strip arguments than to add them.

> ++ quetions below ‘patch-asd-files’. ++
> 
> > +                     #:entry-program ,entry-program
> > +                     #:image-dependencies ,image-dependencies
> > +                     #:image? ,image?
> > +                     #:binary? ,binary?  
> 
> 
> Those will be removed if we build ‘program’ or ’image’ explicitly.  In
> that case, IIUC, we should add variant properties to the source
> package if we add phases (by inherit) to the binary package and want
> the transform get the same (not get the same one is fine too, which
> only lacks the optional programs or binaries) package.
> 
> 

That's correct.

> > +(define source-package->sbcl-package
> > +  (let* ((property 'sbcl-variant)
> > +         (transformer
> > +          (package-with-build-system asdf-build-system/source
> > +                                     asdf-build-system/sbcl
> > +                                     "cl-"
> > +                                     "sbcl-"
> > +                                     #:variant-property property
> > +                                     #:phases-transformer
> > +
> > 'source-phases->sbcl-phases)))  
> 
> The ‘source-phases->sbcl-phases’ proceduce seems missing, is it
> ‘(const %standard-phases)’?
> 

I meant to delete that procedure.

> Doesn’t ‘special-dependencies’ always available, do we need load them
> explicitly?
> 

The problem is that the asd file produced by 'deliver-asd-op' doesn't
add anything to :depends-on, as you mention.

> I think it’s strange that ‘deliver-asd-op’ make a asd file without all
> its original depends, if the original are valid, maybe we can copy
> them back?
> 

That's basically what's being done in a slightly roundabout way. The
key thing is that we let asdf know where the dependencies need to be
found first, so that we can avoid propagation. Copying the :depends-on
field could be difficult since it can contain reader expressions. We'd
need some logic to do that, or have a lisp implementation help us out.
I don't think specifying #:special-dependencies is too onerous; the
package creator is required to specify dependencies anyway.

> > […]
> > +(define* (generate-binary #:key outputs
> > +                          inputs
> > +                          image-dependencies
> > +                          entry-program
> > +                          lisp
> > +                          binary?
> > +                          #:allow-other-keys)
> > +  "Generate a binary program for the system, either in \"bin\" if
> > the package +also contains a library system, or in \"out\"
> > otherwise."
> > +  (define output (if binary? "out" "bin"))
> > +  (generate-executable #:outputs outputs
> > +                       #:inputs inputs
> > +                       #:image-dependencies image-dependencies
> > +                       #:entry-program entry-program
> > +                       #:lisp lisp
> > +                       #:output output
> > +                       #:needs-own-system? (not binary?)
> > +                       #:type "program")
> > +  (and=>
> > +   (assoc-ref outputs output)
> > +   (lambda (bin)
> > +     (let* ((full-name (outputs->name outputs))
> > +            (name (if binary? full-name
> > +                      (remove-lisp-from-name full-name lisp)))
> > +            (bin-directory (string-append bin "/bin")))
> > +       (with-directory-excursion bin-directory
> > +         (rename-file (string-append name "-exec")
> > +                      name)))))
> > +  #t)
> > +
> > +(define* (generate-image #:key outputs
> > +                         inputs
> > +                         image-dependencies
> > +                         lisp
> > +                         image?
> > +                         #:allow-other-keys)
> > +  "Generate an image for the system, possibly standalone, either
> > in \"image\" +if the package also contains a library system, or in
> > \"out\" otherwise."
> > +  (define output (if image? "out" "image"))
> > +  (generate-executable #:outputs outputs
> > +                       #:inputs inputs
> > +                       #:image-dependencies image-dependencies
> > +                       #:entry-program '(nil)
> > +                       #:lisp lisp
> > +                       #:output output
> > +                       #:needs-own-system? (not image?)
> > +                       #:type "image")
> > +  (and=>
> > +   (assoc-ref outputs output)
> > +   (lambda (image)
> > +     (let* ((full-name (outputs->name outputs))
> > +            (name (if image? full-name
> > +                      (remove-lisp-from-name full-name lisp)))
> > +            (bin-directory (string-append image "/bin")))
> > +       (with-directory-excursion bin-directory
> > +         (rename-file (string-append name
> > "-exec--all-systems.image")
> > +                      (string-append name ".image"))))))
> > +  #t)
> > +
> > +(define* (generate-executable #:key outputs
> > +                              image-dependencies
> > +                              entry-program
> > +                              lisp
> > +                              output
> > +                              inputs
> > +                              type
> > +                              needs-own-system?
> > +                              #:allow-other-keys)
> > +  "Generate an executable by using asdf's TYPE-op, containing
> > whithin the +image all IMAGE-DEPNDENCIES, and running ENTRY-PROGRAM
> > in the case of an +executable."
> > +  (and=>
> > +   (assoc-ref outputs output)
> > +   (lambda (out)
> > +     (let* ((bin-directory (string-append out "/bin"))
> > +            (full-name (outputs->name outputs))
> > +            (name (if needs-own-system?
> > +                      (remove-lisp-from-name full-name lisp)
> > +                      full-name)))
> > +       (mkdir-p out)
> > +       (with-directory-excursion out
> > +         (generate-executable-wrapper-system name
> > +                                             image-dependencies
> > +                                             needs-own-system?)
> > +         (generate-executable-entry-point name entry-program))
> > +
> > +       (setenv "CL_SOURCE_REGISTRY"
> > +               (replace-escaped-macros
> > +                (format
> > +                 #f "~S"
> > +                 (wrap-source-registry
> > +                  `(,(source-registry (assoc-ref outputs "out"))
> > +                    ,(source-registry out))))))
> > +
> > +       (setenv "ASDF_OUTPUT_TRANSLATIONS"
> > +               (replace-escaped-macros
> > +                (format
> > +                 #f "~S"
> > +                 (wrap-output-translations
> > +                  `(((,out :**/ :*.*.*)
> > +                     (,bin-directory :**/ :*.*.*)))))))
> > +
> > +       (parameterize ((%lisp (string-append
> > +                              (assoc-ref inputs lisp) "/bin/"
> > lisp)))
> > +         (generate-executable-for-system type name lisp))
> > +
> > +       (delete-file (string-append out "/" name "-exec.asd"))
> > +       (delete-file (string-append out "/" name "-exec.lisp"))))))
> > +  
> 
> Yeah, I’d like to merge those into lisp-utils.scm, and become
> something like ‘wrap-program’.
> 

Ok, sounds good.

> 
> Also, the output-translations way sometime produces unnecessary
> files, eg: in ecl-fiveam:
> 
> --8<---------------cut here---------------start------------->8---
> /gnu/store/jy5525lyx0lk20h02g6ik5q4qfkjcnz6-ecl-fiveam-1.2/lib
> /gnu/store/jy5525lyx0lk20h02g6ik5q4qfkjcnz6-ecl-fiveam-1.2/lib/ecl
> /gnu/store/jy5525lyx0lk20h02g6ik5q4qfkjcnz6-ecl-fiveam-1.2/lib/ecl/src
> /gnu/store/jy5525lyx0lk20h02g6ik5q4qfkjcnz6-ecl-fiveam-1.2/lib/ecl/src/fiveam.fasb
> /gnu/store/jy5525lyx0lk20h02g6ik5q4qfkjcnz6-ecl-fiveam-1.2/lib/ecl/src/fiveam.asd
> /gnu/store/jy5525lyx0lk20h02g6ik5q4qfkjcnz6-ecl-fiveam-1.2/lib/ecl/src/fiveam.a
> /gnu/store/jy5525lyx0lk20h02g6ik5q4qfkjcnz6-ecl-fiveam-1.2/lib/ecl/t
> --8<---------------cut here---------------end--------------->8---
> 
> I think we only want ’lib/ecl/fiveam.*’.
> 

Right, I didn't notice that.

> I have tried ‘asdf/action:output-files’, hoping that will allow
> install only the files we want, but it doesn’t seem give all the
> output files.
> 
> Run this with ECL:
> 
> --8<---------------cut here---------------start------------->8---
> (require :asdf)
> 
> (let* ((asdf/output-translations:*output-translations*
>       '(((t #P"/tmp/build/**/*.*"))))
>        (files
>       (asdf/action:output-files
>        'asdf:compile-bundle-op (asdf:find-system :cl-json))))
>   (asdf:operate 'asdf:compile-bundle-op :cl-json)
>   (format t "~S" files))
> 
> (quit)
> --8<---------------cut here---------------end--------------->8---
> 
> The ‘fasb’ file is returned, but the ‘a’ file is missing.
> What the ECL lib (‘a’) files used for?
> 

It gets created by the 'compile-bundle-op', so I assumed it was
necessary, but it turns out it's perfectly possible to load systems
without it. My guess is that it's just a convenience for C developpers,
and we don't really need it. It could be useful if we ever find a C
program that depends on ECL libraries, but I'm willing to remove it.

> 
> Despite my opinions and ideas, please let me know if this patch is
> considered ready, so we can merge it and improve later.

The patch is in a working state, but I have no problem implementing
your suggestions and continuing discussion. It shouldn't take too long.

> 
> Thanks!

Thank you.

--
Andy