From debbugs-submit-bounces@debbugs.gnu.org Sun Nov 07 08:43:03 2021 Received: (at 51512) by debbugs.gnu.org; 7 Nov 2021 13:43:03 +0000 Received: from localhost ([127.0.0.1]:52093 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mjiRq-0001Xs-1y for submit@debbugs.gnu.org; Sun, 07 Nov 2021 08:43:03 -0500 Received: from tobias.gr ([80.241.217.52]:41858) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mjiRm-0001Xc-Py for 51512@debbugs.gnu.org; Sun, 07 Nov 2021 08:43:00 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; s=2018; bh=Vs9KzApTY6x9O S7x6FWagzdoE/5nhGdmIzbNhRVV2ZE=; h=in-reply-to:date:subject:cc:to: from:references; d=tobias.gr; b=SQOE03ARid4V0sbkc1MQfl1cLZ3xzTB5EIJdrn HxKEwaOzGsxMa0FEbGlGNx0Le8psbm+T+OAsl3ys3s1p8OufmNy4E8Woeg7LLaDvzLQCyX rKyEXE6Udwz2uC+wvOUCbbVSCUJZxEOfNiYoPUG+AzEHqHZAdLVSjNBFLSTCivfXCVkNek ElO4lZ9VkRiNuCNsOTyzsSEvAMLkEgRdZ5w6Yy1oMmVC/n1ehFsWfTQCKcAXFGWRCd5YlW DWjXrF/1y1FFGXXnmni2HHwXFAkk5UZbYMaJikg87LYKe4/Y+wEhnl5UfudIXtZmbWM5hj ag+gBMJLFZ7uIVh9ZK1gT+ww== Received: by submission.tobias.gr (OpenSMTPD) with ESMTPSA id 6130fc35 (TLSv1.3:AEAD-AES256-GCM-SHA384:256:NO); Sun, 7 Nov 2021 13:42:55 +0000 (UTC) References: <3add15b77522d6e9ebd715a19d966666@selfhosted.xyz> From: Tobias Geerinckx-Rice To: david larsson Subject: Re: [bug#51512] [PATCH]: gnu: Add bash-bcu. Date: Sun, 07 Nov 2021 13:40:30 +0100 In-reply-to: <3add15b77522d6e9ebd715a19d966666@selfhosted.xyz> BIMI-Selector: v=BIMI1; s=default; Message-ID: <87r1bsqdy8.fsf@nckx> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" X-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 51512 Cc: 51512@debbugs.gnu.org, guix-patches@gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) --=-=-= Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable David, Thank you! (One fire's out, you'll be glad to hear, so in return here's most=20 of a review. I still reserve the right to tinker with the hunk=20 below =E2=80=94 you'll see which =E2=80=94 later today.) david larsson =E5=86=99=E9=81=93=EF=BC=9A > This patch adds "bash-coding-utils" as the bash-bcu package. [=E2=80=A6] > +(define-public bash-bcu Let's keep =E2=80=98bash-coding-utils=E2=80=99 as the name then, both of th= e=20 variable and the package name string. > + (let ((pyver (version-major+minor (package-version python))) > + (guilever (version-major+minor (package-version=20 > guile-3.0)))) These aren't used until #:builder, which already has a let* where=20 they'll do fine. Binding them this early implies otherwise &=20 indents the whole package. Let's not. > + (package > + (name "bash-bcu") > + (version "v0.2.0") Don't repeat =E2=80=98v=E2=80=99 (or =E2=80=98version-=E2=80=99, =E2=80=98R= ELEASE_=E2=80=99 &c.) here. > + (home-page=20 > "https://gitlab.com/methuselah-0/bash-coding-utils.sh") Not wrong, but I'd move this to the informal =E2=80=98metadata area=E2=80= =99 at=20 the end of the package (synopsis &c.). It doesn't affect the=20 build or even the hash. > + (source > + (origin > + (method git-fetch) > + (uri (git-reference > + (commit=20 > "40d6527a9effb4e18778c37bebaa9f3a58de12d6") > + (url=20 > "https://gitlab.com/methuselah-0/bash-coding-utils.sh.git") + ;; TODO: unbundle submodules/ some day. > + (recursive? #t))) This will create=20 /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=E2=80=A6 There's some minor variation in where to put inputs, but=20 (build-system trivial-build-system) definitely belongs here, above=20 arguments, no matter what. > + (arguments > + `(#:modules ((guix build utils)) > + #:builder > + (begin > + (use-modules (guix build utils)) > + (let* ((bashfull (assoc-ref %build-inputs=20 > "bash-full")) There's nothing with which to confuse it so just "bash" for both=20 the variable and label. > + ;; Some guile libraries such as gnu bash will=20 > need > + ;; to be added to GUILE_LOAD_PATH > + (guile-bash (assoc-ref %build-inputs=20 > "guile-bash")) > + (g-bash-lib (string-append guile-bash > + "/share/guile/site/"=20 > ,guilever)) So drop the top-level =E2=80=98let=E2=80=99 and just use ,(version-major+mi= nor=20 (package-version guile-3.0) directly=E2=80=A6 > + ;; Some python libraries needs added to=20 > PYTHONPATH =E2=80=A6and add + (python-version ,(version-major+minor (package-version python))) here for use ad of =E2=80=98pyver=E2=80=99 (Guile isn't C and hard drives n= ot 5=20 MB). > + (p-elementpath-lib (string-append > + (assoc-ref %build-inputs=20 > "python-elementpath") > + "/lib/python" ,pyver=20 > "/site-packages")) > + (p-lxml-lib (string-append > + (assoc-ref %build-inputs=20 > "python-lxml") > + "/lib/python" ,pyver=20 > "/site-packages")) > + (p-netaddr-lib (string-append > + (assoc-ref %build-inputs=20 > "python-netaddr") > + "/lib/python" ,pyver=20 > "/site-packages")) > + (p-lib (string-append > + (assoc-ref %build-inputs "python") > + "/lib/python" ,pyver=20 > "/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=20 > 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=20 > "source") libexec) > + ;; Create a bcu.sh wrapping script manually that=20 > 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=20 > "bindutils") "/bin" > + ":" (assoc-ref %build-inputs=20 > "coreutils") "/bin" > + ":" (assoc-ref %build-inputs=20 > "ctypes.sh") "/bin" > + ":" (assoc-ref %build-inputs=20 > "curl") "/bin" > + ":" (assoc-ref %build-inputs=20 > "diffutils") "/bin" > + ":" (assoc-ref %build-inputs=20 > "ed") "/bin" > + ":" (assoc-ref %build-inputs=20 > "expect") "/bin" > + ":" (assoc-ref %build-inputs=20 > "find") "/bin" > + ":" (assoc-ref %build-inputs=20 > "gawk") "/bin" > + ":" (assoc-ref %build-inputs=20 > "grep") "/bin" > + ":" (assoc-ref %build-inputs=20 > "guile") "/bin" > + ":" (assoc-ref %build-inputs=20 > "inetutils") "/bin" > + ":" (assoc-ref %build-inputs=20 > "jq") "/bin" > + ":" (assoc-ref %build-inputs=20 > "libxml2-xpath0") "/bin" > + ":" (assoc-ref %build-inputs=20 > "netcat") "/bin" > + ":" (assoc-ref %build-inputs=20 > "nmap") "/bin" > + ":" (assoc-ref %build-inputs=20 > "pcre/bin") "/bin" > + ":" (assoc-ref %build-inputs=20 > "perl") "/bin" > + ":" (assoc-ref %build-inputs=20 > "php") "/bin" > + ":" (assoc-ref %build-inputs=20 > "prips") "/bin" > + ":" (assoc-ref %build-inputs=20 > "python") "/bin" > + ":" (assoc-ref %build-inputs=20 > "python-yq") "/bin" > + ":" (assoc-ref %build-inputs=20 > "sed") "/bin" > + ":" (assoc-ref %build-inputs=20 > "socat") "/bin" > + ":" (assoc-ref %build-inputs=20 > "util-linux") "/bin" > + ":" (assoc-ref %build-inputs=20 > "which") "/bin" > + ":" (assoc-ref %build-inputs=20 > "xdg-utils") "/bin" > + ":" (assoc-ref %build-inputs=20 > "yad") "/bin"))) > + (display (string-append "#!" bashfull=20 > "/bin/bash\n") bcu-port) > + (display > + (string-append > + "[[ \"$_BCU_SH_LOADED\" =3D=3D YES ]] || {=20 > \nexport PATH=3D\"" > + pathline "${PATH:+:}${PATH}\"\nexport=20 > PYTHONPATH=3D\"" > + pylibsline=20 > "${PYTHONPATH:+:}${PYTHONPATH}\"\nexport GUILE_LOAD_PATH=3D\"" > + g-bash-lib=20 > "${GUILE_LOAD_PATH:+:}${GUILE_LOAD_PATH}\"\n" > + ;; XDG_DATA_DIRS needs set for yad to load=20 > icons properly > + "[[ -e /run/current-system/profile/share ]] &&=20 > export XDG_DATA_DIRS=3D" > + "/run/current-system/profile/share${XDG_DATA_DIRS:+:}${= XDG_DATA_DIRS}\n" > + ;; Ensure that the setuid version of ping is=20 > used > + "[[ -e /run/setuid-programs/ping ]] && " > + "ping(){ /run/setuid-programs/ping \"$@\" ; }=20 > && export -f ping\n" > + "[[ -e /run/setuid-programs/ping6 ]] && " > + "ping6(){ /run/setuid-programs/ping6 \"$@\" ;=20 > } && export -f ping6\n}\n") > + bcu-port) > + (display (string-append "source " libexec=20 > "/bcu.sh\n") bcu-port) > + (close-port bcu-port) [So this is the part I was waiting to finish :-) I still don't=20 have time now.] I really want to rewrite this whole block, but for now I just have=20 1 question: why not simply append /run/setuid-programs to the=20 start of $PATH here? What's the difference, if any, and do we=20 care? > + (chmod binfile #o555) s/binfile/wrapper/ or somesuch. > + (setenv "PATH" (string-append pathline ":"=20 > (getenv "PATH")))) s/pathline/path/ > + (for-each (lambda (file) > + (substitute* file substitute* supports a list of (found-)files as the first argument=20 directly. No need to call it multiple times. > + (find-files out ".*\\.sh")) .* is noise: "\\.sh$" > + (find-files out=20 > ".*\\.(sh|scm|awk|php|py)$")) =E2=80=A6same here. Wonderful that you took the trouble to run tests! Let's visually separate the =E2=80=98test phase=E2=80=99: + + ;;; Now that everything's installed, prepare & run=20 the tests. + ;; Set up PATH for tests. > + (setenv "PATH" (string-append bin ":" (getenv=20 > "PATH"))) > + ;; Some tests need a HOME-directory > + (setenv "HOME" "/tmp") > + ;; Disable network tests, and all tests for=20 > setopts which > + ;; don't work inside the Guix build environment > + (call-with-output-file (string-append libexec=20 > "/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_te= st_6\n") > + port))) + (with-output-to-file (string-append libexec=20 "/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=20 > prototyping in Bash") I dropped the leading =E2=80=98Bash =E2=80=99 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=E2=80=94I used less to avoid turning it into=20 an unreadable mess in some MUAs. > + "Bash-bcu contains bash functions and wrappers that can=20 > be useful when" > + " writing quick implementations of new programs. It=20 > helps you work with" > + " JSON, XML, parallelization and installs some commonly=20 > 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=20 more keywords than =E2=80=98command=E2=80=99! ;-) @key, @code, =E2=80=A6 > + Just run @command{. bcu.sh}, type I left this out because we don't usually include =E2=80=98getting started= =E2=80=99=20 instructions in package descriptions. There are exceptions, and=20 this package not including info (or man) pages is unfortunate, so=20 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 --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCYYfX4A0cbWVAdG9iaWFz LmdyAAoJEA2w/4hPVW15BH4BAI66CGDpcXE1T+t0wJDld2La2F1abNGwIkDJdDhH bbCDAQDZ9wgsFR9sak430GhQwr87Ca6aZMH6bOgcFNlABCpEAg== =H+/K -----END PGP SIGNATURE----- --=-=-=--