diff mbox series

[bug#45712,PATCHES] Improve Python package quality

Message ID 87czxs3jel.fsf_-_@gmail.com
State Accepted
Headers show
Series [bug#45712,PATCHES] Improve Python package quality | expand

Checks

Context Check Description
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue

Commit Message

Maxim Cournoyer Jan. 25, 2021, 7:29 p.m. UTC
Hi Lars-Dominik,

Lars-Dominik Braun <lars@6xq.net> writes:

> Adds a new phase validating usalibity of installed Python packages.
>
> * guix/build/python-build-system.scm (validate-script): Add script.
> (validate-loadable): New phase.
> (%standard-phases): Use it.
> * tests/builders.scm (make-python-dummy): Add test package generator.
> (check-build-{success,failure}): Add build helper functions.
> (python-dummy-*): Add test packages.
> ("python-build-system: &"): Add tests.

Attached is a small rework of your original patch.  I've made the Python
script standalone, which should make it easier to maintain.  I've also
refactored the tests somewhat and added your copyright information.

Is this OK with you?

Thanks!

Maxim

Comments

Lars-Dominik Braun Jan. 26, 2021, 8:39 a.m. UTC | #1
Hi Maxim,

> Attached is a small rework of your original patch.  I've made the Python
> script standalone, which should make it easier to maintain.  I've also
> refactored the tests somewhat and added your copyright information.
> Is this OK with you?
sure, no problem with the refactoring in gerenal, but I think you used
an old patchset. I sent a v2 to this issue, which had some changes to
the scripts and tests.

I can’t test your patch properly unfortunately, because `make check`
does not work on core-updates and trying to build any Python package on
core-updates triggers the requirements checker immediately: 

validating 'attrs' /gnu/store/hdjip92izsf9anfhd6ijgc9glvbi4dzv-python-attrs-bootstrap-19.3.0/lib/python3.9/site-packages
...checking requirements: ERROR: attrs==19.3.0 The 'attrs==19.3.0' distribution was not found and is required by the application”)

Maybe that’s an issue with Python 3.9?

Cheers,
Lars
Maxim Cournoyer Jan. 28, 2021, 3:40 p.m. UTC | #2
Hi Lars,

Lars-Dominik Braun <lars@6xq.net> writes:

> Hi Maxim,
>
>> Attached is a small rework of your original patch.  I've made the Python
>> script standalone, which should make it easier to maintain.  I've also
>> refactored the tests somewhat and added your copyright information.
>> Is this OK with you?
> sure, no problem with the refactoring in gerenal, but I think you used
> an old patchset. I sent a v2 to this issue, which had some changes to
> the scripts and tests.
>
> I can’t test your patch properly unfortunately, because `make check`
> does not work on core-updates and trying to build any Python package on
> core-updates triggers the requirements checker immediately: 
>
> validating 'attrs'
> /gnu/store/hdjip92izsf9anfhd6ijgc9glvbi4dzv-python-attrs-bootstrap-19.3.0/lib/python3.9/site-packages
> ...checking requirements: ERROR: attrs==19.3.0 The 'attrs==19.3.0'
> distribution was not found and is required by the application”)
>
> Maybe that’s an issue with Python 3.9?
>
> Cheers,
> Lars

Doesn't that mean that the attrs requirement should be lifted from the
bootstrap version? As its purpose is exactly this: breaking the cyclic
dependency with itself.

Maxim
Lars-Dominik Braun Jan. 28, 2021, 4:18 p.m. UTC | #3
Hi Maxim,

> Doesn't that mean that the attrs requirement should be lifted from the
> bootstrap version? As its purpose is exactly this: breaking the cyclic
> dependency with itself.
ah, now I see the problem, there’s a (add-installed-pythonpath inputs
outputs) missing and so it can’t find the installed packages. Maybe
we’re working on different branches? I applied your patch directly to
core-updates.

Cheers,
Lars
Maxim Cournoyer Jan. 29, 2021, 2:26 p.m. UTC | #4
Hi Lars,

