diff mbox series

[bug#54216,v2] gnu: shepherd-for-guix: New package for use in Guix.

Message ID 20220301184517.13439-1-attila@lendvai.name
State New
Headers show
Series [bug#54216,v2] gnu: shepherd-for-guix: New package for use in Guix. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue

Commit Message

Attila Lendvai March 1, 2022, 6:45 p.m. UTC
This also updates shepherd-for-guix to the latest commit, so that the two
variants have a different version.

* gnu/packages/admin.scm (shepherd-for-guix): New variable.
---

v2: add the recommended comment and some edits to the other comments.

 gnu/packages/admin.scm    | 41 +++++++++++++++++++++++++++++++++++++++
 gnu/services/shepherd.scm |  2 +-
 2 files changed, 42 insertions(+), 1 deletion(-)

Comments

M March 1, 2022, 7:01 p.m. UTC | #1
Attila Lendvai schreef op di 01-03-2022 om 19:45 [+0100]:
> +       (origin
> +         (method git-fetch)
> +         (uri (git-reference
> +               ;; Build from git and add Autotools inputs, to make developing
> +               ;; Shepherd easier. It enables easier package inheritance.
> +               (url "https://git.savannah.gnu.org/git/shepherd.git/")
> +               (commit commit)))
> +         (sha256
> +          (base32
> +           "1hgkbl3fyzwi5vw63kbkswnf3viyfl52c5dzkx7vbkr4sj5ysz1g"))
> +         (modules '((guix build utils)))
> +         (snippet
> +          '(begin
> +             ;; Build with -O1 to work around <https://bugs.gnu.org/48368>.
> +             (substitute* "Makefile.am"
> +               (("compile --target")
> +                "compile -O1 --target")))))

This origin record can be simplified a bit by using inheritance:

(origin
  (inherit (package-source shepherd))
  (method git-fetch)
  (uri (git-reference [...]))
  (sha256 [...]))

'inherit' is not restricted to packages, it can be used for any record
type defined with (guix records)!

Greetings,
Maxime.
Attila Lendvai March 1, 2022, 7:27 p.m. UTC | #2
> This origin record can be simplified a bit by using inheritance:
>
> (origin
> (inherit (package-source shepherd))
> (method git-fetch)
> (uri (git-reference [...]))
> (sha256 [...]))
>
> 'inherit' is not restricted to packages, it can be used for any record
> type defined with (guix records)!


oh, excellent point, thanks Maxime!

unfortunately, it won't help us much here, because one snippet modifies
Makefile.am, while the other Makefile.in.

once it's merged i'm planning to send another patch to staging that will clean
this up a little by building normal shepherd also from git. that patch will
include this inheritance.

--
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
The truth cannot be told, only realized.
Simon Tournier March 2, 2022, 9:14 a.m. UTC | #3
Hi Attila,

On Tue, 1 Mar 2022 at 20:42, Attila Lendvai <attila@lendvai.name> wrote:

> unfortunately, it won't help us much here, because one snippet modifies
> Makefile.am, while the other Makefile.in.

Guix does not have a clear line for patching (or I am not aware of the
update); what is going to source+snippet vs arguments+phase.  Pros and
cons for both; basically the question is what "guix build --source"
should return?

Option source+snippet means it returns the source of what Guix really
builds -- so many packages would not respect this rule of thumb.

Option arguments+phase means it returns the real unmodified upstream
source (modulo removal of non-free) -- so "guix shell -D foo" would
break for many packages.

Difficult tension. :-)  For most cases, not an unique answer; maybe
that's why we do not have a clear documentation. :-)  I just mention
it i.e., I am not saying you can do something. :-)  I am simply
pointing that Guix does not have a clear recommendation /
documentation where the patches should go; probably depending on their
nature.  Well, nothing related with your patch. :-)

That's said, personally, in this case, instead of having the Makefile*
patch in 'source', I would do the patching using a phase.

Cheers,
simon
Leo Famulari March 2, 2022, 11:50 p.m. UTC | #4
Origin snippets should only be used to remove nonfree things from the upstream source code. All other changes should use patch files or a build phase.

