guix-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Tobias Geerinckx-Rice
Subject: [bug#51512] [PATCH]: gnu: Add bash-bcu.
Date: Sun, 07 Nov 2021 13:40:30 +0100

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: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]