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

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

Peter Mikkelsen <petermikkelsen10@gmail.com> 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.  :-)

> +(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.)

> +  "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?

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”?

> +  (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.)

> +  (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)

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’.