On Wed, Mar 2, 2022, at 04:14, zimoun wrote:
> Hi Attila,
>
> On Tue, 1 Mar 2022 at 20:42, Attila Lendvai <attila@lendvai.name> wrote:
>
>> unfortunately, it won't help us much here, because one snippet modifies
>> Makefile.am, while the other Makefile.in.
>
> Guix does not have a clear line for patching (or I am not aware of the
> update); what is going to source+snippet vs arguments+phase.  Pros and
> cons for both; basically the question is what "guix build --source"
> should return?
>
> Option source+snippet means it returns the source of what Guix really
> builds -- so many packages would not respect this rule of thumb.
>
> Option arguments+phase means it returns the real unmodified upstream
> source (modulo removal of non-free) -- so "guix shell -D foo" would
> break for many packages.
>
> Difficult tension. :-)  For most cases, not an unique answer; maybe
> that's why we do not have a clear documentation. :-)  I just mention
> it i.e., I am not saying you can do something. :-)  I am simply
> pointing that Guix does not have a clear recommendation /
> documentation where the patches should go; probably depending on their
> nature.  Well, nothing related with your patch. :-)
>
> That's said, personally, in this case, instead of having the Makefile*
> patch in 'source', I would do the patching using a phase.
>
> Cheers,
> simon
M March 3, 2022, 6:25 a.m. UTC | #5
Leo Famulari schreef op wo 02-03-2022 om 18:50 [-0500]:
> Origin snippets should only be used to remove nonfree things
> from the upstream source code. All other changes should use
> patch files or a build phase.

Why?  If it's a source code change and it fits an origin snippet,
why not an origin snippet?  Why would the source in Guix need to match
the source upstream?

Also, in this case, it was just copied from the 'shepherd' packages
with some slight adjustments.

Greetings,
Maxime.
Simon Tournier March 3, 2022, 8:48 a.m. UTC | #6
Hi Maxime,

On Thu, 3 Mar 2022 at 07:25, Maxime Devos <maximedevos@telenet.be> wrote:
> Leo Famulari schreef op wo 02-03-2022 om 18:50 [-0500]:

> > Origin snippets should only be used to remove nonfree things
> > from the upstream source code. All other changes should use
> > patch files or a build phase.
>
> Why?  If it's a source code change and it fits an origin snippet,
> why not an origin snippet?  Why would the source in Guix need to match
> the source upstream?

Because as I tried to explain here [1] ;-)

1: <https://issues.guix.gnu.org/54216#5>

a) the location of patches depending of their nature is **not well-documented**,
b) because it is a tension between two intentions,
c) and thus we never take the time to clearly document for consistency.

FWIW, it would be unfair for the patch to have the discussion here.

As Leo, I also think this source modification should go to a phase.


> Also, in this case, it was just copied from the 'shepherd' packages
> with some slight adjustments.

And I also think this recent commit
79be6a985799adc6d663890250f4fb7c12f015b4 introducing the 'snippet' is
unfortunate.  And it should be fixed, i.e., move the substitution to a
phase.


Cheers,
simon
M March 3, 2022, 2:36 p.m. UTC | #7
zimoun schreef op wo 02-03-2022 om 10:14 [+0100]:
> Hi Attila,
> 
> On Tue, 1 Mar 2022 at 20:42, Attila Lendvai <attila@lendvai.name> wrote:
> 
> > unfortunately, it won't help us much here, because one snippet modifies
> > Makefile.am, while the other Makefile.in.
> 
> Guix does not have a clear line for patching (or I am not aware of the
> update); what is going to source+snippet vs arguments+phase.  Pros and
> cons for both; basically the question is what "guix build --source"
> should return?
> 
> Option source+snippet means it returns the source of what Guix really
> builds

Seems like a pro for source+snippet (or source+patch, though that
would be a bit more verbose) to me.

>  -- so many packages would not respect this rule of thumb.

In that case, it seems like there are plenty of package definitions to
improve!

> Option arguments+phase means it returns the real unmodified upstream
> source (modulo removal of non-free) -- so "guix shell -D foo" would
> break for many packages.

I assume you meant "guix build --source foo"?
This seems like a con for "phases+arguments" to me.
Sometimes, to hack on software, I download the source code with
"guix build --source the-package", unpack it and do
"guix shell -D the-package".

I don't see much value in returning the unmodified upstream source.
Especially since in this case the modified source fixes a bug
(well, works-around a Guile bug). As long as it's source code, it
builds, it doesn't do things like bundling, including binaries or
non-free things, and it avoids being Guix-specific and fixes known
bugs, it seems good source code to me.

Also, most packages don't modify upstream code, so I don't see
the ‘would break for many packages’ here ...

> Difficult tension. :-)

As implied from my explanations above, I don't see any tension here.

> That's said, personally, in this case, instead of having the Makefile*
> patch in 'source', I would do the patching using a phase.

It's ‘merely’ setting some compilation flags, so maybe.
Both options seem fine to me here but I don't see a point to _moving_
from the snippet-shed to phase-shed.


Greetings,
Maxime.
Simon Tournier March 3, 2022, 2:51 p.m. UTC | #8
Hi Maxime,