Lars-Dominik Braun <lars@6xq.net> writes:

> Hi Maxim,
>
>> Doesn't that mean that the attrs requirement should be lifted from the
>> bootstrap version? As its purpose is exactly this: breaking the cyclic
>> dependency with itself.
> ah, now I see the problem, there’s a (add-installed-pythonpath inputs
> outputs) missing and so it can’t find the installed packages. Maybe
> we’re working on different branches? I applied your patch directly to
> core-updates.
>
> Cheers,
> Lars

Oh yes, sorry I had failed to mention it, this is on the
cu/farewell-to-pythonpath branch, which is the integration of
GUIX_PYTHONPATH and this work of yours.  I've included your v2 patches,
with light editing: fixed the script indentation (spot via flake8), I
removed the python2 tests as Python 2 is obsolete, and removed the space
between the (procedure)[area/conditional] part of the GNU changelog
commit messages.

Another note to help with review; when sending v2 patches, make sure the
title of your mail reply mentions [PATCH v2]; this helps to spot the
later versions in email threads.

The branch is shaping up nicely; I encourage you to try it.  If no major
problem is found with it, I'll merge it in core-updates soon.

Thank you,

Maxim
Lars-Dominik Braun Feb. 1, 2021, 7:20 a.m. UTC | #5
Hi Maxim,

> Oh yes, sorry I had failed to mention it, this is on the
> cu/farewell-to-pythonpath branch, which is the integration of
> GUIX_PYTHONPATH and this work of yours.  I've included your v2 patches,
> with light editing: fixed the script indentation (spot via flake8), I
> removed the python2 tests as Python 2 is obsolete, and removed the space
> between the (procedure)[area/conditional] part of the GNU changelog
> commit messages.
thanks!

> Another note to help with review; when sending v2 patches, make sure the
> title of your mail reply mentions [PATCH v2]; this helps to spot the
> later versions in email threads.
Sorry about that, will do next time.

> The branch is shaping up nicely; I encourage you to try it.  If no major
> problem is found with it, I'll merge it in core-updates soon.
I’ve been rebuilding packages on my list that use python-build-system
using this branch and quite a few fail, but in a random sample I found
none that fails due to this patch and also I imagine some issues are
already fixed on core-updates? Is there anything else I can do?

Cheers,
Lars
Maxim Cournoyer Feb. 1, 2021, 5:02 p.m. UTC | #6
Hi Lars,

Lars-Dominik Braun <lars@6xq.net> writes:

> Hi Maxim,
>
>> Oh yes, sorry I had failed to mention it, this is on the
>> cu/farewell-to-pythonpath branch, which is the integration of
>> GUIX_PYTHONPATH and this work of yours.  I've included your v2 patches,
>> with light editing: fixed the script indentation (spot via flake8), I
>> removed the python2 tests as Python 2 is obsolete, and removed the space
>> between the (procedure)[area/conditional] part of the GNU changelog
>> commit messages.
> thanks!
>
>> Another note to help with review; when sending v2 patches, make sure the
>> title of your mail reply mentions [PATCH v2]; this helps to spot the
>> later versions in email threads.
> Sorry about that, will do next time.
>
>> The branch is shaping up nicely; I encourage you to try it.  If no major
>> problem is found with it, I'll merge it in core-updates soon.
> I’ve been rebuilding packages on my list that use python-build-system
> using this branch and quite a few fail, but in a random sample I found
> none that fails due to this patch and also I imagine some issues are
> already fixed on core-updates? Is there anything else I can do?

I guess we can be bold and merge the branch to core-updates, and fix any
breakage that may occur there!

I've done so, the last commit of the series is
1b9186828867e77af1f2ee6741063424f8256398.

Let's make Python on Guix great again!

Thank you :-)

Closing,

