[bug#41010] Upgrade Oil to 0.8.pre4

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

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

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

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