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

Hi Jelle,

On 9/20/20 9:51 PM, Jelle Licht wrote:
> Hey Giacomo,
>
> Apologies for the delay! Better late than never, a review just for you.
No problem really, I spent some time AFK this summer and didn't ping 
soon enough.
> 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.
Yeah I actually don't know why I avoided to type two more letters in the 
first place. I didn't build the patterns here because that would have 
required storing the match result in a separate variable binding and 
requiring to check twice if the binding was false (which is the way I 
went in the new patch. The only slight downside in the new patch is that 
if the match result is #f then patterns is #<unspecified> but is also 
provably never accessed. If you can think of a better way to solve this, 
please do tell me), but mapping first is still more efficient, so I 
changed it.
>
>> +                     (#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.
Definitely, I just fixed that.
>
>> +                          (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'.
Thanks, I didn't know. Fixed :).
>
>> +         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?
I put this because also the "main" field from package.json is also 
guaranteed to be installed by NPM, according to 
https://docs.npmjs.com/files/package.json#main . Thus if a developer 
populates the "files" field without including the main file in that 
list, but they do insert it in the "main" field the file should be 
installed. Does it make sense?
> Thanks for you patience, and thanks again for working on this.
>
> HTH,
>
>   - Jelle

Thank you for your patience in reviewing this patch. I'm attaching an 
updated version of the second patch.

Cheers,

Giacomo

Hi Jelle,

just a quick follow up that I forgot yesterday.


>>> +                 (map (lambda (path)
>>> +                        (string-drop path 2))
>>                             ^
>>                           If this is meant to drop the "./" prefix, you
>>                           should be able to leave it out.
>>
This seems to be necessary because given the way glob-match? is 
implemented a string matches a pattern iif either they start with the 
same character or the pattern starts with a wildcard. So for example:


scheme@(guix-user)> ,use (guix glob)

scheme@(guix-user)> (string->compiled-sglob "*.json")

$1 = (* ".json")

scheme@(guix-user)> (string->compiled-sglob "package.json")

$2 = "package.json"

scheme@(guix-user)> (glob-match? $1 "./package.json")

$3 = #t

scheme@(guix-user)> (glob-match? $2 "./package.json")

$4 = #f


Thank you again for your help,

Cheers

Giacomo

Hey Giacomo,

paul <goodoldpaul@autistici.org> writes:
>>> +         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)))))
                                          ^

{New,Forgotten} nitpick; this only checks for the `dirname': why not
just `(string-append install-dir "/" main)'? Because if
e.g. "lib/utils.js" is in "files", and main is "lib/main.js", it seems
that main would not be installed with this snippet. Does that make
sense?

Thanks in advance,

- Jelle