[bug#28444] [PATCH 3/3] build-system: Add 'meson-build-system'.

[Peter Mikkelsen]

2017-09-15 23:07 GMT+02:00 Ludovic Courtès <address@hidden>:
>
> Peter Mikkelsen <address@hidden> skribis:
>
> > * Makefile.am (MODULES): Add 'guix/build-system/meson.scm' and
> >   'guix/build/meson-build-system.scm'.
> > * guix/build-system/meson.scm: New file.
> > * guix/build/meson-build-system.scm: New file.
> > * doc/guix.texi (Build Systems): Add 'meson-build-system'.
>
> Overall LGTM!  I have minor questions and comments:
>
> > +(define* (configure #:key outputs configure-flags build-type
> > +                    #:allow-other-keys)
> > +  "Configure the given package"
>
> Make sure to add a period at the end of sentences.  :-)

Oops, will do :)

>
> > +(define* (fix-runpath #:key elf-directories outputs
> > +                      #:allow-other-keys)
>
> ELF-DIRECTORIES should default to a list, probably the empty list (here
> it defaults to #f, which cannot work.)

I thought it was enough giving it a default value in
guix/build-system/meson.scm, but allright, I have added it here.

>
> > +  "Try to make sure all ELF files in ELF-DIRECTORIES are able to find their
> > +local dependencies in their RUNPATH.  Also shrink the RUNPATH to what is 
> > needed,
> > +since alot of directories are left over from meson."
>
> “a lot”
>
> According to this description, half of it corresponds to the
> ‘validate-runpath’ phase, no?

To my understanding 'validate-runpath' just checks and complains, but
this phase checks if it can find any dependencies in directories that
are local to the package, and if it can, it adds them to the runpath.
I have tried to update the description :)
>
> The second half is the shrink-RUNPATH thing, but can you clarify why it
> is needed?  Which directories in RUNPATH are “left over from meson”?

I also tried giving an example of that in the new description.
>
> > +  (define (find-deps dep-name elf-files)
> > +    "Find the directories (if any) that contains DEP-NAME.  The directories
> > +searched are the ones that ELF-FILES are in."
> > +    (let* ((matches (filter (lambda (file)
> > +                              (string=? dep-name (basename file)))
> > +                            elf-files)))
> > +      (map dirname matches)))
>
> Avoid local variable ‘matches’, to keep it concise.  Also, for inner
> ‘define’s, use a comment instead of a docstring (the docstring would be
> inaccessible.)
>
OK.

> > +  (define (file-needed file)
> > +    "Return a list of libraries that FILE needs."
> > +    (let* ((elf (call-with-input-file file
> > +                  (compose parse-elf get-bytevector-all)))
> > +           (dyninfo (elf-dynamic-info elf)))
> > +      (if dyninfo
> > +          (elf-dynamic-info-needed dyninfo)
> > +          '())))
> > +
> > +  (define (handle-file file elf-files)
> > +    "If FILE needs any libs that are part of ELF-FILES, the RUNPATH
> > +is modified accordingly."
> > +    (let* ((dep-dirs (apply append (map (lambda (dep-name)
> > +                                          (find-deps dep-name elf-files))
> > +                                        (file-needed file)))))
> > +      (unless (null? dep-dirs)
> > +        (augment-rpath file (string-join dep-dirs ":")))))
> > +
> > +  (define handle-output
> > +    (match-lambda
> > +              (elf-list (apply append (map (lambda (dir)
> > +                                             (find-files dir elf-pred))
> > +                                           excisting-elf-dirs))))
>
> (apply append lstlst) = (concatenate lstlst)
>
I assume it is the 'concatenate' from (srfi srfi-1), so I have added
it to the imports
> That’s it!  Could you comment and send an updated patch?
>
> Thanks for working on it, looks like it’s going to be useful very soon!
>
> Ludo’.

Updated patch attached, thanks for reviewing!

Peter

0001-build-system-Add-meson-build-system.patch
Description: Text Data