[bug#51512] [PATCH]: gnu: Add bash-bcu.

[Tobias Geerinckx-Rice via Guix-patches via <guix-patches@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 14200 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]