[bug#65351,v2,1/7] gnu: cgit: Make git-source a native input.

* gnu/packages/version-control.scm (cgit)[inputs]: Move git-source to ...
[native-inputs]: ... here.
---
 gnu/packages/version-control.scm | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Am Montag, dem 04.09.2023 um 10:02 +0100 schrieb Arun Isaac:
> * gnu/packages/version-control.scm (cgit)[inputs]: Move git-source to
> ...
> [native-inputs]: ... here.
> ---
>  gnu/packages/version-control.scm | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/gnu/packages/version-control.scm b/gnu/packages/version-
> control.scm
> index 86dcea908c..c977da468d 100644
> --- a/gnu/packages/version-control.scm
> +++ b/gnu/packages/version-control.scm
> @@ -21,7 +21,7 @@
>  ;;; Copyright © 2018 Sou Bunnbu <iyzsong@member.fsf.org>
>  ;;; Copyright © 2018 Christopher Baines <mail@cbaines.net>
>  ;;; Copyright © 2018 Timothy Sample <samplet@ngyro.com>
> -;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
> +;;; Copyright © 2018, 2023 Arun Isaac <arunisaac@systemreboot.net>
>  ;;; Copyright © 2019 Jovany Leandro G.C <bit4bit@riseup.net>
>  ;;; Copyright © 2019 Kei Kebreau <kkebreau@posteo.net>
>  ;;; Copyright © 2019, 2020 Alex Griffin <a@ajgrf.com>
> @@ -1104,9 +1104,6 @@ (define-public cgit
>                  "html-converters/md2html"))
>               #t)))))
>      (native-inputs
> -     ;; For building manpage.
> -     (list asciidoc gzip bzip2 xz))
> -    (inputs
>       `(;; Building cgit requires a Git source tree.
>         ("git-source"
>          ,(origin
> @@ -1116,13 +1113,15 @@ (define-public cgit
>             (uri "mirror://kernel.org/software/scm/git/git-
> 2.25.4.tar.xz")
>             (sha256
>              (base32
> "11am6s46wmn1yll5614smjhzlghbqq6gysgcs64igjr9y5wzpdxq"))))
> -       ("openssl" ,openssl)
> -       ("groff" ,groff)
> -       ("python" ,python)
> -       ("python-docutils" ,python-docutils)
> -       ("python-markdown" ,python-markdown)
> -       ("python-pygments" ,python-pygments)
> -       ("zlib" ,zlib)))
> +       ;; For building manpage.
> +       ("asciidoc" ,asciidoc)
> +       ("gzip" ,gzip)
> +       ("bzip2" ,bzip2)
> +       ("xz" ,xz)))
> +    (inputs
> +     (list openssl groff
> +           python python-docutils python-markdown python-pygments
> +           zlib))
This patch is broken and it doesn't appear to get fixed in 4/7.  If you
want git-source to be in native-inputs, you need to use (or native-
inputs inputs).

Most of these patches are not really atomic, depending on each other to
make sense.  I'd instead possibly do the following:
3
[1, 2, 4, 5] squashed into a single commit
6
7

In 7, you might want to use /bin/sh if the compatibility is meant for
stuff that actually lands in the store.

Cheers

Hi Liliana,

Sorry for the long delay! Life got in the way.

> This patch is broken and it doesn't appear to get fixed in 4/7.  If you
> want git-source to be in native-inputs, you need to use (or native-
> inputs inputs).

I'm not sure what you mean. I am able to build both cgit and cgit-pink
on my machine. The QA system also agrees with
me. https://qa.guix.gnu.org/issue/65351

> Most of these patches are not really atomic, depending on each other to
> make sense.  I'd instead possibly do the following:
> 3
> [1, 2, 4, 5] squashed into a single commit
> 6
> 7

I disagree. I do think these commits are atomic. Each one of these
commits fixes a single independent problem with cgit that has no
relevance to cgit-pink. Finally, the last commit adds cgit-pink. Note
that there are already many commits in guix along similar lines. So,
there is precedent.

* gnu: cgit: Make git-source a native input.
* gnu: cgit: Make bzip2, gzip and xz inputs.
* gnu: cgit: Do not return #t from custom phases.
* gnu: cgit: Use G-expressions.
* gnu: cgit: Add bash-minimal to inputs.
* gnu: cgit: Use cc-for-target.
* cgit-pink gnu: Add cgit-pink.

> In 7, you might want to use /bin/sh if the compatibility is meant for
> stuff that actually lands in the store.

/bin/sh doesn't work. I have tried. If I remember correctly, this is to
do with how the SHELL_PATH gets substituted into generated scripts.

Cheers!

Am Donnerstag, dem 28.09.2023 um 08:12 +0100 schrieb Arun Isaac:
> 
> Hi Liliana,
> 
> Sorry for the long delay! Life got in the way.
> 
> > This patch is broken and it doesn't appear to get fixed in 4/7.  If
> > you want git-source to be in native-inputs, you need to use (or
> > native-inputs inputs).
> 
> I'm not sure what you mean. I am able to build both cgit and cgit-
> pink on my machine. The QA system also agrees with
> me. https://qa.guix.gnu.org/issue/65351
CI doesn't do cross-builds, it does simulated native builds, so you
won't see the issue.

> > Most of these patches are not really atomic, depending on each
> > other to make sense.  I'd instead possibly do the following:
> > 3
> > [1, 2, 4, 5] squashed into a single commit
> > 6
> > 7
> 
> I disagree. I do think these commits are atomic. Each one of these
> commits fixes a single independent problem with cgit that has no
> relevance to cgit-pink. Finally, the last commit adds cgit-pink. Note
> that there are already many commits in guix along similar lines. So,
> there is precedent.
> 
> * gnu: cgit: Make git-source a native input.
> * gnu: cgit: Make bzip2, gzip and xz inputs.
> * gnu: cgit: Do not return #t from custom phases.
> * gnu: cgit: Use G-expressions.
> * gnu: cgit: Add bash-minimal to inputs.
> * gnu: cgit: Use cc-for-target.
> * cgit-pink gnu: Add cgit-pink.
Indeed, there is precedent.  However, when moving stuff around, such as
in 1, you also need to update the dependent stuff, which you haven't
done and which also causes more noise along the line.  In this series,
you need three commits to basically get input handling "correct".  The
intermediate commits are duds in terms of guix time-machine, which
isn't great.

> > In 7, you might want to use /bin/sh if the compatibility is meant
> > for stuff that actually lands in the store.
> 
> /bin/sh doesn't work. I have tried. If I remember correctly, this is
> to do with how the SHELL_PATH gets substituted into generated
> scripts.
For the record, what kind of generated scripts are we talking here? 
Invoked at build time or sent to the store?

Cheers

Hi Liliana,

>> > This patch is broken and it doesn't appear to get fixed in 4/7.  If
>> > you want git-source to be in native-inputs, you need to use (or
>> > native-inputs inputs).
>> 
>> I'm not sure what you mean. I am able to build both cgit and cgit-
>> pink on my machine. The QA system also agrees with
>> me. https://qa.guix.gnu.org/issue/65351
> CI doesn't do cross-builds, it does simulated native builds, so you
> won't see the issue.

Ah, I see what you mean now. I have deleted the patch making git-source a
native input. I guess it's ok to leave it as an input since it doesn't
really matter in this case.

> Indeed, there is precedent.  However, when moving stuff around, such as
> in 1, you also need to update the dependent stuff, which you haven't
> done and which also causes more noise along the line.  In this series,
> you need three commits to basically get input handling "correct".  The
> intermediate commits are duds in terms of guix time-machine, which
> isn't great.

I have reworked the patchset into only 3 patches now. Let me know if
this is ok.

>> > In 7, you might want to use /bin/sh if the compatibility is meant
>> > for stuff that actually lands in the store.
>> 
>> /bin/sh doesn't work. I have tried. If I remember correctly, this is
>> to do with how the SHELL_PATH gets substituted into generated
>> scripts.
> For the record, what kind of generated scripts are we talking here? 
> Invoked at build time or sent to the store?

The cgit Makefiles are tightly coupled with git source code and the git
Makefiles. The generated scripts are wrapper scripts for git binaries
like git-shell, git-upload-archive, git-upload-pack, etc. As far as I
can tell, cgit does not use them at all. They are neither invoked at
build time nor are they sent to the store. They are only invoked at test
time, and that fails if you have SHELL_PATH=sh. This specifically
bothers only cgit-pink and not cgit since cgit does not run the test
suite.

SHELL_PATH is also directly executed as a shell in some parts of the
Makefile. For example,

--8<---------------cut here---------------start------------->8---
GIT-VERSION-FILE: FORCE
	@$(SHELL_PATH) ./GIT-VERSION-GEN
--8<---------------cut here---------------end--------------->8---

and

--8<---------------cut here---------------start------------->8---
config-list.h: Documentation/*config.txt Documentation/config/*.txt
	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh >$@
--8<---------------cut here---------------end--------------->8---

That's why SHELL_PATH=/bin/sh fails since /bin/sh does not exist in the
build environment.

A v3 patchset follows.

Cheers!