diff mbox series

[bug#47582,2/2] gnu: Add python-pysctp.

Message ID 35e212d90dd9d99a273e6c667cbd92b41b80219f.1617463189.git.h.goebel@crazy-compilers.com
State Accepted
Headers show
Series [bug#47582,1/2] gnu: lksctp-tools: Fix build of include file. | expand

Checks

Context Check Description
cbaines/submitting builds success
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue

Commit Message

Hartmut Goebel April 3, 2021, 3:28 p.m. UTC
* gnu/packages/networking.scm(python-pysctp): New variable.
---
 gnu/packages/networking.scm | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Comments

Maxime Devos April 3, 2021, 4:12 p.m. UTC | #1
On Sat, 2021-04-03 at 17:28 +0200, Hartmut Goebel wrote:
> * gnu/packages/networking.scm(python-pysctp): New variable.
> ---
>  gnu/packages/networking.scm | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> [...]
> +     #:phases
> +     (modify-phases %standard-phases
> +       (add-after 'unpack 'patch-setup.py
> +         (lambda _
> +           (substitute* "setup.py"
> +             (("include_dirs\\s*=.*")
> +              (string-append "include_dirs = ['.'] + '"
> +                             (getenv "C_INCLUDE_PATH") "'.split(':'),"))
> +             (("library_dirs\\s*=.*")
> +              (string-append "library_dirs = '"
> +                             (getenv "LIBRARY_PATH") "'.split(':'),")))
> +           #t)))))

Phases do not need to return #t anymore.  IIUC the warning message that results
if it is left out has been removed on core-updates.

Greetings,
Maxime.
Hartmut Goebel April 3, 2021, 5:37 p.m. UTC | #2
Am 03.04.21 um 18:12 schrieb Maxime Devos:
> Phases do not need to return #t anymore.  IIUC the warning message that results
> if it is left out has been removed on core-updates.

Okay, will do. Any other remarks?
Maxime Devos April 3, 2021, 7:01 p.m. UTC | #3
On Sat, 2021-04-03 at 19:37 +0200, Hartmut Goebel wrote:
> Am 03.04.21 um 18:12 schrieb Maxime Devos:
> > Phases do not need to return #t anymore.  IIUC the warning message that results
> > if it is left out has been removed on core-updates.
> 
> Okay, will do. Any other remarks?

About the following code:

> +           (substitute* "setup.py"
> +             (("include_dirs\\s*=.*")
> +              (string-append "include_dirs = ['.'] + '"
> +                             (getenv "C_INCLUDE_PATH") "'.split(':'),"))
> +             (("library_dirs\\s*=.*")
> +              (string-append "library_dirs = '"
> +                             (getenv "LIBRARY_PATH") "'.split(':'),")))

When cross-compiling, this code should most likely use CROSS_C_INCLUDE_PATH
and CROSS_LIBRARY_PATH instead.  (Admittedly, python-build-system does not
support cross-compilation yet, so this is not important ... yet.)

Suggestion: replace (getenv "C_INCLUDE_PATH") with
(getenv ,(if (%current-target-system)
             "CROSS_C_INCLUDE_PATH"
             "C_INCLUDE_PATH")).
Likewise for LIBRARY_PATH.

Some aesthetic nitpicks (YMMV):

> +  (synopsis  "Python module for the SCTP protocol stack and library")
A space has been doubled.

> * gnu/packages/networking.scm(python-pysctp): New variable.

I would put a space between .scm and (python-pysctp).  Most commit messages
do.

+(define-public python-pysctp
+(package
+  (name "python-pysctp")
+  ...

Indentation is wrong here.  See 16.5.4 Formatting Code for how to automatically
indent code.

Greetings,
Maxime.
Hartmut Goebel April 11, 2021, 3:07 p.m. UTC | #4
Thanks for the review. I applied most of the changes and poushed as
03e80cf42240f65346713ff414eb2ca628ef0884

If did not apply

> Suggestion: replace (getenv "C_INCLUDE_PATH") with
> (getenv ,(if (%current-target-system)
>              "CROSS_C_INCLUDE_PATH"
>              "C_INCLUDE_PATH")).
> Likewise for LIBRARY_PATH.
since "CROSS_C" is used on 14 times in gnu/packages/*.scm and not at all
in guix/build*. So I assume this is not the way to implement
cross-compiling :-)
Maxime Devos April 11, 2021, 3:51 p.m. UTC | #5
On Sun, 2021-04-11 at 17:07 +0200, Hartmut Goebel wrote:
> Thanks for the review. I applied most of the changes and poushed as
> 03e80cf42240f65346713ff414eb2ca628ef0884
> 
> If did not apply
> 
> > Suggestion: replace (getenv "C_INCLUDE_PATH") with
> > (getenv ,(if (%current-target-system)
> >              "CROSS_C_INCLUDE_PATH"
> >              "C_INCLUDE_PATH")).
> > Likewise for LIBRARY_PATH.

I could be wrong about how/when "CROSS_C_INCLUDE_PATH" or "C_INCLUDE_PATH" should
be used.

I don't agree with the following reasoning however:
> since "CROSS_C" is used on 14 times in gnu/packages/*.scm and not at all
> in guix/build*. So I assume this is not the way to implement
> cross-compiling :-)

... as many (much more than 14) packages have cross-compilation bugs.
For example, try searching for "CC=gcc".  Practically all usages are incorrect,
they should use ,(string-append "CC=" (cc-for-target)) instead.

gcc = *not* the cross-compiler
(cc-for-target): when cross-compiling, something like "aarch64-linux-gnu-gcc",
  otherwise simply "gcc".

I've submitted a patch adding a linter, and a patch series fixing some of the
buggy packages (at least instances of "CC=gcc", there may be other bugs).

Links:
https://issues.guix.gnu.org/47676,
https://issues.guix.gnu.org/47693

Greetings,
Maxime.
diff mbox series

Patch

diff --git a/gnu/packages/networking.scm b/gnu/packages/networking.scm
index a227cb2d3e..f8ab1bbddb 100644
--- a/gnu/packages/networking.scm
+++ b/gnu/packages/networking.scm
@@ -473,6 +473,40 @@  sockets, and also some helper utilities around SCTP.")
       ;; Others.
       license:gpl2+))))
 
+(define-public python-pysctp
+(package
+  (name "python-pysctp")
+  (version "0.6.1")
+  (source
+    (origin
+      (method url-fetch)
+      (uri (pypi-uri "pysctp" version))
+      (sha256
+        (base32 "14h2qlmfi24bizhvvqkfqfa78pzm3911ibrzy9k94i97xy1978dy"))))
+  (build-system python-build-system)
+  (inputs
+   `(("lksctp-tools" ,lksctp-tools)))
+  (arguments
+   `(#:tests? #f  ;; require network
+     #:phases
+     (modify-phases %standard-phases
+       (add-after 'unpack 'patch-setup.py
+         (lambda _
+           (substitute* "setup.py"
+             (("include_dirs\\s*=.*")
+              (string-append "include_dirs = ['.'] + '"
+                             (getenv "C_INCLUDE_PATH") "'.split(':'),"))
+             (("library_dirs\\s*=.*")
+              (string-append "library_dirs = '"
+                             (getenv "LIBRARY_PATH") "'.split(':'),")))
+           #t)))))
+  (home-page "https://github.com/p1sec/pysctp")
+  (synopsis  "Python module for the SCTP protocol stack and library")
+  (description
+    "@code{pysctp} implements the SCTP socket API.  You need a SCTP-aware
+kernel (most are).")
+  (license license:lgpl2.1+)))
+
 (define-public knockd
   (package
     (name "knockd")