Subject: Re: bug#53395: Fix pypi import for flake8-array-spacing

From: Vivien Kraus <[email-masked]>
Date: Tue, 25 Jan 2022 17:39:54 +0100

Here we go!

Ludovic Courtès <[email-masked]> writes:

> Yes, just one ‘display-hint’ call, using complete sentences and
> paragraphs.  The text should provide an explanation and more importantly
> a hint as to what can done, like “The generated package definition might
> be wrong; please double-check …”.

I made a new version for that.

>
> You can avoid @strong though, because it doesn’t do
> anything useful.

Okay.

>
>> +        (warning
>> +         (G_ "The project name does not appear verbatim in the pypi URI~%"))
>
> I’d make it:
>
>   "project name ~a does not appear verbatim in the PyPI URI~%"

Changed.

>
>>> Also, what about adding a unit test?
>
> [...]
>
>> +(test-equal "If the package goo is released as foo, the importer warns"
>> +  "warning: The project name does not appear verbatim in the pypi URI
>> +hint: The project name is: *goo*
>> +
>> +hint: The pypi URI is: *`https://example.com/foo-1.0.0.tar.gz'*
>> +
>> +hint: The pypi-uri declaration in the generated package might need to be changed.
>> +
>> +"
>> +  (call-with-output-string
>> +    (lambda (port)
>> +      (parameterize ((guix-warning-port port)
>> +                     (current-error-port port))
>> +        ;; Replace network resources with sample data.
>> +        (mock ((guix import utils) url-fetch
>> +               (lambda (url file-name)
>> +                 (match url
>
> These two tests are really integration tests: they exercise the whole
> shebang.  I was thinking about having a unit test of just
> ‘find-project-url’, which is less work (and maintenance :-)) and may be
> good enough.

Let’s do both! If you don’t want the integration tests, just delete them
(now or when maintenance becomes too demanding).

>
> Perhaps you can also change one of the existing python-foo tests to
> exercise this bit, for instance by making it “Foo”.

I added a test function that can be used by all integration tests. It’s
not perfect (there’s still a lot of code copied from test to test, and
I’m not innocent there too) but I don’t want to change the other tests
beyond trivial things.

Vivien

From 8792367617d288e584a703db38c82c749a067c26 Mon Sep 17 00:00:00 2001
From: Vivien Kraus <[email-masked]>
Date: Thu, 20 Jan 2022 20:11:56 +0100
Subject: [PATCH] pypi importer: Convert - to _ in pypi urls if needed.

* guix/import/pypi.scm (find-project-url): New function.
(make-pypi-sexp): Use find-project-url.
* tests/pypi.scm ("Package Foo has a correct pypi-uri of foo"): New test.
("If the package goo is released as foo, the importer warns"): New test.
---
 guix/import/pypi.scm |  30 ++--
 tests/pypi.scm       | 337 ++++++++++++++++++++++++++++++++++++-------
 2 files changed, 305 insertions(+), 62 deletions(-)

diff --git a/guix/import/pypi.scm b/guix/import/pypi.scm
index b4284f5c33..8245ed6348 100644
--- a/guix/import/pypi.scm
+++ b/guix/import/pypi.scm
@@ -41,6 +41,7 @@ (define-module (guix import pypi)
   #:use-module (guix memoization)
   #:use-module (guix diagnostics)
   #:use-module (guix i18n)
+  #:use-module ((guix ui) #:select (display-hint))
   #:use-module ((guix build utils)
                 #:select ((package-name->name+version
                            . hyphen-package-name->name+version)
@@ -59,6 +60,7 @@ (define-module (guix import pypi)
             specification->requirement-name
             guix-package->pypi-name
             pypi-recursive-import
+            find-project-url
             pypi->guix-package
             %pypi-updater))
 
@@ -418,6 +420,24 @@ (define process-requirements
     (values (map process-requirements dependencies)
             (concatenate dependencies))))
 
+(define (find-project-url name pypi-url)
+  "Try different project name substitution until the result is found in
+pypi-uri.  Downcase is required for \"uWSGI\", and
+underscores are required for flake8-array-spacing."
+  (or (find (cut string-contains pypi-url <>)
+            (list name
+                  (string-downcase name)
+                  (string-replace-substring name "-" "_")))
+      (begin
+        (warning
+         (G_ "project name ~a does not appear verbatim in the PyPI URI~%")
+         name)
+        (display-hint
+         (format #f (G_ "The PyPI URI is: @url{~a}.  You should review the
+pypi-uri declaration in the generated package. You may need to replace ~s with
+a substring of the PyPI URI that identifies the package.")  pypi-url name))
+name)))
+
 (define (make-pypi-sexp name version source-url wheel-url home-page synopsis
                         description license)
   "Return the `package' s-expression for a python package with the given NAME,
@@ -446,15 +466,7 @@ (define (maybe-upstream-name name)
                     (origin
                       (method url-fetch)
                       (uri (pypi-uri
-                             ;; PyPI URL are case sensitive, but sometimes
-                             ;; a project named using mixed case has a URL
-                             ;; using lower case, so we must work around this
-                             ;; inconsistency.  For actual examples, compare
-                             ;; the URLs of the "Deprecated" and "uWSGI" PyPI
-                             ;; packages.
-                             ,(if (string-contains source-url name)
-                                  name
-                                  (string-downcase name))
+                             ,(find-project-url name source-url)
                              version
                              ;; Some packages have been released as `.zip`
                              ;; instead of the more common `.tar.gz`. For
diff --git a/tests/pypi.scm b/tests/pypi.scm
index 1ea5f02643..b1647ac0a7 100644
--- a/tests/pypi.scm
+++ b/tests/pypi.scm
@@ -23,68 +23,57 @@ (define-module (test-pypi)
   #:use-module (guix import pypi)
   #:use-module (guix base32)
   #:use-module (guix memoization)
+  #:use-module (guix utils)
   #:use-module (gcrypt hash)
   #:use-module (guix tests)
   #:use-module (guix build-system python)
   #:use-module ((guix build utils) #:select (delete-file-recursively which mkdir-p))
+  #:use-module ((guix diagnostics) #:select (guix-warning-port))
+  #:use-module (json)
+  #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-64)
-  #:use-module (ice-9 match))
+  #:use-module (ice-9 match)
+  #:use-module (ice-9 optargs))
+
+(define* (foo-json #:key (name "foo") (name-in-url #f))
+  "Create a JSON description of an example pypi package, named @var{name},
+optionally using a different @var{name in its URL}."
+  (scm->json-string
+   `((info
+      . ((version . "1.0.0")
+         (name . ,name)
+         (license . "GNU LGPL")
+         (summary . "summary")
+         (home_page . "http://example.com")
+         (classifiers . #())
+         (download_url . "")))
+     (urls . #())
+     (releases
+      . ((1.0.0
+          . #(((url . ,(format #f "https://example.com/~a-1.0.0.egg"
+                               (or name-in-url name)))
+               (packagetype . "bdist_egg"))
+              ((url . ,(format #f "https://example.com/~a-1.0.0.tar.gz"
+                               (or name-in-url name)))
+               (packagetype . "sdist"))
+              ((url . ,(format #f "https://example.com/~a-1.0.0-py2.py3-none-any.whl"
+                               (or name-in-url name)))
+               (packagetype . "bdist_wheel")))))))))
 
 (define test-json-1
-  "{
-  \"info\": {
-    \"version\": \"1.0.0\",
-    \"name\": \"foo\",
-    \"license\": \"GNU LGPL\",
-    \"summary\": \"summary\",
-    \"home_page\": \"http://example.com\",
-    \"classifiers\": [],
-    \"download_url\": \"\"
-  },
-  \"urls\": [],
-  \"releases\": {
-    \"1.0.0\": [
-      {
-        \"url\": \"https://example.com/foo-1.0.0.egg\",
-        \"packagetype\": \"bdist_egg\"
-      }, {
-        \"url\": \"https://example.com/foo-1.0.0.tar.gz\",
-        \"packagetype\": \"sdist\"
-      }, {
-        \"url\": \"https://example.com/foo-1.0.0-py2.py3-none-any.whl\",
-        \"packagetype\": \"bdist_wheel\"
-      }
-    ]
-  }
-}")
+  (foo-json))
 
 (define test-json-2
-  "{
-  \"info\": {
-    \"version\": \"1.0.0\",
-    \"name\": \"foo-99\",
-    \"license\": \"GNU LGPL\",
-    \"summary\": \"summary\",
-    \"home_page\": \"http://example.com\",
-    \"classifiers\": [],
-    \"download_url\": \"\"
-  },
-  \"urls\": [],
-  \"releases\": {
-    \"1.0.0\": [
-      {
-        \"url\": \"https://example.com/foo-99-1.0.0.egg\",
-        \"packagetype\": \"bdist_egg\"
-      }, {
-        \"url\": \"https://example.com/foo-99-1.0.0.tar.gz\",
-        \"packagetype\": \"sdist\"
-      }, {
-        \"url\": \"https://example.com/foo-99-1.0.0-py2.py3-none-any.whl\",
-        \"packagetype\": \"bdist_wheel\"
-      }
-    ]
-  }
-}")
+  (foo-json #:name "foo-99"))
+
+(define test-json-3
+  (foo-json #:name "Foo" #:name-in-url "foo"))
+
+(define test-json-4
+  (foo-json #:name "foo-bar" #:name-in-url "foo_bar"))
+
+(define test-json-5
+  (foo-json #:name "foo" #:name-in-url "goo"))
 
 (define test-source-hash
   "")
@@ -211,6 +200,30 @@ (define test-metadata-with-extras-jedi "\
          call-with-input-string)
         (parse-wheel-metadata test-metadata-with-extras-jedi)))
 
+(test-equal "find-project-url, with numpy"
+  "numpy"
+  (find-project-url
+   "numpy"
+   "https://files.pythonhosted.org/packages/0a/c8/a62767a6b374a0dfb02d2a0456e5f56a372cdd1689dbc6ffb6bf1ddedbc0/numpy-1.22.1.zip"))
+
+(test-equal "find-project-url, uWSGI"
+  "uwsgi"
+  (find-project-url
+   "uWSGI"
+   "https://files.pythonhosted.org/packages/24/fd/93851e4a076719199868d4c918cc93a52742e68370188c1c570a6e42a54f/uwsgi-2.0.20.tar.gz"))
+
+(test-equal "find-project-url, flake8-array-spacing"
+  "flake8_array_spacing"
+  (find-project-url
+   "flake8-array-spacing"
+   "https://files.pythonhosted.org/packages/a4/21/ff29b901128b681b7de7a2787b3aeb3e1f3cba4a8c0cffa9712cbff016bc/flake8_array_spacing-0.2.0.tar.gz"))
+
+(test-equal "find-project-url, foo/goo"
+  "foo"
+  (find-project-url
+   "foo"
+   "https://files.pythonhosted.org/packages/f0/f00/goo-0.0.0.tar.gz"))
+
 (test-assert "pypi->guix-package, no wheel"
   ;; Replace network resources with sample data.
     (mock ((guix import utils) url-fetch
@@ -428,4 +441,222 @@ (define test-metadata-with-extras-jedi "\
                 (x
                  (pk 'fail x #f))))))
 
+(test-equal "pypi->guix-package, package name is capitalized but not in download URI (like uWSGI)"
+  ;; Checking that the importer doesn’t emit a warning
+  ""
+  (call-with-output-string
+    (lambda (port)
+      (parameterize ((guix-warning-port port))
+        ;; Replace network resources with sample data.
+        (mock ((guix import utils) url-fetch
+               (lambda (url file-name)
+                 (match url
+                   ;; Package Foo has /foo- download URLs
+                   ("https://example.com/foo-1.0.0.tar.gz"
+                    (begin
+                      (mkdir-p "foo-1.0.0/foo.egg-info")
+                      (with-output-to-file "foo-1.0.0/foo.egg-info/requires.txt"
+                        (lambda ()
+                          (display test-requires.txt)))
+                      (parameterize ((current-output-port (%make-void-port "rw+")))
+                        (system* "tar" "czvf" file-name "foo-1.0.0/"))
+                      (delete-file-recursively "foo-1.0.0")))
+                   ("https://example.com/foo-1.0.0-py2.py3-none-any.whl"
+                    (begin
+                      (mkdir "foo-1.0.0.dist-info")
+                      (with-output-to-file "foo-1.0.0.dist-info/METADATA"
+                        (lambda ()
+                          (display test-metadata)))
+                      (let ((zip-file (string-append file-name ".zip")))
+                        ;; zip always adds a "zip" extension to the file it creates,
+                        ;; so we need to rename it.
+                        (system* "zip" "-q" zip-file "foo-1.0.0.dist-info/METADATA")
+                        (rename-file zip-file file-name))
+                      (delete-file-recursively "foo-1.0.0.dist-info")))
+                   (_ (error "Unexpected URL: " url)))))
+              (mock ((guix http-client) http-fetch
+                     (lambda (url . rest)
+                       (match url
+                         ;; Note that the project name is Foo, so the project
+                         ;; URL queried by the importer is /pypi/Foo/json
+                         ("https://pypi.org/pypi/Foo/json"
+                          (values (open-input-string test-json-3)
+                                  (string-length test-json-3)))
+                         ("https://example.com/foo-1.0.0-py2.py3-none-any.whl" #f)
+                         (_ (error "Unexpected URL: " url)))))
+                    (match (pypi->guix-package "Foo")
+                      (('package
+                         ('name "python-foo") ;; The guix package name is
+                                              ;; computed from the pypi
+                                              ;; project name: Foo -> foo ->
+                                              ;; python-foo
+                         ('version "1.0.0")
+                         ('source ('origin
+                                    ('method 'url-fetch)
+                                    ;; If the importer were too simple, it
+                                    ;; would have put '(pypi-uri "Foo"
+                                    ;; 'version) here, but as indicated by
+                                    ;; test-json-4, the package releases are
+                                    ;; usually published with /foo- URLs, so
+                                    ;; we would get a 404 Not Found response
+                                    ;; when building the package.
+                                    ('uri ('pypi-uri "foo" 'version))
+                                    ('sha256
+                                     ('base32
+                                      (? string? hash)))))
+                         ('build-system 'python-build-system)
+                         ('propagated-inputs ('list 'python-bar 'python-baz))
+                         ('native-inputs ('list 'python-pytest))
+                         ('home-page "http://example.com")
+                         ('synopsis "summary")
+                         ('description "summary")
+                         ('license 'license:lgpl2.0))
+                       (string=? (bytevector->nix-base32-string
+                                  test-source-hash)
+                                 hash))
+                      (x
+                       (error "Failed: ~S" x)))))))))
+
+(test-equal "pypi->guix-package, package name is kebab-case but the download URI is snake_case (like flake8-array-spacing)"
+  ;; This test is similar to the previous one
+  ""
+  (call-with-output-string
+    (lambda (port)
+      (parameterize ((guix-warning-port port))
+        ;; Replace network resources with sample data.
+        (mock ((guix import utils) url-fetch
+               (lambda (url file-name)
+                 ;; Everything is foo_bar here:
+                 (match url
+                   ("https://example.com/foo_bar-1.0.0.tar.gz"
+                    (begin
+                      (mkdir-p "foo_bar-1.0.0/foo_bar.egg-info")
+                      (with-output-to-file "foo_bar-1.0.0/foo_bar.egg-info/requires.txt"
+                        (lambda ()
+                          (display test-requires.txt)))
+                      (parameterize ((current-output-port (%make-void-port "rw+")))
+                        (system* "tar" "czvf" file-name "foo_bar-1.0.0/"))
+                      (delete-file-recursively "foo_bar-1.0.0")))
+                   ("https://example.com/foo_bar-1.0.0-py2.py3-none-any.whl"
+                    (begin
+                      (mkdir "foo_bar-1.0.0.dist-info")
+                      (with-output-to-file "foo_bar-1.0.0.dist-info/METADATA"
+                        (lambda ()
+                          (display test-metadata)))
+                      (let ((zip-file (string-append file-name ".zip")))
+                        ;; zip always adds a "zip" extension to the file it creates,
+                        ;; so we need to rename it.
+                        (system* "zip" "-q" zip-file "foo_bar-1.0.0.dist-info/METADATA")
+                        (rename-file zip-file file-name))
+                      (delete-file-recursively "foo_bar-1.0.0.dist-info")))
+                   (_ (error "Unexpected URL: " url)))))
+              (mock ((guix http-client) http-fetch
+                     (lambda (url . rest)
+                       (match url
+                         ("https://pypi.org/pypi/foo-bar/json" ; (!)
+                          (values (open-input-string test-json-4)
+                                  (string-length test-json-4)))
+                         ("https://example.com/foo_bar-1.0.0-py2.py3-none-any.whl" #f)
+                         (_ (error "Unexpected URL: " url)))))
+                    (match (pypi->guix-package "foo-bar")
+                      (('package
+                         ('name "python-foo-bar")
+                         ('version "1.0.0")
+                         ('source ('origin
+                                    ('method 'url-fetch)
+                                    ('uri ('pypi-uri "foo_bar" 'version))
+                                    ('sha256
+                                     ('base32
+                                      (? string? hash)))))
+                         ('build-system 'python-build-system)
+                         ('propagated-inputs ('list 'python-bar 'python-baz))
+                         ('native-inputs ('list 'python-pytest))
+                         ('home-page "http://example.com")
+                         ('synopsis "summary")
+                         ('description "summary")
+                         ('license 'license:lgpl2.0))
+                       (string=? (bytevector->nix-base32-string
+                                  test-source-hash)
+                                 hash))
+                      (x
+                       (error "Failed: ~S" x)))))))))
+
+(test-equal "pypi->guix-package, package name is foo but the download URI uses goo"
+  ;; A warning should be emitted with a hint about the issue
+  "warning: project name foo does not appear verbatim in the PyPI URI
+hint: The PyPI URI is: `https://example.com/goo-1.0.0.tar.gz'.  You should review the pypi-uri
+declaration in the generated package.  You may need to replace \"foo\" with a substring of the PyPI
+URI that identifies the package.
+
+"
+  (call-with-output-string
+    (lambda (port)
+      (parameterize ((guix-warning-port port)
+                     (current-error-port port))
+        ;; Replace network resources with sample data.
+        (mock ((guix import utils) url-fetch
+               (lambda (url file-name)
+                 (match url
+                   ("https://example.com/goo-1.0.0.tar.gz"
+                    (begin
+                      (mkdir-p "goo-1.0.0/goo.egg-info/")
+                      (with-output-to-file "goo-1.0.0/goo.egg-info/requires.txt"
+                        (lambda ()
+                          (display "wrong data to make sure we're testing wheels ")))
+                      (parameterize ((current-output-port (%make-void-port "rw+")))
+                        (system* "tar" "czvf" file-name "goo-1.0.0/"))
+                      (delete-file-recursively "goo-1.0.0")
+                      (set! test-source-hash
+                            (call-with-input-file file-name port-sha256))))
+                   ("https://example.com/goo-1.0.0-py2.py3-none-any.whl"
+                    (begin
+                      (mkdir "goo-1.0.0.dist-info")
+                      (with-output-to-file "goo-1.0.0.dist-info/METADATA"
+                        (lambda ()
+                          (display test-metadata)))
+                      (let ((zip-file (string-append file-name ".zip")))
+                        ;; zip always adds a "zip" extension to the file it creates,
+                        ;; so we need to rename it.
+                        (system* "zip" "-q" zip-file "goo-1.0.0.dist-info/METADATA")
+                        (rename-file zip-file file-name))
+                      (delete-file-recursively "goo-1.0.0.dist-info")))
+                   (_ (error "Unexpected URL: " url)))))
+              (mock ((guix http-client) http-fetch
+                     (lambda (url . rest)
+                       (match url
+                         ("https://pypi.org/pypi/foo/json"
+                          (values (open-input-string test-json-5)
+                                  (string-length test-json-5)))
+                         ("https://example.com/goo-1.0.0-py2.py3-none-any.whl" #f)
+                         (_ (error "Unexpected URL: " url)))))
+                    ;; Not clearing the memoization cache here would mean returning the value
+                    ;; computed in the previous test.
+                    (invalidate-memoization! pypi->guix-package)
+                    (match (pypi->guix-package "foo")
+                      (('package
+                         ('name "python-foo")
+                         ('version "1.0.0")
+                         ('source ('origin
+                                    ('method 'url-fetch)
+                                    ;; The importer can’t guess that we should
+                                    ;; replace with '(pypi-uri "goo"
+                                    ;; 'version), so it defaults to "foo" with
+                                    ;; a warning.
+                                    ('uri ('pypi-uri "foo" 'version))
+                                    ('sha256
+                                     ('base32
+                                      (? string? hash)))))
+                         ('build-system 'python-build-system)
+                         ('propagated-inputs ('list 'python-bar 'python-baz))
+                         ('native-inputs ('list 'python-pytest))
+                         ('home-page "http://example.com")
+                         ('synopsis "summary")
+                         ('description "summary")
+                         ('license 'license:lgpl2.0))
+                       (string=? (bytevector->nix-base32-string
+                                  test-source-hash)
+                                 hash))
+                      (x
+                       (error "Failed: ~S" x)))))))))
+
 (test-end "pypi")
-- 
2.34.0