bug#26373: Various improvements to the CRAN importer

bug#26373: Various improvements to the CRAN importer

[Ricardo Wurmus]

The following patch series improves the CRAN importer.

bug#26373: [PATCH 1/6] import cran: Exclude experiment packages in predicate "bioconductor-package?".

[Ricardo Wurmus]

* guix/import/cran.scm (bioconductor-package?): Exclude experiment packages,
because they cannot be updated with the default bioconductor updater.
---
 guix/import/cran.scm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index 8e24f6e17..f63d23972 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -429,7 +429,9 @@ dependencies."
                           ;; the Github mirror, so we have to exclude them
                           ;; from the set of bioconductor packages that can be
                           ;; updated automatically.
-                          (not (string-contains uri "/data/annotation/"))))))
+                          (not (string-contains uri "/data/annotation/"))
+                          ;; Experiment packages are in a separate repository.
+                          (not (string-contains uri "/data/experiment/"))))))
     (and (string-prefix? "r-" (package-name package))
          (match (and=> (package-source package) origin-uri)
            ((? string? uri)
-- 
2.12.2

bug#26373: [PATCH 2/6] import cran: Add predicate for Bioconductor experiment packages.

[Ricardo Wurmus]

* guix/import/cran.scm (bioconductor-experiment-package?): New variable.
---
 guix/import/cran.scm | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index f63d23972..48ab7355d 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -453,6 +453,19 @@ dependencies."
             (any predicate uris))
            (_ #f)))))
 
+(define (bioconductor-experiment-package? package)
+  "Return true if PACKAGE is an R experiment package from Bioconductor."
+  (let ((predicate (lambda (uri)
+                     (and (string-prefix? "http://bioconductor.org" uri)
+                          (string-contains uri "/data/experiment/")))))
+    (and (string-prefix? "r-" (package-name package))
+         (match (and=> (package-source package) origin-uri)
+           ((? string? uri)
+            (predicate uri))
+           ((? list? uris)
+            (any predicate uris))
+           (_ #f)))))
+
 (define %cran-updater
   (upstream-updater
    (name 'cran)
-- 
2.12.2

bug#26373: [PATCH 3/6] import cran: Refactor "needs-zlib?".

[Ricardo Wurmus]

* guix/import/cran.scm (tarball-files-match-pattern?): New procedure.
(needs-zlib?): Implement in terms of "tarball-files-match-pattern?".
---
 guix/import/cran.scm | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index 48ab7355d..be3b678cd 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -201,17 +201,16 @@ empty list when the FIELD cannot be found."
       (check "*.f95")
       (check "*.f")))
 
-(define (needs-zlib? tarball)
-  "Return #T if any of the Makevars files in the src directory of the TARBALL
-contain a zlib linker flag."
+(define (tarball-files-match-pattern? tarball regexp . file-patterns)
+  "Return #T if any of the files represented by FILE-PATTERNS in the TARBALL
+match the given REGEXP."
   (call-with-temporary-directory
    (lambda (dir)
-     (let ((pattern (make-regexp "-lz")))
+     (let ((pattern (make-regexp regexp)))
        (parameterize ((current-error-port (%make-void-port "rw+")))
-         (system* "tar"
-                  "xf" tarball "-C" dir
-                  "--wildcards"
-                  "*/src/Makevars*" "*/src/configure*" "*/configure*"))
+         (apply system* "tar"
+                "xf" tarball "-C" dir
+                `("--wildcards" ,@file-patterns)))
        (any (lambda (file)
               (call-with-input-file file
                 (lambda (port)
@@ -220,10 +219,16 @@ contain a zlib linker flag."
                       (cond
                        ((eof-object? line) #f)
                        ((regexp-exec pattern line) #t)
-                       (else (loop)))))))
-              #t)
+                       (else (loop))))))))
             (find-files dir))))))
 
+(define (needs-zlib? tarball)
+  "Return #T if any of the Makevars files in the src directory of the TARBALL
+contain a zlib linker flag."
+  (tarball-files-match-pattern?
+   tarball "-lz"
+   "*/src/Makevars*" "*/src/configure*" "*/configure*"))
+
 (define (description->package repository meta)
   "Return the `package' s-expression for an R package published on REPOSITORY
 from the alist META, which was derived from the R package's DESCRIPTION file."
-- 
2.12.2

bug#26373: [PATCH 4/6] import cran: Check if pkg-config is needed.

[Ricardo Wurmus]

* guix/import/cran.scm (needs-pkg-config?): New procedure.
(description->package): Use it.
---
 guix/import/cran.scm | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index be3b678cd..423835637 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -229,6 +229,13 @@ contain a zlib linker flag."
    tarball "-lz"
    "*/src/Makevars*" "*/src/configure*" "*/configure*"))
 
+(define (needs-pkg-config? tarball)
+  "Return #T if any of the Makevars files in the src directory of the TARBALL
+reference the pkg-config tool."
+  (tarball-files-match-pattern?
+   tarball "pkg-config"
+   "*/src/Makevars*" "*/src/configure*" "*/configure*"))
+
 (define (description->package repository meta)
   "Return the `package' s-expression for an R package published on REPOSITORY
 from the alist META, which was derived from the R package's DESCRIPTION file."
@@ -278,11 +285,12 @@ from the alist META, which was derived from the R package's DESCRIPTION file."
         (build-system r-build-system)
         ,@(maybe-inputs sysdepends)
         ,@(maybe-inputs (map guix-name propagate) 'propagated-inputs)
-        ,@(if (needs-fortran? tarball)
-              `((native-inputs (,'quasiquote
-                                ,(list "gfortran"
-                                       (list 'unquote 'gfortran)))))
-              '())
+        ,@(maybe-inputs
+           `(,@(if (needs-fortran? tarball)
+                   '("gfortran") '())
+             ,@(if (needs-pkg-config? tarball)
+                   '("pkg-config") '()))
+           'native-inputs)
         (home-page ,(if (string-null? home-page)
                         (string-append base-url name)
                         home-page))
-- 
2.12.2

bug#26373: [PATCH 5/6] import cran: Ensure substring indices are valid.

[Ricardo Wurmus]

* guix/import/cran.scm (package->upstream-name): Check that "start" and "end"
are valid before using them as substring indices.
---
 guix/import/cran.scm | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index 423835637..557d694ad 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -384,9 +384,10 @@ dependencies."
              ((or (? string? url) (url _ ...))
               (let ((end   (string-rindex url #\_))
                     (start (string-rindex url #\/)))
-                ;; The URL ends on
-                ;; (string-append "/" name "_" version ".tar.gz")
-                (substring url (+ start 1) end)))
+                (and start end
+                     ;; The URL ends on
+                     ;; (string-append "/" name "_" version ".tar.gz")
+                     (substring url (+ start 1) end))))
              (_ #f)))
           (_ #f)))))
 
-- 
2.12.2

bug#26373: [PATCH 6/6] import cran: Skip updating when meta data cannot be downloaded.

[Ricardo Wurmus]

* gnu/packages/bioinformatics.scm (latest-cran-release,
latest-bioconductor-release): Abort early when meta data cannot be downloaded.
---
 guix/import/cran.scm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index 557d694ad..fc7a1ed84 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -398,7 +398,8 @@ dependencies."
     (package->upstream-name package))
 
   (define meta
-    (fetch-description 'cran upstream-name))
+    (false-if-exception
+     (fetch-description 'cran upstream-name)))
 
   (and meta
        (let ((version (assoc-ref meta "Version")))
@@ -415,7 +416,8 @@ dependencies."
     (package->upstream-name package))
 
   (define meta
-    (fetch-description 'bioconductor upstream-name))
+    (false-if-exception
+     (fetch-description 'bioconductor upstream-name)))
 
   (and meta
        (let ((version (assoc-ref meta "Version")))
-- 
2.12.2

bug#26373: [PATCH 1/6] import cran: Exclude experiment packages in predicate "bioconductor-package?".

[Ludovic Courtès]

Ricardo Wurmus <rekado@elephly.net> skribis:

> * guix/import/cran.scm (bioconductor-package?): Exclude experiment packages,
> because they cannot be updated with the default bioconductor updater.

OK.

bug#26373: [PATCH 2/6] import cran: Add predicate for Bioconductor experiment packages.

[Ludovic Courtès]

Ricardo Wurmus <rekado@elephly.net> skribis:

> * guix/import/cran.scm (bioconductor-experiment-package?): New variable.

It seems to be a private and unused procedure.  Is it really needed?
:-)

Thanks,
Ludo’.

bug#26373: [PATCH 3/6] import cran: Refactor "needs-zlib?".

[Ludovic Courtès]

Ricardo Wurmus <rekado@elephly.net> skribis:

> * guix/import/cran.scm (tarball-files-match-pattern?): New procedure.
> (needs-zlib?): Implement in terms of "tarball-files-match-pattern?".

LGTM!

bug#26373: [PATCH 4/6] import cran: Check if pkg-config is needed.

[Ludovic Courtès]

Ricardo Wurmus <rekado@elephly.net> skribis:

> * guix/import/cran.scm (needs-pkg-config?): New procedure.
> (description->package): Use it.

LGTM!

bug#26373: [PATCH 5/6] import cran: Ensure substring indices are valid.

[Ludovic Courtès]

Ricardo Wurmus <rekado@elephly.net> skribis:

> * guix/import/cran.scm (package->upstream-name): Check that "start" and "end"
> are valid before using them as substring indices.

LGTM!

bug#26373: [PATCH 6/6] import cran: Skip updating when meta data cannot be downloaded.

[Ludovic Courtès]

Ricardo Wurmus <rekado@elephly.net> skribis:

> * gnu/packages/bioinformatics.scm (latest-cran-release,
> latest-bioconductor-release): Abort early when meta data cannot be downloaded.
> ---
>  guix/import/cran.scm | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/guix/import/cran.scm b/guix/import/cran.scm
> index 557d694ad..fc7a1ed84 100644
> --- a/guix/import/cran.scm
> +++ b/guix/import/cran.scm
> @@ -398,7 +398,8 @@ dependencies."
>      (package->upstream-name package))
>  
>    (define meta
> -    (fetch-description 'cran upstream-name))
> +    (false-if-exception
> +     (fetch-description 'cran upstream-name)))

I would prefer catching only the relevant exception.  So I suppose
something like:

  (guard (c ((http-get-error? c)
             (if (= 404 (http-get-error-code c))
                 #f
                 (raise c))))
    (fetch-description 'cran upstream-name))

However I see that ‘fetch-description’ already does that, so what
exceptions are we protecting against?

Thanks,
Ludo’.

bug#26373: [PATCH 2/6] import cran: Add predicate for Bioconductor experiment packages.

[Ricardo Wurmus]

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

> Ricardo Wurmus <rekado@elephly.net> skribis:
>
>> * guix/import/cran.scm (bioconductor-experiment-package?): New variable.
>
> It seems to be a private and unused procedure.  Is it really needed?
> :-)

Oh, it wasn’t supposed to be private.  There are three kinds of
bioconductor packages: the normal ones, experiment packages, and
annotation packages.

The latter two cannot be updated the usual way, so they are excluded
from “bioconductor-package?”, but I thought that one may still want to
be able to distinguish between them.  It’s true, though, that there’s
nothing that can use “bioconductor-experiment-package?” yet, but it’s
possible to add an updater for just the experiment packages.  (They use
a different SVN repository.)

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net

bug#26373: [PATCH 6/6] import cran: Skip updating when meta data cannot be downloaded.

[Ricardo Wurmus]

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

> Ricardo Wurmus <rekado@elephly.net> skribis:
>
>> * gnu/packages/bioinformatics.scm (latest-cran-release,
>> latest-bioconductor-release): Abort early when meta data cannot be downloaded.
>> ---
>>  guix/import/cran.scm | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/guix/import/cran.scm b/guix/import/cran.scm
>> index 557d694ad..fc7a1ed84 100644
>> --- a/guix/import/cran.scm
>> +++ b/guix/import/cran.scm
>> @@ -398,7 +398,8 @@ dependencies."
>>      (package->upstream-name package))
>>
>>    (define meta
>> -    (fetch-description 'cran upstream-name))
>> +    (false-if-exception
>> +     (fetch-description 'cran upstream-name)))
>
> I would prefer catching only the relevant exception.  So I suppose
> something like:
>
>   (guard (c ((http-get-error? c)
>              (if (= 404 (http-get-error-code c))
>                  #f
>                  (raise c))))
>     (fetch-description 'cran upstream-name))
>
> However I see that ‘fetch-description’ already does that, so what
> exceptions are we protecting against?

I don’t know what the desired behaviour here is.  When updating
packages, I think it’s good to keep going.  If there’s an error
downloading the package meta data I want “meta” to be “#f”, which leads
to skipping the update.

Without turning errors to “#f” I wasn’t able to just update all packages
with “guix refresh -t cran,bioconductor -u”.  That said, I don’t like
this.  I feel that it would be better to add a “--keep-going” option for
“refresh” and implement it in the shared updater code rather than here.

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net

bug#26373: [PATCH 2/6] import cran: Add predicate for Bioconductor experiment packages.

[Ludovic Courtès]

Ricardo Wurmus <rekado@elephly.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Ricardo Wurmus <rekado@elephly.net> skribis:
>>
>>> * guix/import/cran.scm (bioconductor-experiment-package?): New variable.
>>
>> It seems to be a private and unused procedure.  Is it really needed?
>> :-)
>
> Oh, it wasn’t supposed to be private.  There are three kinds of
> bioconductor packages: the normal ones, experiment packages, and
> annotation packages.
>
> The latter two cannot be updated the usual way, so they are excluded
> from “bioconductor-package?”, but I thought that one may still want to
> be able to distinguish between them.  It’s true, though, that there’s
> nothing that can use “bioconductor-experiment-package?” yet, but it’s
> possible to add an updater for just the experiment packages.  (They use
> a different SVN repository.)

OK, I see.  Make sure to export it, then!  :-)

Thanks for the explanation,
Ludo’.

bug#26373: [PATCH 6/6] import cran: Skip updating when meta data cannot be downloaded.

[Ludovic Courtès]

Ricardo Wurmus <rekado@elephly.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Ricardo Wurmus <rekado@elephly.net> skribis:
>>
>>> * gnu/packages/bioinformatics.scm (latest-cran-release,
>>> latest-bioconductor-release): Abort early when meta data cannot be downloaded.
>>> ---
>>>  guix/import/cran.scm | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/guix/import/cran.scm b/guix/import/cran.scm
>>> index 557d694ad..fc7a1ed84 100644
>>> --- a/guix/import/cran.scm
>>> +++ b/guix/import/cran.scm
>>> @@ -398,7 +398,8 @@ dependencies."
>>>      (package->upstream-name package))
>>>
>>>    (define meta
>>> -    (fetch-description 'cran upstream-name))
>>> +    (false-if-exception
>>> +     (fetch-description 'cran upstream-name)))
>>
>> I would prefer catching only the relevant exception.  So I suppose
>> something like:
>>
>>   (guard (c ((http-get-error? c)
>>              (if (= 404 (http-get-error-code c))
>>                  #f
>>                  (raise c))))
>>     (fetch-description 'cran upstream-name))
>>
>> However I see that ‘fetch-description’ already does that, so what
>> exceptions are we protecting against?
>
> I don’t know what the desired behaviour here is.  When updating
> packages, I think it’s good to keep going.  If there’s an error
> downloading the package meta data I want “meta” to be “#f”, which leads
> to skipping the update.
>
> Without turning errors to “#f” I wasn’t able to just update all packages
> with “guix refresh -t cran,bioconductor -u”.

What was the exception?

I think a good approach is to catch precisely the kind of error that we
don’t want to see.  ‘false-if-exception’ catches everything and could
thus hide genuine errors/bugs (including unbound variables and similar),
which sounds undesirable.

WDYT?

Ludo’.

bug#26373: [PATCH 6/6] import cran: Skip updating when meta data cannot be downloaded.

[Ricardo Wurmus]

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

> Ricardo Wurmus <rekado@elephly.net> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>>> Ricardo Wurmus <rekado@elephly.net> skribis:
>>>
>>>> * gnu/packages/bioinformatics.scm (latest-cran-release,
>>>> latest-bioconductor-release): Abort early when meta data cannot be downloaded.
>>>> ---
>>>>  guix/import/cran.scm | 6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/guix/import/cran.scm b/guix/import/cran.scm
>>>> index 557d694ad..fc7a1ed84 100644
>>>> --- a/guix/import/cran.scm
>>>> +++ b/guix/import/cran.scm
>>>> @@ -398,7 +398,8 @@ dependencies."
>>>>      (package->upstream-name package))
>>>>
>>>>    (define meta
>>>> -    (fetch-description 'cran upstream-name))
>>>> +    (false-if-exception
>>>> +     (fetch-description 'cran upstream-name)))
>>>
>>> I would prefer catching only the relevant exception.  So I suppose
>>> something like:
>>>
>>>   (guard (c ((http-get-error? c)
>>>              (if (= 404 (http-get-error-code c))
>>>                  #f
>>>                  (raise c))))
>>>     (fetch-description 'cran upstream-name))
>>>
>>> However I see that ‘fetch-description’ already does that, so what
>>> exceptions are we protecting against?
>>
>> I don’t know what the desired behaviour here is.  When updating
>> packages, I think it’s good to keep going.  If there’s an error
>> downloading the package meta data I want “meta” to be “#f”, which leads
>> to skipping the update.
>>
>> Without turning errors to “#f” I wasn’t able to just update all packages
>> with “guix refresh -t cran,bioconductor -u”.
>
> What was the exception?
>
> I think a good approach is to catch precisely the kind of error that we
> don’t want to see.  ‘false-if-exception’ catches everything and could
> thus hide genuine errors/bugs (including unbound variables and similar),
> which sounds undesirable.
>
> WDYT?

I don’t remember why I wanted this, so I didn’t push this patch.  I
agree that it’s much better to catch errors precisely.

Thanks for the review!  I pushed all the other patches (with the
exception of one that Mathieu had already implemented independently) to
master.

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net