[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#62375] [PATCH] import: Add binary npm importer.
From: |
Jelle Licht |
Subject: |
[bug#62375] [PATCH] import: Add binary npm importer. |
Date: |
Sat, 08 Apr 2023 20:29:18 +0200 |
Ludovic Courtès <ludo@gnu.org> writes:
> Hello Jelle!
>
> jlicht@fsfe.org skribis:
>
>> From: Jelle Licht <jlicht@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@ngyro.com>
>> Co-authored-by: Lars-Dominik Braun <lars@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
- [bug#62375] [PATCH] import: Add binary npm importer.,
Jelle Licht <=