[bug#41219] [PATCH 2/2] guix: Enforce package.json "files" directive.

[Jelle Licht]

Hey Giacomo, 

Apologies for the delay! Better late than never, a review just for you.
The other patch seems fine to me, but I'm not a 'guix glob' expert.

Giacomo Leidi <goodoldpaul@autistici.org> writes:

> [snip]
> --- a/guix/build/node-build-system.scm
> +++ b/guix/build/node-build-system.scm
> @@ -1,6 +1,7 @@
>  ;;; GNU Guix --- Functional package management for GNU
>  ;;; Copyright © 2015 David Thompson <davet@gnu.org>
>  ;;; Copyright © 2016 Jelle Licht <jlicht@fsfe.org>
> +;;; Copyright © 2020 Giacomo Leidi <goodoldpaul@autistici.org>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -22,6 +23,7 @@
>    #:use-module (guix build json)
>    #:use-module (guix build union)
>    #:use-module (guix build utils)
> +  #:use-module (guix glob)
>    #:use-module (ice-9 match)
>    #:use-module (ice-9 popen)
>    #:use-module (ice-9 regex)
> @@ -110,18 +112,60 @@ the @file{bin} directory."
>                                (#f #f)))
>           (dependencies (match (assoc-ref data "dependencies")
>                           (('@ deps ...) deps)
> -                         (#f #f))))
> +                         (#f #f)))
> +         (patterns (match (assoc-ref data "files")
> +                     (() #f)
> +                     ((? list? patrn-list) patrn-list)
                                  ^
Perhaps 'pattern-list'? I keep reading this as patron-list. We could
also build the patterns here. Mapping over the pattern-list + 'default
patterns' here might also be a wee bit faster.

> +                     (#f #f)))
> +         (main (match (assoc-ref data "main")
> +                     ("" #f)
> +                     ((? string? main-module) main-module)
> +                     (#f #f)))
> +         (install-dir (string-append target "/node_modules/" modulename))
> +         (install-files (lambda (files directory)
                                          ^
You only use install-dir here: you could hard-code it in the lambda.

> +                          (for-each (lambda (file)
> +                                      (install-file
> +                                       file
> +                                       (string-append directory "/"
> +                                                      (dirname file))))
> +                                    files))))

>      (mkdir-p target)
> -    (copy-recursively "." (string-append target "/node_modules/" modulename))
> -    ;; Remove references to dependencies
> -    (delete-file-recursively
> -      (string-append target "/node_modules/" modulename "/node_modules"))
> +    (if patterns
> +        (install-files
> +         (filter (lambda (file)
> +                   (any (lambda (pattern)
> +                          (glob-match?
> +                           (string->compiled-sglob pattern)
> +                           file))
> +                        (append
> +                         patterns
> +                         '("package.json"
> +                           ;; These files get installed no
> +                           ;; matter the case or extension.
> +                           "[rR][eE][aA][dD][mM][eE]*"
> +                           "[cC][hH][aA][nN][gG][eE][sS]*"
> +                           "[cC][hH][aA][nN][gG][eE][lL][oO][gG]*"
> +                           "[hH][iI][sS][tT][oO][rR][yY]*"
> +                           "[nN][oO][tT][iI][cC][eE]*"))))
> +                 (map (lambda (path)
> +                        (string-drop path 2))
                           ^
                         If this is meant to drop the "./" prefix, you
                         should be able to leave it out.

> +                      (find-files ".")))
`find-files' accepts an optional second argument called PRED, so you can
do that instead of the earlier 'filter'.

> +         install-dir)
> +        (begin
> +          (copy-recursively "." install-dir)
> +          ;; Remove references to dependencies
> +          (delete-file-recursively
> +           (string-append install-dir "/node_modules"))))
> +    (if (and main
> +             (not (file-exists?
> +                   (string-append
> +                    install-dir "/" (dirname main)))))
> +        (install-files (list main) install-dir))
           ^

This should not be needed if we use the 'old' (=non-files) approach of
installing. Do you think it makes sense to pull it into the previous
block that only runs on using the 'files' directive?

Thanks for you patience, and thanks again for working on this.

HTH,

 - Jelle