| 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 |
| 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 |
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.
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?
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.
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 :-)
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 --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")