Maxim
Hartmut Goebel Feb. 7, 2021, 4:59 p.m. UTC | #7
Hi Maxim,
>
> Attached is a small rework of your original patch.  I've made the Python
> script standalone, which should make it easier to maintain.  I've also
> refactored the tests somewhat and added your copyright information.
>
> Is this OK with you?

I had discussed some change to his original patch with Lars. Can't 
remember all point, just these:

+ if group not in {'console_scripts', }:

"gui_scripts"m are missing. Using a set there is uncommon, since it is a 
constant value anyway.

+ # And finally try to load top level modules. This should not have any

+ # side-effects.

I'd try loading the top level module first. As this is a pre-condition 
for loading the entry-points.
Lars-Dominik Braun Feb. 8, 2021, 8:02 a.m. UTC | #8
Hi Hartmut,

> I had discussed some change to his original patch with Lars. Can't 
> remember all point, just these:
these points were addressed, before Maxim merged the change into
core-updates.

Cheers,
Lars
diff mbox series

Patch

From 2df41c3fb476822efac1aa8dac8368e91a0e360a Mon Sep 17 00:00:00 2001
From: Lars-Dominik Braun <lars@6xq.net>
Date: Sun, 3 Jan 2021 10:30:29 +0100
Subject: [PATCH] build-system/python: Add a sanity check phase.

Add a new phase validating the usability of installed Python packages.

* gnu/packages/aux-files/python/sanity-check.py: New file.
* Makefile.am (AUX_FILES): Register it.
* guix/build-system/python.scm (sanity-check.py): New variable.
(lower): Add the script as an implicit input.
* guix/build/python-build-system.scm: Remove trailing #t.
(sanity-check): New phase.
(%standard-phases): Use it.
* tests/builders.scm (test-build-package): New syntax.
("python-build-system: dummy-ok")
("python-build-system: dummy-fail-requirements")
("python-build-system: dummy-fail-import")
("python-build-system: dummy-fail-console-script"): Add tests.
---
 Makefile.am                                   |   1 +
 gnu/packages/aux-files/python/sanity-check.py |  85 ++++++++++++++
 guix/build-system/python.scm                  |   8 ++
 guix/build/python-build-system.scm            |  26 +++--
 tests/builders.scm                            | 106 +++++++++++++++++-
 5 files changed, 212 insertions(+), 14 deletions(-)
 create mode 100644 gnu/packages/aux-files/python/sanity-check.py

diff --git a/Makefile.am b/Makefile.am
index dc5cf9babc..dddae69ff1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -380,6 +380,7 @@  AUX_FILES =						\
   gnu/packages/aux-files/linux-libre/4.4-i686.conf	\
   gnu/packages/aux-files/linux-libre/4.4-x86_64.conf	\
   gnu/packages/aux-files/pack-audit.c			\
+  gnu/packages/aux-files/python/sanity-check.py		\
   gnu/packages/aux-files/run-in-namespace.c
 
 # Templates, examples.
