Package: guix-patches;
Reported by: Massimo Zaniboni <mzan <at> dokmelody.org>
Date: Sat, 9 Mar 2024 03:14:01 UTC
Severity: normal
Tags: moreinfo, patch
Message #14 received at 69661 <at> debbugs.gnu.org (full text, mbox):
From: Massimo Zaniboni <mzan <at> dokmelody.org> To: Skyler Ferris <skyvine <at> protonmail.com>, 69661 <at> debbugs.gnu.org Subject: Re: [bug#69661] [PATCH] gnu: Add redo-apenwarr Date: Tue, 12 Mar 2024 16:39:57 +0100
Hi Skyler,
many thanks for your detailed review!
1) I sent right now a PATCH for: testing git send-email; fixing all
things in the review.
2) I don't consider this PATCH definitive, because: I can improve the
way I generate documentation; I'm not using the package enough in
production for being sure it is completely correct; a part of the
package is probably not optimal. I will send a candidate PATCH after
more testing and eventually your next review.
3) I will comment below to the questions of the past review...
> `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
DONE
> 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!
Now I obtain
```
$ make && ./pre-inst-env guix lint redo-apenwarr
make all-recursive
]8;;file://think/home/mzan/tmp/guix/guix-cloned-channel/gnu/packages/build-tools.scm\gnu/packages/build-tools.scm:471:7]8;;\:
warning: failed to fetch Git repository for redo-apenwarr
]8;;file://think/home/mzan/tmp/guix/guix-cloned-channel/gnu/packages/build-tools.scm\gnu/packages/build-tools.scm:471:7]8;;\:
redo-apenwarr <at> 0.42d: updater 'generic-git' failed to find upstream
releasesmake && ./pre-inst-env guix build -K redo-apenwarr
```
I have no idea how to fix them.
>> + #: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.
FIXED. Parallel builds and tests works correctly.
>> + #:phases (modify-phases %standard-phases
> Please make this modify-phases call into a G-Expression
DONE
>> + (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?
FIXED. You were right: it is not necessary.
>> +
>> + ;; 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.
I improved the package code and the comments. Probably now it is more
clear.
Put in other words: the redo-apenwarr installation script executes git
commands for querying the git repo, and for deriving the date of the
last commit. It uses this info for adding version/commit/date to the
installed application.
Apparently, after the Guix git-fetch phase, there is no anymore this
info, because there is no .git directory. So I generate this info
"manually".
This "patch" is not elegant, and I'm open to suggestions about the
correct way to handle this.
>> + ;; 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).
I double-checked all files. They are not header shebangs, but they are
shebangs in the middle of the code, because they will be added to some
generated files. So, I didn't changed this. It is correct.
> 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
DONE
>> + ;; 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?
FIXED
>> +
>> + ;; 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.
DONE
>> +
>> + (substitute* '("docs/cookbook/c/flagtest.o.od")
>> + (("^CC=\"\\$CC\"")
>> + "CC=\"gcc\"")))))))
> As above, file-append would be appropriate here.
DONE
--- 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).
I added `coreutils` also to the inputs of the package.
TODO: I need to test better in production, for seeing if it is working
all correctly, because despite it pass the tests, this tool interact
with the rest of the profile/system during execution.
> 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 improved the description of the package. Previously I cited
"artifacts", but this term is not 100% correct, because the tool can be
used for producing many type of target files. It is more a
build/automation tool for rebuilding the chain of dependencies. For
example I'm using it for updating websites and so on.
So, it is not in competition with Guix, or other deterministic build
systems. It is a mix between a normal scripts, but with annotations for
having incremental updates. A corresponding Makefile would be a lot more
verbose and hard to comprehend, because it will use a bottom-up paradigm
in specifying dependencies.
Sadly, the main web-site of the tool is not describing it in a
marketing-free language, so I had to create a description from scratch.
> 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
Yes probably.
> 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.
DONE. I hope this time it will be better. But, it is the first time I'm
using it.
Regards,
Massimo
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.