Package: guix-patches;
Reported by: david larsson <david.larsson <at> selfhosted.xyz>
Date: Sat, 30 Oct 2021 15:00:01 UTC
Severity: normal
Tags: patch
View this message in rfc822 format
From: Tobias Geerinckx-Rice <me <at> tobias.gr> To: david larsson <david.larsson <at> selfhosted.xyz> Cc: 51512 <at> debbugs.gnu.org Subject: [bug#51512] [PATCH]: gnu: Add bash-bcu. Date: Sun, 07 Nov 2021 13:40:30 +0100
[Message part 1 (text/plain, inline)]
David,
Thank you!
(One fire's out, you'll be glad to hear, so in return here's most
of a review. I still reserve the right to tinker with the hunk
below — you'll see which — later today.)
david larsson 写道：
> This patch adds "bash-coding-utils" as the bash-bcu package.
[…]
> +(define-public bash-bcu
Let's keep ‘bash-coding-utils’ as the name then, both of the
variable and the package name string.
> + (let ((pyver (version-major+minor (package-version python)))
> + (guilever (version-major+minor (package-version
> guile-3.0))))
These aren't used until #:builder, which already has a let* where
they'll do fine. Binding them this early implies otherwise &
indents the whole package. Let's not.
> + (package
> + (name "bash-bcu")
> + (version "v0.2.0")
Don't repeat ‘v’ (or ‘version-’, ‘RELEASE_’ &c.) here.
> + (home-page
> "https://gitlab.com/methuselah-0/bash-coding-utils.sh")
Not wrong, but I'd move this to the informal ‘metadata area’ at
the end of the package (synopsis &c.). It doesn't affect the
build or even the hash.
> + (source
> + (origin
> + (method git-fetch)
> + (uri (git-reference
> + (commit
> "40d6527a9effb4e18778c37bebaa9f3a58de12d6")
> + (url
> "https://gitlab.com/methuselah-0/bash-coding-utils.sh.git")
+ ;; TODO: unbundle submodules/ some day.
> + (recursive? #t)))
This will create
/gnu/store/ivnsnv2yhq9mawxvacmjwxw1z535x9aq-git-checkout.
Guix doesn't care, but please use
+ (file-name (git-file-name name version))
to make it more meaningful for humans.
> + (base32
> + "007g6wfybjr0ms32qikb545r11lgm3p98cd7dbzpfyh0grgn9vj1"))))
This can be one line.
> + (inputs `(("bash-full" ,bash)
> + ("bindutils" ,isc-bind "utils")
> + ("coreutils" ,coreutils)
> + ("ctypes.sh" ,bash-ctypes)
> + ("curl" ,curl)
> + ("diffutils" ,diffutils)
> + ("ed" ,ed)
> + ("expect" ,expect)
> + ("find" ,findutils)
> + ("gawk" ,gawk)
> + ("grep" ,grep)
> + ("guile" ,guile-3.0)
> + ("guile-bash" ,guile-bash)
> + ("guile-daemon" ,guile-daemon)
> + ("inetutils" ,inetutils)
> + ("jq" ,jq)
> + ("libxml2-xpath0" ,libxml2-xpath0)
> + ("netcat" ,netcat)
> + ("nmap" ,nmap)
> + ("pcre/bin" ,pcre "bin")
> + ("perl" ,perl)
> + ("php" ,php)
> + ("prips" ,prips)
> + ("python" ,python)
> + ("python-elementpath" ,python-elementpath)
> + ("python-lxml" ,python-lxml)
> + ("python-netaddr" ,python-netaddr)
> + ("python-yq" ,python-yq)
> + ("sed" ,sed)
> + ("socat" ,socat)
> + ("util-linux" ,util-linux)
> + ("which" ,which)
> + ("xdg-utils" ,xdg-utils)
> + ("yad" ,yad)))
So the de-facto ordering of common fields is something like:
name
version
source
build-system
outputs ; a bit inconsistent, yes, and sometimes put after *inputs
arguments ; to the build-system
native-inputs, inputs, propagated-inputs
metadata: synopsis, description, home-page, properties, license…
There's some minor variation in where to put inputs, but
(build-system trivial-build-system) definitely belongs here, above
arguments, no matter what.
> + (arguments
> + `(#:modules ((guix build utils))
> + #:builder
> + (begin
> + (use-modules (guix build utils))
> + (let* ((bashfull (assoc-ref %build-inputs
> "bash-full"))
There's nothing with which to confuse it so just "bash" for both
the variable and label.
> + ;; Some guile libraries such as gnu bash will
> need
> + ;; to be added to GUILE_LOAD_PATH
> + (guile-bash (assoc-ref %build-inputs
> "guile-bash"))
> + (g-bash-lib (string-append guile-bash
> + "/share/guile/site/"
> ,guilever))
So drop the top-level ‘let’ and just use ,(version-major+minor
(package-version guile-3.0) directly…
> + ;; Some python libraries needs added to
> PYTHONPATH
…and add
+ (python-version ,(version-major+minor (package-version python)))
here for use ad of ‘pyver’ (Guile isn't C and hard drives not 5
MB).
> + (p-elementpath-lib (string-append
> + (assoc-ref %build-inputs
> "python-elementpath")
> + "/lib/python" ,pyver
> "/site-packages"))
> + (p-lxml-lib (string-append
> + (assoc-ref %build-inputs
> "python-lxml")
> + "/lib/python" ,pyver
> "/site-packages"))
> + (p-netaddr-lib (string-append
> + (assoc-ref %build-inputs
> "python-netaddr")
> + "/lib/python" ,pyver
> "/site-packages"))
> + (p-lib (string-append
> + (assoc-ref %build-inputs "python")
> + "/lib/python" ,pyver
> "/site-packages"))
> + (pylibsline (string-append
> + p-elementpath-lib ":" p-lxml-lib
> + ":" p-netaddr-lib ":" p-lib))
> + (out (assoc-ref %outputs "out"))
> + (bin (string-append out "/bin"))
> + ;; Everything but bcu.sh itself is only
> accessed
> + ;; internally by bcu so we put it in libexec.
> + (libexec (string-append out "/libexec/bcu")))
> + (mkdir-p libexec)
> + (copy-recursively (assoc-ref %build-inputs
> "source") libexec)
> + ;; Create a bcu.sh wrapping script manually that
> ensures
> + ;; we prepend necessary PATHs.
> + (mkdir-p bin)
> + (let* ((binfile (string-append bin "/bcu.sh"))
> + (bcu-port (open-file binfile "a"))
> + (pathline (string-append
> + bashfull "/bin"
> + ":" (assoc-ref %build-inputs
> "bindutils") "/bin"
> + ":" (assoc-ref %build-inputs
> "coreutils") "/bin"
> + ":" (assoc-ref %build-inputs
> "ctypes.sh") "/bin"
> + ":" (assoc-ref %build-inputs
> "curl") "/bin"
> + ":" (assoc-ref %build-inputs
> "diffutils") "/bin"
> + ":" (assoc-ref %build-inputs
> "ed") "/bin"
> + ":" (assoc-ref %build-inputs
> "expect") "/bin"
> + ":" (assoc-ref %build-inputs
> "find") "/bin"
> + ":" (assoc-ref %build-inputs
> "gawk") "/bin"
> + ":" (assoc-ref %build-inputs
> "grep") "/bin"
> + ":" (assoc-ref %build-inputs
> "guile") "/bin"
> + ":" (assoc-ref %build-inputs
> "inetutils") "/bin"
> + ":" (assoc-ref %build-inputs
> "jq") "/bin"
> + ":" (assoc-ref %build-inputs
> "libxml2-xpath0") "/bin"
> + ":" (assoc-ref %build-inputs
> "netcat") "/bin"
> + ":" (assoc-ref %build-inputs
> "nmap") "/bin"
> + ":" (assoc-ref %build-inputs
> "pcre/bin") "/bin"
> + ":" (assoc-ref %build-inputs
> "perl") "/bin"
> + ":" (assoc-ref %build-inputs
> "php") "/bin"
> + ":" (assoc-ref %build-inputs
> "prips") "/bin"
> + ":" (assoc-ref %build-inputs
> "python") "/bin"
> + ":" (assoc-ref %build-inputs
> "python-yq") "/bin"
> + ":" (assoc-ref %build-inputs
> "sed") "/bin"
> + ":" (assoc-ref %build-inputs
> "socat") "/bin"
> + ":" (assoc-ref %build-inputs
> "util-linux") "/bin"
> + ":" (assoc-ref %build-inputs
> "which") "/bin"
> + ":" (assoc-ref %build-inputs
> "xdg-utils") "/bin"
> + ":" (assoc-ref %build-inputs
> "yad") "/bin")))
> + (display (string-append "#!" bashfull
> "/bin/bash\n") bcu-port)
> + (display
> + (string-append
> + "[[ \"$_BCU_SH_LOADED\" == YES ]] || {
> \nexport PATH=\""
> + pathline "${PATH:+:}${PATH}\"\nexport
> PYTHONPATH=\""
> + pylibsline
> "${PYTHONPATH:+:}${PYTHONPATH}\"\nexport GUILE_LOAD_PATH=\""
> + g-bash-lib
> "${GUILE_LOAD_PATH:+:}${GUILE_LOAD_PATH}\"\n"
> + ;; XDG_DATA_DIRS needs set for yad to load
> icons properly
> + "[[ -e /run/current-system/profile/share ]] &&
> export XDG_DATA_DIRS="
> + "/run/current-system/profile/share${XDG_DATA_DIRS:+:}${XDG_DATA_DIRS}\n"
> + ;; Ensure that the setuid version of ping is
> used
> + "[[ -e /run/setuid-programs/ping ]] && "
> + "ping(){ /run/setuid-programs/ping \"$@\" ; }
> && export -f ping\n"
> + "[[ -e /run/setuid-programs/ping6 ]] && "
> + "ping6(){ /run/setuid-programs/ping6 \"$@\" ;
> } && export -f ping6\n}\n")
> + bcu-port)
> + (display (string-append "source " libexec
> "/bcu.sh\n") bcu-port)
> + (close-port bcu-port)
[So this is the part I was waiting to finish :-) I still don't
have time now.]
I really want to rewrite this whole block, but for now I just have
1 question: why not simply append /run/setuid-programs to the
start of $PATH here? What's the difference, if any, and do we
care?
> + (chmod binfile #o555)
s/binfile/wrapper/ or somesuch.
> + (setenv "PATH" (string-append pathline ":"
> (getenv "PATH"))))
s/pathline/path/
> + (for-each (lambda (file)
> + (substitute* file
substitute* supports a list of (found-)files as the first argument
directly. No need to call it multiple times.
> + (find-files out ".*\\.sh"))
.* is noise: "\\.sh$"
> + (find-files out
> ".*\\.(sh|scm|awk|php|py)$"))
…same here.
Wonderful that you took the trouble to run tests!
Let's visually separate the ‘test phase’:
+
+ ;;; Now that everything's installed, prepare & run
the tests.
+ ;; Set up PATH for tests.
> + (setenv "PATH" (string-append bin ":" (getenv
> "PATH")))
> + ;; Some tests need a HOME-directory
> + (setenv "HOME" "/tmp")
> + ;; Disable network tests, and all tests for
> setopts which
> + ;; don't work inside the Guix build environment
> + (call-with-output-file (string-append libexec
> "/disabled_tests.txt")
> + (lambda (port)
> + (display (string-append
> + "ip_of_test_1\nsetopts_test_1\nsetopts_test_2\nsetopts_test_3"
> + "\nsetopts_test_4\nsetopts_test_5\nsetopts_test_6\n")
> + port)))
+ (with-output-to-file (string-append libexec
"/disabled_tests.txt")
+ (lambda _
+ (format #t "~{~a~%~}"
+ (list "ip_of_test_1"
+ "setopts_test_1"
+ "setopts_test_2"
+ "setopts_test_3"
+ "setopts_test_4"
+ "setopts_test_5"
+ "setopts_test_6"))))
> + (synopsis "Bash functions and tools for software
> prototyping in Bash")
I dropped the leading ‘Bash ’ here.
> + (description
> + (string-append
Just use
(description
"This package contains Bash functions and wrappers that can
be useful when writing quick implementations of new programs. It
helps you work with JSON, XML, and parallelization, and installs
some commonly used helper programs used in Bash scripting.
Run @command{bcu__docs} for the full HTML documentation.")
But wrapped at 80 characters—I used less to avoid turning it into
an unreadable mess in some MUAs.
> + "Bash-bcu contains bash functions and wrappers that can
> be useful when"
> + " writing quick implementations of new programs. It
> helps you work with"
> + " JSON, XML, parallelization and installs some commonly
> used \"helper\""
Texinfo double quotes are ``thus'' but can just be dropped here.
> + @command{TAB}
> + @command{ --help}
Even the (comfortable) subset of Texinfo that Guix supports has
more keywords than ‘command’! ;-) @key, @code, …
> + Just run @command{. bcu.sh}, type
I left this out because we don't usually include ‘getting started’
instructions in package descriptions. There are exceptions, and
this package not including info (or man) pages is unfortunate, so
I kept the bcu__docs hint.
Acceptable?
> + (license license:gpl3))))
Why not lgpl3+? I can't find the gpl3-only file(s).
Kind regards,
T G-R
[signature.asc (application/pgp-signature, inline)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.