diff --git a/gnu/packages/aux-files/python/sanity-check.py b/gnu/packages/aux-files/python/sanity-check.py
new file mode 100644
index 0000000000..1ba912a931
--- /dev/null
+++ b/gnu/packages/aux-files/python/sanity-check.py
@@ -0,0 +1,85 @@ 
+# GNU Guix --- Functional package management for GNU
+# Copyright © 2021 Lars-Dominik Braun <lars@6xq.net>
+#
+# This file is part of GNU Guix.
+#
+# GNU Guix is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or (at
+# your option) any later version.
+#
+# GNU Guix is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
+
+from __future__ import print_function  # Python 2 support.
+import importlib
+import pkg_resources
+import sys
+import traceback
+
+try:
+    from importlib.machinery import PathFinder
+except ImportError:
+    PathFinder = None
+
+ret = 0
+
+# Only check site-packages installed by this package, but not dependencies
+# (which pkg_resources.working_set would include). Path supplied via argv.
+ws = pkg_resources.find_distributions(sys.argv[1])
+
+for dist in ws:
+    print('validating', repr(dist.project_name), dist.location)
+    try:
+        print('...checking requirements', end=': ')
+        req = str(dist.as_requirement())
+        # dist.activate() is not enough to actually check requirements, we
+        # have to .require() it.
+        pkg_resources.require(req)
+        print('OK')
+    except Exception as e:
+        print('ERROR:', req, e)
+        ret = 1
+        continue
+
+    # Try to load entry points of console scripts too, making sure they
+    # work. They should be removed if they don't. Other groups may not be
+    # safe, as they can depend on optional packages.
+    for group, v in dist.get_entry_map().items():
+        if group not in {'console_scripts', }:
+            continue
+        for name, ep in v.items():
+            try:
+                print('...trying to load endpoint', group, name, end=': ')
+                ep.load()
+                print('OK')
+            except Exception:
+                print('ERROR:')
+                traceback.print_exc(file=sys.stdout)
+                ret = 1
+                continue
+
+    # And finally try to load top level modules. This should not have any
+    # side-effects.
+    for name in dist.get_metadata_lines('top_level.txt'):
+        # Only available on Python 3.
+        if PathFinder and PathFinder.find_spec(name) is None:
+            # Ignore unavailable modules. Cannot use ModuleNotFoundError,
+            # because it is raised by failed imports too.
+            continue
+        try:
+            print('...trying to load module', name, end=': ')
+            importlib.import_module(name)
+            print('OK')
+        except Exception:
+            print('ERROR:')
+            traceback.print_exc(file=sys.stdout)
+            ret = 1
+            continue
+
+sys.exit(ret)
diff --git a/guix/build-system/python.scm b/guix/build-system/python.scm
index e39c06528e..2bb6fa87ca 100644
--- a/guix/build-system/python.scm
+++ b/guix/build-system/python.scm
@@ -2,6 +2,7 @@ 
 ;;; Copyright © 2013, 2014, 2015, 2016, 2017 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2013 Andreas Enge <andreas@enge.fr>
 ;;; Copyright © 2013 Nikita Karetnikov <nikita@karetnikov.org>
+;;; Copyright © 2021 Lars-Dominik Braun <lars@6xq.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -19,6 +20,8 @@ 
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (guix build-system python)
+  #:use-module ((gnu packages) #:select (search-auxiliary-file))
+  #:use-module (guix gexp)
   #:use-module (guix store)
   #:use-module (guix utils)
   #:use-module (guix memoization)
