GNU bug report logs - #62375
[PATCH 0/1] npm binary importer

Previous Next

Full log

Message #16 received at 62375 <at> debbugs.gnu.org (full text, mbox):

From: Jelle Licht <jlicht <at> fsfe.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Timothy Sample <samplet <at> ngyro.com>, Josselin Poiret <dev <at> jpoiret.xyz>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Christopher Baines <mail <at> cbaines.net>, Lars-Dominik Braun <lars <at> 6xq.net>,
 62375 <at> debbugs.gnu.org, Ricardo Wurmus <rekado <at> elephly.net>
Subject: Re: [PATCH] import: Add binary npm importer.
Date: Sat, 08 Apr 2023 20:29:18 +0200

Ludovic Courtès <ludo <at> gnu.org> writes:

> Hello Jelle!
>
> jlicht <at> fsfe.org skribis:
>
>> From: Jelle Licht <jlicht <at> fsfe.org>
>>
>> * guix/scripts/import.scm: (importers): Add "npm-binary".
>> * guix/import/npm-binary.scm: New file.
>> * guix/scripts/import/npm-binary.scm: New file.
>> * Makefile.am: Add them.
>>
>> Co-authored-by: Timothy Sample <samplet <at> ngyro.com>
>> Co-authored-by: Lars-Dominik Braun <lars <at> 6xq.net>
>
> Woohoo!  I think it’ll be useful to many, even if we know it’s far from
> “ideal” by our standards.
>
> We’ll need doc under “Invoking guix import” and tests in
> ‘tests/npm-binary.scm’, similar to what’s done for the other importers.
> The doc should clarify why it’s called that way and what the limitations
> are.
>
> For tests, I’d recommend mocking the npmjs.org HTTP servers using
> ‘with-http-server’ as in ‘tests/cpan.scm’.
>
> Also, please add docstrings to top-level procedures.
>
>> +;; TODO: Support other registries
>> +(define *registry* "https://registry.npmjs.org")
>> +(define *default-page* "https://www.npmjs.com/package")
>
> The convention currently is more like ‘%registry’.
>
>> +(define (http-error-code arglist)
>> +  (match arglist
>> +    (('http-error _ _ _ (code)) code)
>> +    (_ #f)))
>
> Unused.  :-)
>
>> +(define (hash-url url)
>> +  "Downloads the resource at URL and computes the base32 hash for it."
>> +  (call-with-temporary-output-file
>> +   (lambda (temp port)
>> +     (begin ((@ (guix import utils) url-fetch) url temp)
>> +            (guix-hash-url temp)))))
>
> Maybe something more like: (port-sha256 (http-fetch …)) ?
>
>> +(define (npm-package->package-sexp npm-package)
>> +  "Return the `package' s-expression for an NPM-PACKAGE."
>> +  (define (new-or-existing-inputs resolved-deps)
>> +    (map package-revision->input resolved-deps))
>> +
>> +  (match npm-package
>> +    (($ <package-revision> name version home-page dependencies dev-dependencies peer-dependencies license description dist)
>
> Please use ‘match-record’ instead and keep lines below 80 chars.  :-)

The records generated by `define-json-mapping' through
`define-record-type' seem to not work with `match-record'.

I could define our very own `define-json-mapping*' that works /w
`define-record-type*' instead (and through that, `match-record'), but I
thought I'd ask if you had something else in mind first :).

>> +     (let* ((name (npm-name->name name))
>> +            (url (dist-tarball dist))
>> +            (home-page (if (string? home-page)
>> +                           home-page
>> +                           (string-append *default-page* "/" (uri-encode name))))
>> +            (synopsis description)
>> +            (resolved-deps (map (match-lambda (($ <versioned-package> name version)
>> +                                               (resolve-package name (string->semver-range version)))) (append dependencies peer-dependencies)))
>
> Likewise.
>
> That’s it!  Could you send an updated patch?

Will do, once I got some proper test cases set up.

Thanks again for the review! 
- Jelle

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.