Message ID | h2J1HZL5TX-LD7iEonk5V7VW9SayB4-irRVMFeJgPnZUtokvrqPKg_5kyLqybb8UrlaW10p1-UOsek5Z8yV_qoZIPyOI4PBDQIZEFPH6--g=@protonmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [bug#41010] Upgrade Oil to 0.8.pre4 | expand |
Context | Check | Description |
---|---|---|
cbaines/applying patch | fail | View Laminar job |
Ryan, > This patch upgrades the oil package. As noted in the package > description, upstream considers this to be a stable release & > the best available version of oil despite the "pre" tag. Thanks! I agree with the name change, although I'm unaware of why ‘oil-shell’ was originally chosen. However, please do build and test patches before submitting them. These were broken in 2 places: adding the unused ‘license:’ prefix breaks evaluation, as does referring to a variable (‘oil’ in deprecated-package) before it's defined. > Subject: [PATCH] gnu: oil: Update to 0.8.pre4 Add a full stop after commit summaries, and a ‘change log’ entry as commit body: * gnu/packages/shells.scm (oil): Update to 0.8.pre4. > + (version "0.8.pre4") ; "Despite the pre4 version qualifier, > this is by far > + ; the best Oil release ever… I may > change the version > + ; numbering scheme in the near future > to reflect this." > + ; - upstream on whether to ship pre4 > in Guix ;; "Despite the pre4 version qualifier, this is by far ;; the best Oil release ever… I may change the version ;; numbering scheme in the near future to reflect this." ;; - upstream on whether to ship pre4 in Guix (version "0.8.pre4") Format long and/or multi-line comments like so: OTOH a one-line link to that thread, if one exists, would be preferable. > + (source > + (origin > + (method url-fetch) > + (uri (string-append > "https://www.oilshell.org/download/oil-" > + version ".tar.gz")) > + (sha256 > + (base32 > + > "0m2p8p5hi2r14xx9pphsa0ar56kqsa33gr2w2blc3jx07aqhjpzy")))) If you're going to re-indent like this, the hash fits beside (base32 …. > - #:strip-binaries? #f ; the binaries cannot be stripped > + `(#:strip-binaries? #f ; Strip breaks the binary. I like your comment better but the original formatting (lowercase, no full stop) was fine. > + (setenv "CC" "gcc") > (let ((out (assoc-ref outputs "out"))) > - (setenv "CC" "gcc") (let ((out (assoc-ref outputs "out"))) (do-something "foo") (do-something out)) It's canonical style in Guix (not sure about wider Schemeland) to ‘bind early’: (do-something "foo") (let ((out (assoc-ref outputs "out"))) (do-something out)) While you'll find a fair share of it's much less common, possibly frowned upon, and idly rearranging existing code is right out. > + (substitute* "configure" > + ((" cc ") " gcc ")) (substitute* "configure" ((" cc ") " $CC ")) > + (invoke > + "./configure" More line nitpicking: keep these on one line & indent the other arguments accordingly. > + (replace 'check ; The tests are not distributed in the > tarballs but > + ; upstream recommends running this > smoke test. Same as above: (replace 'check ;; The tests are not distributed in the tarballs but upstream ;; recommends running this smoke test. … Where do they recommend this? It's nice to have a link in case the recommendation changes. > + (native-inputs Nak. ‘Native’ means ‘when cross compiling, don't bother building this for the target architecture, it will only ever run on the host’… > `(("readline" ,readline))) …as ‘guix gc --references oil’ (and readline's general nature) tell us, that's not the case here: Oil links to it at run time, so it must not be native. > - (synopsis "Bash-compatible Unix shell") > - (description "Oil is a Unix / POSIX shell, compatible with > Bash. It > -implements the Oil language, which is a new shell language to > which Bash can be > -automatically translated. The Oil language is a superset of > Bash. It also > -implements the OSH language, a statically-parseable language > based on Bash as it > -is commonly written.") […] > + (synopsis "A Unix shell") > + (description "Oil is a Unix shell and programming > language. It's our upgrade > +path from Bash.") Both the original synopsis and description are much better. If certain things are no longer accurate they can be adjusted but this is just upstream's marketing pitch. > + (license (list license:psfl ; Tarball vendors python2.7 Hmm, this doesn't parse as English (it's missing a verb). I'd guess typo… but for what? Are upstream the ‘tarball vendors’ here? What was wrong with the original comment? If upstream still bundles Python (and it would seem so), that's important information that shouldn't be removed. Phew. ‘I'll just review this trivial bump before bed-time.’ This patch changes lots of things for one small package. I hope you don't mind lots of comments :-) Kind regards, T G-R
of lines out of order This bizarre formatting: is not something I did. Emacs tried to warn me about some weird encoding issues (which were in turn related to the original patch encoding, e.g. © and …) and I ignored it at my peril. The patch applied just fine. Kind regards, T G-R
On Sat, May 02, 2020 at 01:23:30AM +0200, Tobias Geerinckx-Rice via Guix-patches via wrote: > I agree with the name change, although I'm unaware of why ‘oil-shell’ was > originally chosen. No serious reason. I thought it was close the upstream name (it's the GitHub organization name) and that 'oil' was a bit short for something so obscure. Feel free to change it to 'oil' though.
From 4b9726a57da890f36337fec045248812933d63ec Mon Sep 17 00:00:00 2001 From: Ryan Prior <rprior@protonmail.com> Date: Fri, 1 May 2020 14:49:42 -0500 Subject: [PATCH] gnu: oil: Update to 0.8.pre4 --- gnu/packages/shells.scm | 66 ++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/gnu/packages/shells.scm b/gnu/packages/shells.scm index 08a475dfe1..324bdf513d 100644 --- a/gnu/packages/shells.scm +++ b/gnu/packages/shells.scm @@ -13,6 +13,7 @@ ;;; Copyright © 2019 Timothy Sample <samplet@ngyro.com> ;;; Copyright © 2019, 2020 Jan (janneke) Nieuwenhuizen <janneke@gnu.org> ;;; Copyright © 2020 Brice Waegeneire <brice@waegenei.re> +;;; Copyright © 2020 Ryan Prior <rprior@protonmail.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -754,48 +755,47 @@ Shell (pdksh).") (define-public oil (package (name "oil") - (version "0.7.0") - (source (origin - (method url-fetch) - (uri (string-append "https://www.oilshell.org/download/oil-" - version ".tar.xz")) - (sha256 - (base32 - "12c9s462879adb6mwd3fqafk0dnqsm16s18rhym6cmzfzy8v8zm3")))) + (version "0.8.pre4") ; "Despite the pre4 version qualifier, this is by far + ; the best Oil release ever… I may change the version + ; numbering scheme in the near future to reflect this." + ; - upstream on whether to ship pre4 in Guix + (source + (origin + (method url-fetch) + (uri (string-append "https://www.oilshell.org/download/oil-" + version ".tar.gz")) + (sha256 + (base32 + "0m2p8p5hi2r14xx9pphsa0ar56kqsa33gr2w2blc3jx07aqhjpzy")))) (build-system gnu-build-system) (arguments - '(#:tests? #f ; the tests are not distributed in the tarballs - #:strip-binaries? #f ; the binaries cannot be stripped + `(#:strip-binaries? #f ; Strip breaks the binary. #:phases (modify-phases %standard-phases - (add-after 'unpack 'patch-compiler-invocation - (lambda _ - (substitute* "configure" - ((" cc ") " gcc ")) - #t)) (replace 'configure (lambda* (#:key outputs #:allow-other-keys) + (setenv "CC" "gcc") + (substitute* "configure" + ((" cc ") " gcc ")) (let ((out (assoc-ref outputs "out"))) - (setenv "CC" "gcc") - ;; The configure script doesn't recognize CONFIG_SHELL. - (setenv "CONFIG_SHELL" (which "sh")) - (invoke "./configure" (string-append "--prefix=" out) - "--with-readline")))) - (add-before 'install 'make-destination + (invoke + "./configure" + (string-append "--prefix=" out) + "--with-readline")))) + (replace 'check ; The tests are not distributed in the tarballs but + ; upstream recommends running this smoke test. (lambda _ - ;; The build scripts don't create the destination directory. - (mkdir-p (string-append (assoc-ref %outputs "out") "/bin"))))))) - (inputs + (let* ((oil "_bin/oil.ovm")) + (invoke/quiet oil "osh" "-c" "echo hi") + (invoke/quiet oil "osh" "-n" "configure"))))))) + (native-inputs `(("readline" ,readline))) - (synopsis "Bash-compatible Unix shell") - (description "Oil is a Unix / POSIX shell, compatible with Bash. It -implements the Oil language, which is a new shell language to which Bash can be -automatically translated. The Oil language is a superset of Bash. It also -implements the OSH language, a statically-parseable language based on Bash as it -is commonly written.") - (home-page "https://www.oilshell.org/") - (license (list psfl ; The Oil sources include a patched Python 2 source tree - asl2.0)))) + (home-page "https://www.oilshell.org") + (synopsis "A Unix shell") + (description "Oil is a Unix shell and programming language. It's our upgrade +path from Bash.") + (license (list license:psfl ; Tarball vendors python2.7 + license:asl2.0)))) (define-public gash (package -- 2.17.1