@@ -70,6 +73,10 @@  extension, such as '.tar.gz'."
   (let ((python (resolve-interface '(gnu packages python))))
     (module-ref python 'python-2)))
 
+(define sanity-check.py
+  ;; The script used to validate the installation of a Python package.
+  (search-auxiliary-file "python/sanity-check.py"))
+
 (define* (package-with-explicit-python python old-prefix new-prefix
                                        #:key variant-property)
   "Return a procedure of one argument, P.  The procedure creates a package with
@@ -156,6 +163,7 @@  pre-defined variants."
                         ;; Keep the standard inputs of 'gnu-build-system'.
                         ,@(standard-packages)))
          (build-inputs `(("python" ,python)
+                         ("sanity-check.py" ,(local-file sanity-check.py))
                          ,@native-inputs))
          (outputs outputs)
          (build python-build)
diff --git a/guix/build/python-build-system.scm b/guix/build/python-build-system.scm
index 7fef0b2278..1f11dd2b0a 100644
--- a/guix/build/python-build-system.scm
+++ b/guix/build/python-build-system.scm
@@ -9,6 +9,7 @@ 
 ;;; Copyright © 2019, 2020, 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;; Copyright © 2020 Jakub Kądziołka <kuba@kadziolka.net>
 ;;; Copyright © 2020 Efraim Flashner <efraim@flashner.co.il>
+;;; Copyright © 2021 Lars-Dominik Braun <lars@6xq.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -134,6 +135,15 @@ 
              (apply invoke "python" "./setup.py" command params)))
       (error "no setup.py found")))
 
+(define* (sanity-check #:key tests? inputs outputs #:allow-other-keys)
+  "Ensure packages depending on this package via setuptools work properly,
+their advertised endpoints work and their top level modules are importable
+without errors."
+  (let ((sanity-check.py (assoc-ref inputs "sanity-check.py")))
+    ;; Make sure the working directory is empty (i.e. no Python modules in it)
+    (with-directory-excursion "/tmp"
+      (invoke "python" sanity-check.py (site-packages inputs outputs)))))
+
 (define* (build #:key use-setuptools? #:allow-other-keys)
   "Build a given Python package."
   (call-setuppy "build" '() use-setuptools?)
@@ -225,8 +235,7 @@  useful when running checks after installing the package."
     ;; '--invalidation-mode' option, do not generate any.
     (unless <3.7?
       (invoke "python" "-m" "compileall" "--invalidation-mode=unchecked-hash"
-              out))
-    #t))
+              out))))
 
 (define* (wrap #:key inputs outputs #:allow-other-keys)
   (let ((pythonpath (guix-pythonpath inputs)))
@@ -262,8 +271,7 @@  installed with setuptools."
          (easy-install-pth (string-append site-packages "/easy-install.pth"))
          (new-pth (string-append site-packages "/" name ".pth")))
     (when (file-exists? easy-install-pth)
-      (rename-file easy-install-pth new-pth))
-    #t))
+      (rename-file easy-install-pth new-pth))))
 
 (define* (ensure-no-mtimes-pre-1980 #:rest _)
   "Ensure that there are no mtimes before 1980-01-02 in the source tree."
@@ -275,8 +283,7 @@  installed with setuptools."
     (ftw "." (lambda (file stat flag)
                (unless (<= early-1980 (stat:mtime stat))
                  (utime file early-1980 early-1980))
-               #t))
-    #t))
+               #t))))
 
 (define* (enable-bytecode-determinism #:rest _)
   "Improve determinism of pyc files."
@@ -284,8 +291,7 @@  installed with setuptools."
   (setenv "PYTHONHASHSEED" "0")
   ;; Prevent Python from creating .pyc files when loading modules (such as
   ;; when running a test suite).
-  (setenv "PYTHONDONTWRITEBYTECODE" "1")
-  #t)
+  (setenv "PYTHONDONTWRITEBYTECODE" "1"))
 
 (define* (ensure-no-cythonized-files #:rest _)
   "Check the source code for @code{.c} files which may have been pre-generated
@@ -296,8 +302,7 @@  by Cython."
               (string-append (string-drop-right file 3) "c")))
         (when (file-exists? generated-file)
           (format #t "Possible Cythonized file found: ~a~%" generated-file))))
-    (find-files "." "\\.pyx$"))
-  #t)
+    (find-files "." "\\.pyx$")))
 
 (define %standard-phases
   ;; The build phase only builds C extensions and copies the Python sources,
@@ -319,6 +324,7 @@  by Cython."
     (add-after 'install 'wrap wrap)
     (add-before 'check 'add-install-to-pythonpath add-install-to-pythonpath)
     (add-before 'check 'add-install-to-path add-install-to-path)
+    (add-after 'check 'sanity-check sanity-check)
     (add-before 'strip 'rename-pth-file rename-pth-file)))
 
 (define* (python-build #:key inputs (phases %standard-phases)
diff --git a/tests/builders.scm b/tests/builders.scm
index fdcf38ded3..c5528b2593 100644
--- a/tests/builders.scm
+++ b/tests/builders.scm
@@ -1,5 +1,6 @@ 
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2012, 2013, 2014, 2015, 2019 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2021 Lars-Dominik Braun <lars@6xq.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -17,19 +18,19 @@ 
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 
-(define-module (test-builders)
+(define-module (tests builders)
   #:use-module (guix download)
   #:use-module (guix build-system)
   #:use-module (guix build-system gnu)
+  #:use-module (guix build-system python)
   #:use-module (guix store)
+  #:use-module (guix monads)
   #:use-module (guix utils)
   #:use-module (guix base32)
   #:use-module (guix derivations)
   #:use-module (gcrypt hash)
   #:use-module (guix tests)
-  #:use-module ((guix packages)
-                #:select (package?
-                          package-derivation package-native-search-paths))
+  #:use-module (guix packages)
   #:use-module (gnu packages bootstrap)
   #:use-module (ice-9 match)
   #:use-module (srfi srfi-1)
@@ -78,4 +79,101 @@ 
 (test-assert "gnu-build-system"
   (build-system? gnu-build-system))
 
+
+;;;
+;;; Test the sanity-check phase of the Python build system.
+;;;
+
+(define-syntax-rule (test-build-package name expect-failure? package)
+  "Return a test named NAME, building PACKAGE in the external store."
+  (with-external-store store
+    (unless store (test-skip 1))
+    (let ((build (lambda (p)
+                   (build-derivations
+                    store (list (package-derivation store p))))))
+      (if expect-failure?
+          (test-error name
+                      (store-protocol-error?)
+                      (build package))
+          (test-assert name (build package))))))
+
+(test-build-package "python-build-system: dummy-ok" #f
+                    (dummy-package "python-dummy-ok"
+                      (build-system python-build-system)
+                      (arguments
+                       `(#:phases
+                         (modify-phases %standard-phases
+                           (replace 'unpack
+                             (lambda _
+                               (mkdir-p "dummy")
+                               (invoke "touch" "dummy/__init__.py")
+                               (with-output-to-file "setup.py"
+                                 (lambda _
+                                   (display "\
+from setuptools import setup
+setup(name='dummy-ok',
+      version='0',
+      packages=['dummy'])"))))))))))
+
+(test-build-package "python-build-system: dummy-fail-requirements" #t
+                    (dummy-package "python-dummy-fail-requirements"
+                      (build-system python-build-system)
+                      (arguments
+                       `(#:tests? #f
+                         #:phases
+                         (modify-phases %standard-phases
+                           (replace 'unpack
+                             (lambda _
+                               (mkdir-p "dummy")
+                               (invoke "touch" "dummy/__init__.py")
+                               (with-output-to-file "setup.py"
+                                 (lambda _
+                                   (display "\
+from setuptools import setup
+setup(name='dummy-fail-requirements',
+      version='0',
+      packages=['dummy'],
+      install_requires=['nonexistent'])"))))))))))
+
+(test-build-package "python-build-system: dummy-fail-import" #t
+                    (dummy-package "python-dummy-fail-import"
+                      (build-system python-build-system)
+                      (arguments
+                       `(#:tests? #f
+                         #:phases
+                         (modify-phases %standard-phases
+                           (replace 'unpack
+                             (lambda _
+                               (mkdir-p "dummy")
+                               (with-output-to-file "dummy/__init__.py"
+                                 (lambda _
+                                   (display "import nonexistent")))
+                               (with-output-to-file "setup.py"
+                                 (lambda _
+                                   (display "\
+from setuptools import setup
+setup(name='dummy-fail-import',
+      version='0',
+      packages=['dummy'])"))))))))))
+
+(test-build-package "python-build-system: dummy-fail-console-script" #f
+                    (dummy-package "python-dummy-fail-console-script"
+                      (build-system python-build-system)
+                      (arguments
+                       `(#:tests? #f
+                         #:phases
+                         (modify-phases %standard-phases
+                           (replace 'unpack
+                             (lambda _
+                               (mkdir-p "dummy")
+                               (invoke "touch" "dummy/__init__.py")
+                               (with-output-to-file "setup.py"
+                                 (lambda _
+                                   (display "\
+from setuptools import setup
+setup(name='dummy-fail-console-script',
+      version='0',
+      packages=['dummy'],
+      entry_points={'console_scripts': ['broken = dummy:nonexistent']})"))))))))))
+
 (test-end "builders")
-- 
2.30.0