You are doing the discussion and we should stay focus on this patch.
:-)  Leo and me are considering better to have the substitution in a
phase, FWIW, and Attila submitted a new version.  Let's move on, again
for what my opinion is worth here.

I am totally fine to have the discussion on guix-devel or elsewhere --
I disagree on some points but I will not comment here; it appears to
me not fair to pollute by lengthy arguments the nice patch by Attila,
IMHO.


Cheers,
simon
Leo Famulari March 5, 2022, 9:13 p.m. UTC | #9
On Thu, Mar 03, 2022 at 07:25:22AM +0100, Maxime Devos wrote:
> Leo Famulari schreef op wo 02-03-2022 om 18:50 [-0500]:
> > Origin snippets should only be used to remove nonfree things
> > from the upstream source code. All other changes should use
> > patch files or a build phase.
> 
> Why?  If it's a source code change and it fits an origin snippet,
> why not an origin snippet?  Why would the source in Guix need to match
> the source upstream?

`guix build --source` is a tool to provide freely licensed source code
to be used for any purpose, including building on systems besides Guix.

Using the Guix tools, there is no way to access the upstream source code
without applying the snippets. The reason for that is that the origin
snippet mechanism was introduced specifically to remove non-free
components without making it easy to reverse the transformation.

Compare that to patch files, which are easily reversed, and build
phases, which do not apply to `guix build --source`.

So, we have to be careful when using snippets, to ensure that the result
of `guix build --source` is useful on any system, not just Guix.

More info:
https://guix.gnu.org/manual/en/html_node/Snippets-versus-Phases.html

Please let me know if these guidelines are still unclear.
M March 5, 2022, 9:50 p.m. UTC | #10
Leo Famulari schreef op za 05-03-2022 om 16:13 [-0500]:
> [...]

Replied to on guix-devel@ (see ‘gnu: shepherd: patch, snippet or
phase’).
diff mbox series

Patch

diff --git a/gnu/packages/admin.scm b/gnu/packages/admin.scm
index c8f91aab0d..f43526b7d9 100644
--- a/gnu/packages/admin.scm
+++ b/gnu/packages/admin.scm
@@ -307,6 +307,47 @@  (define-public shepherd
     (license license:gpl3+)
     (home-page "https://www.gnu.org/software/shepherd/")))
 
+;; This is the Shepherd package used by Guix.  This package variant
+;; allows us to add new features and fix bugs in Shepherd and use the
+;; new features in Guix, without having to go through the 'staging'
+;; branch, and without having to wait for a new release, see
+;; [TODO] in the manual.
+(define-public shepherd-for-guix
+  (let* ((version "0.8.1")
+         ;; If it's an unreleased commit:
+         (commit "7c380590164ea8ee40de46059d07e08a48963577")
+         ;; Use the below form if it's a release, and also set REVISION to #f.
+         ;; (commit (string-append "v" version))
+         (revision "1"))
+    (package
+      (inherit shepherd)
+      (source
+       (origin
+         (method git-fetch)
+         (uri (git-reference
+               ;; Build from git and add Autotools inputs, to make developing
+               ;; Shepherd easier. It enables easier package inheritance.
+               (url "https://git.savannah.gnu.org/git/shepherd.git/")
+               (commit commit)))
+         (sha256
+          (base32
+           "1hgkbl3fyzwi5vw63kbkswnf3viyfl52c5dzkx7vbkr4sj5ysz1g"))
+         (modules '((guix build utils)))
+         (snippet
+          '(begin
+             ;; Build with -O1 to work around <https://bugs.gnu.org/48368>.
+             (substitute* "Makefile.am"
+               (("compile --target")
+                "compile -O1 --target"))))))
+      (version (if revision
+                   (git-version version revision commit)
+                   version))
+      (native-inputs
+       (modify-inputs (package-native-inputs shepherd)
+         (prepend autoconf automake gettext-minimal help2man texinfo)))
+      (description "A package variant for use in Guix. It helps lowering
+the build time of Guix when working on Shepherd."))))
+
 (define-public guile2.2-shepherd
   (package
     (inherit shepherd)
diff --git a/gnu/services/shepherd.scm b/gnu/services/shepherd.scm
index b44dbf9d9f..991194ffe6 100644
--- a/gnu/services/shepherd.scm
+++ b/gnu/services/shepherd.scm
@@ -88,7 +88,7 @@  (define-record-type* <shepherd-configuration>
   shepherd-configuration make-shepherd-configuration
   shepherd-configuration?
   (shepherd shepherd-configuration-shepherd
-            (default shepherd)) ; file-like
+            (default shepherd-for-guix)) ; file-like
   (services shepherd-configuration-services
             (default '()))) ; list of <shepherd-service>