Hi Massimo,

Thank you for your submission. I am adding some specific notes about the package definition followed by some high-level notes about the overall package and the patch.

On 3/8/24 16:00, Massimo Zaniboni wrote:

> +    (source
> +     (origin
> +       (method git-fetch)
> +       (uri (git-reference
> +             (url
> ["https://github.com/apenwarr/redo"](https://github.com/apenwarr/redo)
> )
> +             (commit (string-append "redo-" version))))
> +       (sha256
> +        (base32 "0z78fmkb85k9hnpvh0pgi8cahljjgnr6j7mb614cykvpa3xsyl0p"))))

`guix lint redo-apenwarr` reports an warning about the package name not being included in the source. This can be fixed by using the [file-name field in the origin](https://guix.gnu.org/manual/en/html_node/origin-Reference.html). This is helpful so that the store path clearly identifies the package it is related to. The README for this package mentions that there are other implementations of the redo build system. If an alternative implementation is added at some point in the future then store paths that simply identify themselves as "redo" will be ambiguous. Linting is one of the steps mentioned in the "[Submitting Patches](https://guix.gnu.org/manual/en/html_node/Submitting-Patches.html)" section of the manual. If the output from this tool or any of the other steps is unclear please let me know and I'll do my best to help!

> +       #:parallel-build? #f
> +       #:parallel-tests? #f

Why is parallel building/testing disabled? Does this cause a build failure? If so, it would be preferable to find a way to fix it. If it cannot be fixed please add a comment explaining the problem.

> +       #:phases (modify-phases %standard-phases

Please make this modify-phases call into a [G-Expression](https://guix.gnu.org/manual/en/html_node/G_002dExpressions.html), as this is the updated convention and will become relevant to a later note. You will probably also want to change the arguments list from using a backtick to calling the `list` function.

> +                      (patch-shebang "minimal/do")

My understanding is that the patch-source-shebangs phase, which runs before build in gnu-build-system, should patch this file. Do you know why it was necessary to add this line?

> +
> +                      ;; In Guix build phase there is no anymore a Git
> repo,
> +                      ;; hence the Git tool cannot be anymore called.
> +                      ;; So the content of the file is manually generated.
> +                      (let* ((repo-version "0.42d")
> +                             (repo-commit
> +                              "7f00abc36be15f398fa3ecf9f4e5283509c34a00")
> +                             (repo-date "2021-07-27 20:48:36 -0700")
> +                             (repo-head (format #f
> +                                         "(HEAD -> main, tag: redo-~a)"
> +                                         repo-version)))
> +
> +                        (substitute* '("redo/version/gitvars.pre")
> +                          (("\\$Format:%H\\$")
> +                           repo-commit)
> +                          (("\\$Format:%d\\$")
> +                           repo-head)
> +                          (("\\$Format:%ci\\$")
> +                           repo-date)))

I see that git is included in native-inputs already, which should make it available in the build phase. If this is still a problem then I think there is something else we need to fix, rather than constructing the file manually.

> +                      ;; Redo scripts can inject shebangs headers to
> generated scripts.
> +                      (substitute* '("bin/default.do"
> +                                     "t/203-make/whichmake.do"
> +                                     "redo/py.do"
> +                                     "redoconf/link.od"
> +                                     "redoconf/run.od"
> +                                     "redoconf/link-shlib.od"
> +                                     "redoconf/_compile.od"
> +                                     "redoconf/compile.od"
> +                                     "minimal/do")
> +                        (("#!/bin/sh")
> +                         (format #f "#!~a"
> +                                 (which "sh"))))

In contrast to the patch-shebang call above, I think that this substitution is necessary because the shebang is in the contents of the script not the leading line (at least, this is the case for bin/default.do). However, the preferred way to locate binaries is with G-Expressions rather than looking them up dynamically. In this case I would expect the call to be `#$([file-append](https://guix.gnu.org/manual/en/html_node/G_002dExpressions.html#index-file_002dappend) bash "/bin/sh")`. There are some other places in the package where this also applies, but I will only annotate the ones that are meaningfully different.

> +                      ;; Use `perl' on the store.
> +                      (substitute* '("t/200-shell/nonshelltest.do")
> +                        (("/usr/bin/env perl")
> +                         (format #f "~a"
> +                                 (which "perl"))))

I don't think the format call is necessary here?

> +
> +                      ;; Use `gcc' compiler, because Guix has no
> default `cc' compiler.
> +                      (substitute* '("docs/cookbook/hello/hello.do"
> +                                     "t/110-compile/LD.do"
> +                                     "t/110-compile/CC.do"
> +                                     "t/110-compile/yellow.o.do"
> +                                     "t/111-example/CC.do"
> +                                     "t/111-example/hello.do")
> +                        (("^([ \t]*)cc " dummy starting-spaces)
> +                         (string-append starting-spaces "gcc ")))

While guix packages do not typically patch every single file to use absolute paths I think that it is better to use file-append since a substitution is happening here anyway.

> +
> +                      (substitute* '("docs/cookbook/c/flagtest.o.od")
> +                        (("^CC=\"\\$CC\"")
> +                         "CC=\"gcc\"")))))))

As above, file-append would be appropriate here. --- The output of the build works well. I was able to run the ["Hello, world!" example from the manual](https://redo.readthedocs.io/en/latest/cookbook/hello/) from a [container](https://guix.gnu.org/manual/en/html_node/Invoking-guix-shell.html#index-container) with only redo-apenwall, coreutils, and gcc-toolchain (coreutils might not have been necessary, but it was convenient for me and reasonable for a minimal container IMO). Additionally, building with [`--rounds=2`](https://guix.gnu.org/manual/en/html_node/Common-Build-Options.html) succeeded. Have you put any thought into what a redo-build-system in Guix might look like? Since it advertises itself as a make replacement I do not expect that packages using it would be naturally compatible with gnu-build-system.
I do not see any contents in the commit message other than the header line and Change-ID. This might be related to the problem I explain in the next paragraph but please make sure that the commit message follows the [Change Log format](https://www.gnu.org/prep/standards/standards.html#Change-Logs). You can look at previous commits in the log to see relevant examples, patches that add new packages typically have simple messages. As a final note, I want to address some problems I ran into with the patch format. I'm not sure if the source of the problem is on your machine or if it has something to do with the server but I have previously downloaded patches from this server and they applied cleanly so I think it might have to do with your email client. The problems I ran into include the following:

1. Unchanged lines were prefixed with 2 spaces instead of 1. This caused `git apply` to report an error that the unchanged text could not be found.
2. There were 3 erroneous line breaks. This caused `git apply` to report that the patch was corrupt.

You can see the version of the patch I received with `wget https://issues.guix.gnu.org/issue/69661/attachment/0`. I manually modified the patch file so that it applies cleanly in order to perform the above review. Would it be possible for you to try sending the next revision using `git send-email` as [described by the manual](https://guix.gnu.org/manual/en/html_node/Sending-a-Patch-Series.html)? Note that while the section is titled "Sending a Patch Series" it also applies to sending single patches.