GNU bug report logs - #31736
[PATCH] Add an opam importer

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 31736 in the body.
You can then email your comments to 31736 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Julien Lepiller <julien <at> lepiller.eu>
To: guix-patches <at> gnu.org
Subject: [PATCH] Add an opam importer
Date: Wed, 6 Jun 2018 19:23:29 +0200

[Message part 1 (text/plain, inline)]

Hi, this patch adds an importer for ocaml packages from the opam
repository.

[0001-guix-Add-opam-importer.patch (text/x-patch, attachment)]

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

From: Julien Lepiller <julien <at> lepiller.eu>
To: guix-patches <at> gnu.org
Subject: Re: [bug#31736] [PATCH] Add an opam importer
Date: Wed, 6 Jun 2018 19:37:40 +0200

[Message part 1 (text/plain, inline)]

Le Wed, 6 Jun 2018 19:23:29 +0200,
Julien Lepiller <julien <at> lepiller.eu> a écrit :

> Hi, this patch adds an importer for ocaml packages from the opam
> repository.

Whoops, the copyright lines and part of the code was wrong. This
version should be better :)

[0001-guix-Add-opam-importer.patch (text/x-patch, attachment)]

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 31736 <at> debbugs.gnu.org
Subject: Re: [bug#31736] [PATCH] Add an opam importer
Date: Sun, 10 Jun 2018 23:28:26 +0200

Salut Julien!

Julien Lepiller <julien <at> lepiller.eu> skribis:

> From a5250186722305961f0a5d77cb8f7f36cdae0da0 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien <at> lepiller.eu>
> Date: Wed, 6 Jun 2018 19:14:39 +0200
> Subject: [PATCH] guix: Add opam importer.
>
> * guix/scripts/import.scm (importers): Add opam.
> * guix/scripts/import/opam.scm: New file.
> * guix/import/opam.scm: New file.
> * Makefile.am: Add them.

Very nice!  I hope that’ll help create bridges with more fellow OCaml
hackers.  :-)

I have some comments, mostly about style:

> +(define (htable-update htable line)
> +  "Parse @var{line} to get the name and version of the package and adds them
> +to the hashtable."
> +  (let* ((line (string-split line #\ ))
> +         (url (car line)))
> +    (unless (equal? url "repo")
> +      (let ((sp (string-split url #\/)))
> +        (when (equal? (car sp) "packages")
> +          (let* ((versionstr (car (cdr (cdr sp))))
> +                 (name1 (car (cdr sp)))
> +                 (name2 (car (string-split versionstr #\.)))
> +                 (version (string-join (cdr (string-split versionstr #\.)) ".")))
> +            (when (equal? name1 name2)
> +              (let ((curr (hash-ref htable name1 '())))
> +                (hash-set! htable name1 (cons version curr))))))))))

There are a couple of things that don’t fully match the coding style
(see <https://www.gnu.org/software/guix/manual/html_node/Coding-Style.html>):
try to use full names for identifiers, favor a functional style (here
maybe you could use a vhash¹ instead of a hash table), and, last but not
least, use ‘match’ instead of ‘car’ and ‘cdr’.  :-)

¹ https://www.gnu.org/software/guile/manual/html_node/VHashes.html

> +(define (fetch-url uri)
> +  "Fetch and parse the url file.  Return the URL the package can be downloaded
> +from."

Maybe ‘fetch-url-list’ or ‘fetch-package-urls’?

> +(define (fetch-metadata uri)
> +  "Fetch and parse the opam file.  Return an association list containing the
> +homepage, the license and the list of inputs."

Maybe ‘fetch-package-metadata’ to clarify that it’s per-package?

> +(define (deps->inputs deps)
> +  "Transform the list of dependencies in a list of inputs.  Filter out anything
> +that looks like a native-input."

So that would be ‘dependencies->inputs’.  :-)

> +  (if (eq? deps #f)

Rather: (if (not dependencies) …)

> +    (let ((inputs
> +            (map (lambda (input)
> +                   (list input (list 'unquote (string->symbol input))))
> +              (map (lambda (input)
> +                     (cond
> +                       ((equal? input "ocamlfind") "ocaml-findlib")
> +                       ((string-prefix? "ocaml" input) input)
> +                       (else (string-append "ocaml-" input))))
> +                (filter (lambda (input) (not (string-prefix? "conf-" input))) deps)))))

The indentation is misleading: the 2nd argument to ‘map’ should be
aligned with the 1st.

Perhaps you can use ‘filter-map’ here?

> +      (if (eq? (length inputs) 0) '() inputs))))

(eq? (length inputs) 0) → (null? inputs)

Note that ‘null?’ is constant-time whereas ‘length’ is O(n).

Could you add:

  • A few lines in guix.texi, under “Invoking guix import”;

  • Tests in tests/opam.scm (you can take a look at tests/cpan.scm,
    tests/elpa.scm, etc. for inspiration.)

Thank you!

Ludo’.

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

From: Julien Lepiller <julien <at> lepiller.eu>
To: 31736 <at> debbugs.gnu.org
Subject: Re: [bug#31736] [PATCH] Add an opam importer
Date: Sat, 07 Jul 2018 23:56:08 +0200

[Message part 1 (text/plain, inline)]

Le 2018-06-10 23:28, ludo <at> gnu.org a écrit :
> Salut Julien!
> 
> Julien Lepiller <julien <at> lepiller.eu> skribis:
> 
>> From a5250186722305961f0a5d77cb8f7f36cdae0da0 Mon Sep 17 00:00:00 2001
>> From: Julien Lepiller <julien <at> lepiller.eu>
>> Date: Wed, 6 Jun 2018 19:14:39 +0200
>> Subject: [PATCH] guix: Add opam importer.
>> 
>> * guix/scripts/import.scm (importers): Add opam.
>> * guix/scripts/import/opam.scm: New file.
>> * guix/import/opam.scm: New file.
>> * Makefile.am: Add them.
> 
> Very nice!  I hope that’ll help create bridges with more fellow OCaml
> hackers.  :-)
> 
> I have some comments, mostly about style:
> 
>> +(define (htable-update htable line)
>> +  "Parse @var{line} to get the name and version of the package and 
>> adds them
>> +to the hashtable."
>> +  (let* ((line (string-split line #\ ))
>> +         (url (car line)))
>> +    (unless (equal? url "repo")
>> +      (let ((sp (string-split url #\/)))
>> +        (when (equal? (car sp) "packages")
>> +          (let* ((versionstr (car (cdr (cdr sp))))
>> +                 (name1 (car (cdr sp)))
>> +                 (name2 (car (string-split versionstr #\.)))
>> +                 (version (string-join (cdr (string-split versionstr 
>> #\.)) ".")))
>> +            (when (equal? name1 name2)
>> +              (let ((curr (hash-ref htable name1 '())))
>> +                (hash-set! htable name1 (cons version curr))))))))))
> 
> There are a couple of things that don’t fully match the coding style
> (see 
> <https://www.gnu.org/software/guix/manual/html_node/Coding-Style.html>):
> try to use full names for identifiers, favor a functional style (here
> maybe you could use a vhash¹ instead of a hash table), and, last but 
> not
> least, use ‘match’ instead of ‘car’ and ‘cdr’.  :-)
> 
> ¹ https://www.gnu.org/software/guile/manual/html_node/VHashes.html
> 
>> +(define (fetch-url uri)
>> +  "Fetch and parse the url file.  Return the URL the package can be 
>> downloaded
>> +from."
> 
> Maybe ‘fetch-url-list’ or ‘fetch-package-urls’?
> 
>> +(define (fetch-metadata uri)
>> +  "Fetch and parse the opam file.  Return an association list 
>> containing the
>> +homepage, the license and the list of inputs."
> 
> Maybe ‘fetch-package-metadata’ to clarify that it’s per-package?
> 
>> +(define (deps->inputs deps)
>> +  "Transform the list of dependencies in a list of inputs.  Filter 
>> out anything
>> +that looks like a native-input."
> 
> So that would be ‘dependencies->inputs’.  :-)
> 
>> +  (if (eq? deps #f)
> 
> Rather: (if (not dependencies) …)
> 
>> +    (let ((inputs
>> +            (map (lambda (input)
>> +                   (list input (list 'unquote (string->symbol 
>> input))))
>> +              (map (lambda (input)
>> +                     (cond
>> +                       ((equal? input "ocamlfind") "ocaml-findlib")
>> +                       ((string-prefix? "ocaml" input) input)
>> +                       (else (string-append "ocaml-" input))))
>> +                (filter (lambda (input) (not (string-prefix? "conf-" 
>> input))) deps)))))
> 
> The indentation is misleading: the 2nd argument to ‘map’ should be
> aligned with the 1st.
> 
> Perhaps you can use ‘filter-map’ here?
> 
>> +      (if (eq? (length inputs) 0) '() inputs))))
> 
> (eq? (length inputs) 0) → (null? inputs)
> 
> Note that ‘null?’ is constant-time whereas ‘length’ is O(n).
> 
> Could you add:
> 
>   • A few lines in guix.texi, under “Invoking guix import”;
> 
>   • Tests in tests/opam.scm (you can take a look at tests/cpan.scm,
>     tests/elpa.scm, etc. for inspiration.)
> 
> Thank you!
> 
> Ludo’.


Here is a new version. What do you think?

[0001-guix-Add-opam-importer.patch (text/x-diff, attachment)]

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 31736 <at> debbugs.gnu.org
Subject: Re: [bug#31736] [PATCH] Add an opam importer
Date: Mon, 09 Jul 2018 14:44:20 +0200

Hello Julien,

Julien Lepiller <julien <at> lepiller.eu> skribis:

> Here is a new version. What do you think?

I think it looks great.  :-)

> From b3fd75a804a4264cb083584591a3e589886d96c8 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien <at> lepiller.eu>
> Date: Wed, 6 Jun 2018 19:14:39 +0200
> Subject: [PATCH] guix: Add opam importer.
>
> * guix/scripts/import.scm (importers): Add opam.
> * guix/scripts/import/opam.scm: New file.
> * guix/import/opam.scm: New file.
> * tests/opam.scm: New file.
> * Makefile.am: Add them.
> * doc/guix.texi (Invoking guix import): Document it.

[...]

> +@item opam
> +@cindex opam
> +@cindex ocaml
> +Import metadata from the @uref{https://opam.ocaml.org/, Opam} package
> +repository used by the OCaml community.

Nitpick: the proper spelling is “OPAM” and “OCaml”, so I think you need
to fix these above (in the concept index as well.)

Otherwise LGTM!

As a next step, you can add an OPAM updater, which will hopefully take
you no more than a few lines of code (see the bottom of cpam.scm, for
example.)

:-)

Thank you!

Ludo’.

Message #22 received at 31736-done <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: 31736-done <at> debbugs.gnu.org
Subject: Re: [bug#31736] [PATCH] Add an opam importer
Date: Tue, 10 Jul 2018 13:47:58 +0200

Le Mon, 09 Jul 2018 14:44:20 +0200,
ludo <at> gnu.org (Ludovic Courtès) a écrit :

> Hello Julien,
> 
> Julien Lepiller <julien <at> lepiller.eu> skribis:
> 
> > Here is a new version. What do you think?  
> 
> I think it looks great.  :-)
> 
> > From b3fd75a804a4264cb083584591a3e589886d96c8 Mon Sep 17 00:00:00
> > 2001 From: Julien Lepiller <julien <at> lepiller.eu>
> > Date: Wed, 6 Jun 2018 19:14:39 +0200
> > Subject: [PATCH] guix: Add opam importer.
> >
> > * guix/scripts/import.scm (importers): Add opam.
> > * guix/scripts/import/opam.scm: New file.
> > * guix/import/opam.scm: New file.
> > * tests/opam.scm: New file.
> > * Makefile.am: Add them.
> > * doc/guix.texi (Invoking guix import): Document it.  
> 
> [...]
> 
> > +@item opam
> > +@cindex opam
> > +@cindex ocaml
> > +Import metadata from the @uref{https://opam.ocaml.org/, Opam}
> > package +repository used by the OCaml community.  
> 
> Nitpick: the proper spelling is “OPAM” and “OCaml”, so I think you
> need to fix these above (in the concept index as well.)
> 
> Otherwise LGTM!
> 
> As a next step, you can add an OPAM updater, which will hopefully take
> you no more than a few lines of code (see the bottom of cpam.scm, for
> example.)
> 
> :-)
> 
> Thank you!
> 
> Ludo’.

Pushed as b24443bff9f9f3f36353eea2ef35e6dc3745a417

Previous Next

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