Subject: Re: bug#60838: [PATCH 0/8] Add datasette and python-sqlite-utils.
From: Maxim Cournoyer <[email-masked]>
To: Felix Gruber <[email-masked]>
Cc: [email-masked]
Date: Wed, 29 Mar 2023 13:22:11 -0400
List-Id: <debbugs-submit.debbugs.gnu.org>

Hi Felix!

Thanks for the rework!  I'm applied it locally, then looked at it more
closely; I ended up with the following changes, which I'll comment
inline below:

--8<---------------cut here---------------start------------->8---
5 files changed, 105 insertions(+), 116 deletions(-)
gnu/packages/check.scm         |   7 +++----
gnu/packages/databases.scm     | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------
gnu/packages/python-crypto.scm |  17 ++++++-----------
gnu/packages/python-web.scm    |  56 +++++++++++++++++++++++++++++-------------------------
gnu/packages/python-xyz.scm    |  16 ++++++----------

modified   gnu/packages/check.scm
@@ -1322,15 +1322,14 @@ (define-public python-pytest-7.1
          "0f8c31v5r2kgjixvy267n0nhc4xsy65g3n9lz1i1377z5pn5ydjg"))))
     (arguments
      (substitute-keyword-arguments (package-arguments python-pytest)
-      ((#:phases phases #~%standard-phases)
+       ((#:phases phases #~%standard-phases)
         #~(modify-phases #$phases
             (add-before 'build 'pretend-version
               (lambda _
                 (setenv "SETUPTOOLS_SCM_PRETEND_VERSION" #$version)))))))
     (propagated-inputs
-      (modify-inputs (package-propagated-inputs python-pytest)
-                     (replace "python-pluggy" python-pluggy-next)))
-    ))
+     (modify-inputs (package-propagated-inputs python-pytest)
+       (replace "python-pluggy" python-pluggy-next)))))

Stylistic.  Lonely parens are sad :-).

 (define-public python-pytest-bootstrap
   (package
modified   gnu/packages/databases.scm
@@ -3586,7 +3586,7 @@ (define-public python-sqlite-utils
     (name "python-sqlite-utils")
     (version "3.30")
     (source (origin
-              (method git-fetch)  ; for tests
+              (method git-fetch)        ;for tests

Stylistic; I do not use a space between the ';' and the comment for
inline comments.
               
               (uri (git-reference
                     (url "https://github.com/simonw/sqlite-utils")
                     (commit version)))
@@ -3595,7 +3595,17 @@ (define-public python-sqlite-utils
                (base32
                 "1a58syvh5jp40vi5libsxkqy99z75kj4ckxqmylbhd342ppfy1wp"))))
     (build-system pyproject-build-system)
-    (propagated-inputs (list python-click python-click-default-group-wheel
+    (arguments
+     (list #:phases #~(modify-phases %standard-phases
+                        (add-after 'unpack 'relax-requirements
+                          (lambda _
+                            (substitute* "setup.py"
+                              ;; This is a variant designed to have a binary
+                              ;; wheel made available on PyPI, which is not a
+                              ;; concern to Guix.
+                              (("click-default-group-wheel")
+                               "click-default-group")))))))

I ended up not packaging the click-default-group-wheel variant and used
the our existing python-click-default-group package instead, since the
variant probably caters to Windows users installing from PyPI.

+    (propagated-inputs (list python-click python-click-default-group
                              python-dateutil python-sqlite-fts4
                              python-tabulate))
     (native-inputs (list python-pytest))
@@ -3604,9 +3614,7 @@ (define-public python-sqlite-utils
      "CLI tool and Python utility functions for manipulating SQLite databases")
     (description
      "This package provides a CLI tool and Python utility functions for
-manipulating SQLite databases.
-
-It's main features are:
+manipulating SQLite databases.  It's main features are:
 @itemize
 @item
 Pipe JSON (or CSV or TSV) directly into a new SQLite database file,
@@ -5091,7 +5099,7 @@ (define-public datasette
     (name "datasette")
     (version "0.64.2")
     (source (origin
-              (method git-fetch)  ; for tests
+              (method git-fetch)        ;for tests
               (uri (git-reference
                     (url "https://github.com/simonw/datasette")
                     (commit version)))
@@ -5103,40 +5111,25 @@ (define-public datasette
     (arguments
      (list
       #:test-flags
-      ;; disable failing tests
-      '(list
-         "--deselect"
-         "tests/test_api.py::test_database_page_for_database_with_dot_in_name"
-         "--deselect"
-         "tests/test_api.py::test_row_strange_table_name"
-         "--deselect"
-         "tests/test_api.py::test_database_with_space_in_name"
-         "--deselect"
-         "tests/test_api.py::test_tilde_encoded_database_names[fo%o]"
-         "--deselect"
-         "tests/test_api.py::test_tilde_encoded_database_names[f~/c.d]"
-         "--deselect"
-         "tests/test_cli.py::test_weird_database_names"
-         "--deselect"
-         "tests/test_html.py::test_css_classes_on_body[/fixtures/table~2Fwith~2Fslashes~2Ecsv-expected_classes5]"
-         "--deselect"
-         "tests/test_html.py::test_templates_considered[/fixtures/table~2Fwith~2Fslashes~2Ecsv-table-fixtures-tablewithslashescsv-fa7563.html, *table.html]"
-         "--deselect"
-         "tests/test_html.py::test_row_html_compound_primary_key[/fixtures/compound_primary_key/a~2Fb,~2Ec~2Dd-expected1]"
-         "--deselect"
-         "tests/test_html.py::test_edit_sql_link_on_canned_queries[/fixtures/~F0~9D~90~9C~F0~9D~90~A2~F0~9D~90~AD~F0~9D~90~A2~F0~9D~90~9E~F0~9D~90~AC-/fixtures?sql=select+id%2C+name+from+facet_cities+order+by+id+limit+1%3B]"
-         "--deselect"
-         "tests/test_html.py::test_alternate_url_json[/fixtures/table~2Fwith~2Fslashes~2Ecsv-http://localhost/fixtures/table~2Fwith~2Fslashes~2Ecsv.json]"
-         "--deselect"
-         "tests/test_table_api.py::test_table_with_slashes_in_name"
-         "--deselect"
-         "tests/test_table_api.py::test_searchable[/fixtures/searchable.json?_search=te*+AND+do*&_searchmode=raw-expected_rows3]"
-         "--deselect"
-         "tests/test_table_api.py::test_searchmode[table_metadata1-_search=te*+AND+do*-expected_rows1]"
-         "--deselect"
-         "tests/test_table_api.py::test_searchmode[table_metadata2-_search=te*+AND+do*&_searchmode=raw-expected_rows2]"
-         "--deselect"
-         "tests/test_table_api.py::test_custom_query_with_unicode_characters")
+      ;; There are multiple unexplained test failures (see:
+      ;; https://github.com/simonw/datasette/issues/2048).
+      #~(list "-k" (string-append
+                    "not (test_database_page_for_database_with_dot_in_name"
+                    " or test_row_strange_table_name"
+                    " or test_database_with_space_in_name"
+                    " or test_tilde_encoded_database_names"
+                    " or test_weird_database_names"
+                    " or test_css_classes_on_body"
+                    " or test_templates_considered"
+                    " or test_row_html_compound_primary_key"
+                    " or test_edit_sql_link_on_canned_queries"
+                    " or test_alternate_url_json"
+                    " or test_table_with_slashes_in_name"
+                    " or test_searchable"
+                    " or test_custom_query_with_unicode_characters"
+                    " or test_searchmode)")

I had two failures in the last suite; I reformatted for compactness nad
readability.

+              "-n" (number->string (parallel-job-count))

Since python-xdist is a native-input, we can make use of it (run tests i
parallel)

+              "-m" "not serial")        ;cannot run in parallel

I found that bit in the github workflow file of the project, necessary
when running the tests in parallel.

       #:phases
       #~(modify-phases %standard-phases
           (add-after 'unpack 'relax-requirements
@@ -5146,31 +5139,33 @@ (define-public datasette
               (substitute* "setup.py"
                 (("(black)==[0-9\\.]+" _ package)
                  package)))))))
-    (propagated-inputs (list python-aiofiles
-                             python-asgi-csrf
-                             python-asgiref
-                             python-click
-                             python-click-default-group-wheel
-                             python-httpx
-                             python-hupper
-                             python-itsdangerous
-                             python-janus
-                             python-jinja2
-                             python-mergedeep
-                             python-pint
-                             python-pluggy-next
-                             python-pyyaml
-                             python-uvicorn))
-    (native-inputs (list python-beautifulsoup4
-                         python-black
-                         python-cogapp
-                         python-pytest-7.1
-                         python-pytest-asyncio
-                         python-pytest-runner
-                         python-pytest-timeout
-                         python-pytest-xdist-next
-                         python-setuptools
-                         python-trustme-next))
+                 package)
+                (("click-default-group-wheel")
+                 "click-default-group")))))))

Same fix as earlier.

+    (propagated-inputs
+     (list python-aiofiles
+           python-asgi-csrf
+           python-asgiref
+           python-click
+           python-click-default-group
+           python-httpx
+           python-hupper
+           python-itsdangerous
+           python-janus
+           python-jinja2
+           python-mergedeep
+           python-pint
+           python-pluggy-next
+           python-pyyaml
+           python-uvicorn))
+    (native-inputs
+     (list python-beautifulsoup4
+           python-black
+           python-cogapp
+           python-pytest-7.1
+           python-pytest-asyncio
+           python-pytest-runner
+           python-pytest-timeout
+           python-pytest-xdist-next
+           python-setuptools
+           python-trustme))

Stylistic changes.

     (home-page "https://datasette.io/")
     (synopsis "Multi-tool for exploring and publishing data")
     (description "Datasette is a tool for exploring and publishing data.
modified   gnu/packages/python-crypto.scm
@@ -1503,21 +1503,14 @@ (define-public python-secretstorage
 (define-public python-trustme
   (package
     (name "python-trustme")
-    (version "0.6.0")
+    (version "0.9.0")
     (source
      (origin
        (method url-fetch)
        (uri (pypi-uri "trustme" version))
        (sha256
-        (base32 "0v3vr5z6apnfmklf07m45kv5kaqvm6hxrkaqywch57bjd2siiywx"))))
-    (build-system python-build-system)
-    (arguments
-     `(#:phases
-       (modify-phases %standard-phases
-         (replace 'check
-           (lambda* (#:key inputs outputs #:allow-other-keys)
-             (add-installed-pythonpath inputs outputs)
-             (invoke "pytest" "-vv"))))))
+        (base32 "0v2qzszmyazfgc1snicdr4b4qdajpjd4pbinpgrn9vfff0yv41sy"))))
+    (build-system pyproject-build-system)
     (native-inputs
      (list python-more-itertools
            python-pyopenssl
@@ -1526,7 +1519,9 @@ (define-public python-trustme
            python-service-identity
            python-zipp))
     (propagated-inputs
-     (list python-cryptography))
+     (list python-cryptography
+           python-idna
+           python-ipaddress))
     (home-page "https://github.com/python-trio/trustme")
     (synopsis "Fake a certificate authority for tests")
     (description

I've chosen to bump our main python-trustme package to its latest
version instead of introducing a 'python-trustme-next' variant, since
all its dependents rebuilt OK with it.  It's best to avoid variants
unless there's no other option.

modified   gnu/packages/python-web.scm
@@ -575,18 +575,30 @@ (define-public python-asgi-csrf
     (name "python-asgi-csrf")
     (version "0.9")
     (source (origin
-              (method url-fetch)
-              (uri (pypi-uri "asgi-csrf" version))
+              (method git-fetch)        ;for tests
+              (uri (git-reference
+                    (url "https://github.com/simonw/asgi-csrf")
+                    (commit version)))
+              (file-name (git-file-name name version))

I switched to git because otherwise the test suite ran 0 test
(successfully, ah!).

               (sha256
                (base32
-                "06klgxfxzjfkyjky3rkvmf2r07r7r2my53qq7g9qy6mcmvfkp7bf"))))
-    (build-system python-build-system)
+                "1j134mjh0ff61rvkm3q67m463j1bhyxc9dwsdany3scnd4vsqqws"))))
+    (build-system pyproject-build-system)
+    (arguments
+     (list #:test-flags
+           ;; Provide a null config to avoid the extraneous dependency on
+           ;; python-pytest-coverage.
+           #~(list "-c" "/dev/null"

This trick was used to avoid the --cov* options added to pytest options,
to avoid requiring the coverage tool.

+                   ;; Disable two failing tests (see:
+                   ;; https://github.com/simonw/asgi-csrf/issues/24).
+                   "-k" (string-append
+                         "not (test_multipart "
+                         "or test_multipart_failure_wrong_token)"))))

I encountered two test failures, so I reported them upstream and skipped
them in the package.
     
     (propagated-inputs (list python-itsdangerous python-multipart))
     (native-inputs (list python-asgi-lifespan
                          python-httpx
                          python-pytest
                          python-pytest-asyncio
-                         python-pytest-cov
                          python-starlette))
     (home-page "https://github.com/simonw/asgi-csrf")
     (synopsis "ASGI middleware for protecting against CSRF attacks")
@@ -602,7 +614,7 @@ (define-public python-asgi-lifespan
     (name "python-asgi-lifespan")
     (version "1.0.1")
     (source (origin
-              (method git-fetch)  ; for tests
+              (method git-fetch)        ;for tests
               (uri (git-reference
                     (url "https://github.com/florimondmanca/asgi-lifespan")
                     (commit version)))
@@ -613,28 +625,20 @@ (define-public python-asgi-lifespan
     (build-system pyproject-build-system)
     (arguments
      (list
-       #:test-flags
-       ;; disable failing tests
-       '(list "-k"
-              (string-append
-                "not test_lifespan_manager[asyncio-None-ValueError-None]"
-                " and not test_lifespan_manager[asyncio-ValueError-None-None]"
-                " and not test_lifespan_manager[asyncio-ValueError-ValueError-None]"
-                " and not test_lifespan_manager[trio-None-ValueError-None]"
-                " and not test_lifespan_manager[trio-ValueError-None-None]"
-                " and not test_lifespan_manager[trio-ValueError-ValueError-None]"))
-       #:phases
-       '(modify-phases %standard-phases
-          (add-after 'unpack 'lower-coverage-requirement
-            ;; after disabling the failing tests, the coverage
-            ;; dropped slightly below 100%.
-            (lambda _
-              (substitute* "setup.cfg"
-                (("(--cov-fail-under=)[0-9]+" _ cov)
-                 (string-append cov "90"))))))))
+      #:test-flags
+      '(list "-c" "/dev/null"           ;ignore coverage-related options
+             "-k"
+             (string-append
+              ;; XXX: Some tests fail because of a 'lifespan.shutdown.failed'
+              ;; extra event, perhaps because our version of trio is older.

I left a comment explaining a reason why we may be seeing these
failures.  Our environment differs from upstream so I haven't reported
them there.  We'd need to update a bunch of asyncio things but these are
rather tied to the Pytest version, I think.

+              "not (test_lifespan_manager[asyncio-None-ValueError-None]"
+              " or test_lifespan_manager[asyncio-ValueError-None-None]"
+              " or test_lifespan_manager[asyncio-ValueError-ValueError-None]"
+              " or test_lifespan_manager[trio-None-ValueError-None]"
+              " or test_lifespan_manager[trio-ValueError-None-None]"
+              " or test_lifespan_manager[trio-ValueError-ValueError-None])"))))
     (native-inputs (list python-pytest
                          python-pytest-asyncio
-                         python-pytest-cov
                          python-pytest-trio
                          python-starlette))
     (propagated-inputs (list python-sniffio))
modified   gnu/packages/python-xyz.scm
@@ -31569,16 +31569,12 @@ (define-public python-cogapp
               (sha256
                (base32
                 "1c0xx3p3lzrlyqhmccyq9c50f8v9pqk2992gb4nl50h2yy1m3s8v"))))
-    (build-system python-build-system)
-    (arguments
-     '(#:tests? #f))
-    (home-page "http://nedbatchelder.com/code/cog")
-    (synopsis
-     "Content generator for executing Python snippets in source files")
-    (description
-     "Cog is a file generation tool. It lets you use pieces of Python
-code as generators in your source files to generate whatever text you
-need.")
+    (build-system pyproject-build-system)
+    (native-inputs (list python-pytest))
+    (home-page "https://nedbatchelder.com/code/cog")
+    (synopsis "Content generation tool that leverages Python")
+    (description "Cog is a file generation tool.  It allows using pieces of
+Python code as generators in your source files to generate arbitrary text.")
     (license license:expat)))

Here I've switched the build system to pyproject-build-system and added
python-pytest; the package had tests.  I've also used HTTPS for the home
page URL and reworded synopsis a bit.
--8<---------------cut here---------------end--------------->8---

With this, the changes were installed.  I hope my review comments were
insightful for your future contributions!

Thank you!

-- 
Maxim