unofficial mirror of guix-devel@gnu.org 
 help / color / mirror / code / Atom feed
* [PATCH 3/4] gnu: libcanberra: Add propagated-input.
@ 2014-12-18 21:14 Federico Beffa
  2014-12-21 11:06 ` Ludovic Courtès
  0 siblings, 1 reply; 14+ messages in thread
From: Federico Beffa @ 2014-12-18 21:14 UTC (permalink / raw)
  To: Guix-devel

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

I propose to make sound-theme-freedesktop a propagated input of
libcanberra. This is because, according to the XDG sound theme
specification, those event sounds should always be present and used as
fall-back in case other sounds are not present.

http://www.freedesktop.org/wiki/Specifications/sound-theme-spec/

Regards,
Fede

[-- Attachment #2: 0003-gnu-libcanberra-Add-propagated-input.patch --]
[-- Type: text/x-patch, Size: 1105 bytes --]

From 72daec908a2f790d73e332c680433734125c5eee Mon Sep 17 00:00:00 2001
From: Federico Beffa <beffa@fbengineering.ch>
Date: Thu, 18 Dec 2014 21:32:34 +0100
Subject: [PATCH 3/4] gnu: libcanberra: Add propagated-input.

* gnu/packages/libcanberra.scm (libcanberra): Add 'sound-theme-freedesktop' as
  a propagated-input.
---
 gnu/packages/libcanberra.scm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gnu/packages/libcanberra.scm b/gnu/packages/libcanberra.scm
index 764c327..24a6a0b 100644
--- a/gnu/packages/libcanberra.scm
+++ b/gnu/packages/libcanberra.scm
@@ -58,6 +58,11 @@
        ("udev" ,eudev)))
     (native-inputs
      `(("pkg-config" ,pkg-config)))
+    ;; This is the default and fall-back sound theme for XDG desktips and
+    ;; should always be present.
+    ;; http://www.freedesktop.org/wiki/Specifications/sound-theme-spec/
+    (propagated-inputs
+     `(("sound-theme-freedesktop" ,sound-theme-freedesktop)))
     (home-page "http://0pointer.de/lennart/projects/libcanberra/")
     (synopsis
      "Implementation of the XDG Sound Theme and Name Specifications")
-- 
1.8.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2014-12-18 21:14 [PATCH 3/4] gnu: libcanberra: Add propagated-input Federico Beffa
@ 2014-12-21 11:06 ` Ludovic Courtès
  2014-12-21 15:33   ` Federico Beffa
  0 siblings, 1 reply; 14+ messages in thread
From: Ludovic Courtès @ 2014-12-21 11:06 UTC (permalink / raw)
  To: Federico Beffa; +Cc: Guix-devel

Federico Beffa <beffa@ieee.org> skribis:

> I propose to make sound-theme-freedesktop a propagated input of
> libcanberra. This is because, according to the XDG sound theme
> specification, those event sounds should always be present and used as
> fall-back in case other sounds are not present.
>
> http://www.freedesktop.org/wiki/Specifications/sound-theme-spec/

That’s not the right fix, I think.  For instance, if Evince is installed
in a profile, but libcanberra itself is not in the profile, then the
sound theme is not pulled and ends up not being used.

Would it be possible, instead, to patch libcanberra to refer to the
sound-theme directory as its fallback?

Thanks,
Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2014-12-21 11:06 ` Ludovic Courtès
@ 2014-12-21 15:33   ` Federico Beffa
  2015-01-07 17:20     ` Federico Beffa
  2015-01-07 20:11     ` Ludovic Courtès
  0 siblings, 2 replies; 14+ messages in thread
From: Federico Beffa @ 2014-12-21 15:33 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: Guix-devel

On Sun, Dec 21, 2014 at 12:06 PM, Ludovic Courtès <ludo@gnu.org> wrote:
> Federico Beffa <beffa@ieee.org> skribis:
>
>> I propose to make sound-theme-freedesktop a propagated input of
>> libcanberra. This is because, according to the XDG sound theme
>> specification, those event sounds should always be present and used as
>> fall-back in case other sounds are not present.
>>
>> http://www.freedesktop.org/wiki/Specifications/sound-theme-spec/
>
> That’s not the right fix, I think.  For instance, if Evince is installed
> in a profile, but libcanberra itself is not in the profile, then the
> sound theme is not pulled and ends up not being used.
>
> Would it be possible, instead, to patch libcanberra to refer to the
> sound-theme directory as its fallback?

The location of the sound theme is specified, among other things, by
the variable XDG_DATA_DIRS. So, if an application makes use of the
glib-or-gtk-build-system and has the sounds as inputs, then it should
find them.  I don't think we need to patch libcanberra in any way.

With my suggestion I was trying to avoid having to specify
sound-theme-freedesktop in addition to libcanberra in every gtk
application (as, e.g., evince).

If we make libcanberra a propagated-input of applications like evince,
then they would automatically know the location of the sounds (by the
inheritance of propagated inputs).

>
> Thanks,
> Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2014-12-21 15:33   ` Federico Beffa
@ 2015-01-07 17:20     ` Federico Beffa
  2015-01-07 20:11     ` Ludovic Courtès
  1 sibling, 0 replies; 14+ messages in thread
From: Federico Beffa @ 2015-01-07 17:20 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: Guix-devel

On Sun, Dec 21, 2014 at 4:33 PM, Federico Beffa <beffa@ieee.org> wrote:
> On Sun, Dec 21, 2014 at 12:06 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>> Federico Beffa <beffa@ieee.org> skribis:
>>
>>> I propose to make sound-theme-freedesktop a propagated input of
>>> libcanberra. This is because, according to the XDG sound theme
>>> specification, those event sounds should always be present and used as
>>> fall-back in case other sounds are not present.
>>>
>>> http://www.freedesktop.org/wiki/Specifications/sound-theme-spec/
>>
>> That’s not the right fix, I think.  For instance, if Evince is installed
>> in a profile, but libcanberra itself is not in the profile, then the
>> sound theme is not pulled and ends up not being used.
>>
>> Would it be possible, instead, to patch libcanberra to refer to the
>> sound-theme directory as its fallback?
>
> The location of the sound theme is specified, among other things, by
> the variable XDG_DATA_DIRS. So, if an application makes use of the
> glib-or-gtk-build-system and has the sounds as inputs, then it should
> find them.  I don't think we need to patch libcanberra in any way.
>
> With my suggestion I was trying to avoid having to specify
> sound-theme-freedesktop in addition to libcanberra in every gtk
> application (as, e.g., evince).
>
> If we make libcanberra a propagated-input of applications like evince,
> then they would automatically know the location of the sounds (by the
> inheritance of propagated inputs).

Not sure we concluded on this. Is my intention clear? Does my previous
answer address your point?

Regards,
Fede

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2014-12-21 15:33   ` Federico Beffa
  2015-01-07 17:20     ` Federico Beffa
@ 2015-01-07 20:11     ` Ludovic Courtès
  2015-01-08 17:08       ` Federico Beffa
  1 sibling, 1 reply; 14+ messages in thread
From: Ludovic Courtès @ 2015-01-07 20:11 UTC (permalink / raw)
  To: Federico Beffa; +Cc: Guix-devel

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

(Sorry for the delay.)

Federico Beffa <beffa@ieee.org> skribis:

> On Sun, Dec 21, 2014 at 12:06 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>> Federico Beffa <beffa@ieee.org> skribis:
>>
>>> I propose to make sound-theme-freedesktop a propagated input of
>>> libcanberra. This is because, according to the XDG sound theme
>>> specification, those event sounds should always be present and used as
>>> fall-back in case other sounds are not present.
>>>
>>> http://www.freedesktop.org/wiki/Specifications/sound-theme-spec/
>>
>> That’s not the right fix, I think.  For instance, if Evince is installed
>> in a profile, but libcanberra itself is not in the profile, then the
>> sound theme is not pulled and ends up not being used.
>>
>> Would it be possible, instead, to patch libcanberra to refer to the
>> sound-theme directory as its fallback?
>
> The location of the sound theme is specified, among other things, by
> the variable XDG_DATA_DIRS. So, if an application makes use of the
> glib-or-gtk-build-system and has the sounds as inputs, then it should
> find them.  I don't think we need to patch libcanberra in any way.
>
> With my suggestion I was trying to avoid having to specify
> sound-theme-freedesktop in addition to libcanberra in every gtk
> application (as, e.g., evince).
>
> If we make libcanberra a propagated-input of applications like evince,
> then they would automatically know the location of the sounds (by the
> inheritance of propagated inputs).

OK, I understand the plan.

What I had in mind was to instead add sound-theme-freedesktop as an
input to libcanberra, and to patch libcanberra along these untested
lines:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 521 bytes --]

--- libcanberra-0.30/src/sound-theme-spec.c~	2010-02-20 00:39:40.000000000 +0100
+++ libcanberra-0.30/src/sound-theme-spec.c	2015-01-07 21:08:51.029841980 +0100
@@ -69,8 +69,7 @@ int ca_get_data_home(char **e) {
         else if ((env = getenv("HOME")) && *env == '/')
                 subdir = "/.local/share";
         else {
-                *e = NULL;
-                return CA_SUCCESS;
+		subdir = "/gnu/store/...-sound-theme/share";
         }
 
         if (!(r = ca_new(char, strlen(env) + strlen(subdir) + 1)))

[-- Attachment #3: Type: text/plain, Size: 114 bytes --]


I believe this is the simplest approach, and one that is likely to
always work.

WDYT?

Thanks,
Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2015-01-07 20:11     ` Ludovic Courtès
@ 2015-01-08 17:08       ` Federico Beffa
  2015-01-08 20:39         ` Ludovic Courtès
  0 siblings, 1 reply; 14+ messages in thread
From: Federico Beffa @ 2015-01-08 17:08 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: Guix-devel

On Wed, Jan 7, 2015 at 9:11 PM, Ludovic Courtès <ludo@gnu.org> wrote:
> (Sorry for the delay.)
>
> Federico Beffa <beffa@ieee.org> skribis:
>
>> On Sun, Dec 21, 2014 at 12:06 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>> Federico Beffa <beffa@ieee.org> skribis:
>>>
>>>> I propose to make sound-theme-freedesktop a propagated input of
>>>> libcanberra. This is because, according to the XDG sound theme
>>>> specification, those event sounds should always be present and used as
>>>> fall-back in case other sounds are not present.
>>>>
>>>> http://www.freedesktop.org/wiki/Specifications/sound-theme-spec/
>>>
>>> That’s not the right fix, I think.  For instance, if Evince is installed
>>> in a profile, but libcanberra itself is not in the profile, then the
>>> sound theme is not pulled and ends up not being used.
>>>
>>> Would it be possible, instead, to patch libcanberra to refer to the
>>> sound-theme directory as its fallback?
>>
>> The location of the sound theme is specified, among other things, by
>> the variable XDG_DATA_DIRS. So, if an application makes use of the
>> glib-or-gtk-build-system and has the sounds as inputs, then it should
>> find them.  I don't think we need to patch libcanberra in any way.
>>
>> With my suggestion I was trying to avoid having to specify
>> sound-theme-freedesktop in addition to libcanberra in every gtk
>> application (as, e.g., evince).
>>
>> If we make libcanberra a propagated-input of applications like evince,
>> then they would automatically know the location of the sounds (by the
>> inheritance of propagated inputs).
>
> OK, I understand the plan.
>
> What I had in mind was to instead add sound-theme-freedesktop as an
> input to libcanberra, and to patch libcanberra along these untested
> lines:
>
>
>
> I believe this is the simplest approach, and one that is likely to
> always work.

In my opinion we should only patch software where we have no
alternative which here is not the case.

This is because a patch will not necessarily apply to future versions
of a piece of software and makes maintenance more difficult and
(requiring manual intervention) more laborious.

Regards,
Fede

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2015-01-08 17:08       ` Federico Beffa
@ 2015-01-08 20:39         ` Ludovic Courtès
  2015-01-10 11:15           ` Federico Beffa
  0 siblings, 1 reply; 14+ messages in thread
From: Ludovic Courtès @ 2015-01-08 20:39 UTC (permalink / raw)
  To: Federico Beffa; +Cc: Guix-devel

Federico Beffa <beffa@ieee.org> skribis:

> On Wed, Jan 7, 2015 at 9:11 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>> (Sorry for the delay.)
>>
>> Federico Beffa <beffa@ieee.org> skribis:
>>
>>> On Sun, Dec 21, 2014 at 12:06 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>>> Federico Beffa <beffa@ieee.org> skribis:
>>>>
>>>>> I propose to make sound-theme-freedesktop a propagated input of
>>>>> libcanberra. This is because, according to the XDG sound theme
>>>>> specification, those event sounds should always be present and used as
>>>>> fall-back in case other sounds are not present.
>>>>>
>>>>> http://www.freedesktop.org/wiki/Specifications/sound-theme-spec/
>>>>
>>>> That’s not the right fix, I think.  For instance, if Evince is installed
>>>> in a profile, but libcanberra itself is not in the profile, then the
>>>> sound theme is not pulled and ends up not being used.
>>>>
>>>> Would it be possible, instead, to patch libcanberra to refer to the
>>>> sound-theme directory as its fallback?
>>>
>>> The location of the sound theme is specified, among other things, by
>>> the variable XDG_DATA_DIRS. So, if an application makes use of the
>>> glib-or-gtk-build-system and has the sounds as inputs, then it should
>>> find them.  I don't think we need to patch libcanberra in any way.
>>>
>>> With my suggestion I was trying to avoid having to specify
>>> sound-theme-freedesktop in addition to libcanberra in every gtk
>>> application (as, e.g., evince).
>>>
>>> If we make libcanberra a propagated-input of applications like evince,
>>> then they would automatically know the location of the sounds (by the
>>> inheritance of propagated inputs).
>>
>> OK, I understand the plan.
>>
>> What I had in mind was to instead add sound-theme-freedesktop as an
>> input to libcanberra, and to patch libcanberra along these untested
>> lines:

(It seems the MUA may have munged the patch that was supposed to be
here, see
<http://lists.gnu.org/archive/html/guix-devel/2015-01/msg00100.html>.)

>> I believe this is the simplest approach, and one that is likely to
>> always work.
>
> In my opinion we should only patch software where we have no
> alternative which here is not the case.

Yes, I agree on the general principle, and that’s always what we’ve been
trying to do.

That said, this kind of patch is not very different from the automated
shebang patching that we do, IMO.

The reason I’m not thrilled by the use of ‘propagated-inputs’ here is
that that’s not what they were designed for at the beginning.

It may also lead to usability problems.  Remember that when A is a
propagated input of B, installing B in a profile also installs A in that
profile.  So, if each GTKish package propagates sound-theme-freedesktop,
then installing several such packages in the same profile is likely to
lead to collisions just because the themes differ (those collisions may
be harmless, but they will at least trigger a bunch of warnings every
time the user operates on their profile, which is not desirable.)

WDYT?

Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2015-01-08 20:39         ` Ludovic Courtès
@ 2015-01-10 11:15           ` Federico Beffa
  2015-01-10 21:13             ` Ludovic Courtès
  0 siblings, 1 reply; 14+ messages in thread
From: Federico Beffa @ 2015-01-10 11:15 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: Guix-devel

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

On Thu, Jan 8, 2015 at 9:39 PM, Ludovic Courtès <ludo@gnu.org> wrote:
> That said, this kind of patch is not very different from the automated
> shebang patching that we do, IMO.

I do not see it this way because the shebang concept is fully
standardized, while here we are patching a "random" piece of code.

> The reason I’m not thrilled by the use of ‘propagated-inputs’ here is
> that that’s not what they were designed for at the beginning.
>
> It may also lead to usability problems.  Remember that when A is a
> propagated input of B, installing B in a profile also installs A in that
> profile.  So, if each GTKish package propagates sound-theme-freedesktop,
> then installing several such packages in the same profile is likely to
> lead to collisions just because the themes differ (those collisions may
> be harmless, but they will at least trigger a bunch of warnings every
> time the user operates on their profile, which is not desirable.)

Thanks for explaining your concern. But doesn't this problem exist
with most uses of propagated-inputs? As one example libssh2 propagates
libgcrypt. If the user installs a different version of libgcrypt (with
a possibly different ABI), doesn't this causes problems (required
version not found, or worse)?

In any case I've updated libcanberra with a patch to find the default
sounds without the need for propagated-inputs. Differently from your
suggested patch I'm proposing to add the default sounds store
directory to the code dealing with XDG_DATA_DIRS. This is because
XDG_DATA_HOME can only be a single directory and is inspected first.
XDG_DATA_DIRS can list an arbitrary number of directories and is only
inspected later. This is designed to allows the user to modify any
theme at his pleasure.

Regards,
Fede

[-- Attachment #2: 0003-gnu-libcanberra-Add-default-sounds-support.patch --]
[-- Type: text/x-patch, Size: 2480 bytes --]

From a1d7a5b9ed1392d8c5c1a8561357b72e48253ccf Mon Sep 17 00:00:00 2001
From: Federico Beffa <beffa@fbengineering.ch>
Date: Thu, 18 Dec 2014 21:32:34 +0100
Subject: [PATCH 3/4] gnu: libcanberra: Add default sounds support.

* gnu/packages/libcanberra.scm (libcanberra): Add input
  'sound-theme-freedesktop'.  Add phase 'patch-default-sounds-directory to
  patch the default sounds directory.
---
 gnu/packages/libcanberra.scm | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/libcanberra.scm b/gnu/packages/libcanberra.scm
index 764c327..91760d8 100644
--- a/gnu/packages/libcanberra.scm
+++ b/gnu/packages/libcanberra.scm
@@ -55,9 +55,36 @@
        ("libtool" ,libtool)
        ("libvorbis" ,libvorbis)
        ("pulseaudio" ,pulseaudio)
-       ("udev" ,eudev)))
+       ("udev" ,eudev)
+       ("sound-theme-freedesktop" ,sound-theme-freedesktop)))
     (native-inputs
      `(("pkg-config" ,pkg-config)))
+    ;; "sound-theme-freedesktop" is the default and fall-back sound theme for
+    ;; XDG desktops and should always be present.
+    ;; http://www.freedesktop.org/wiki/Specifications/sound-theme-spec/
+    ;; We make sure libcanberra will find it.
+    (arguments
+     `(#:phases 
+       (alist-cons-before
+        'build 'patch-default-sounds-directory
+        (lambda* (#:key inputs #:allow-other-keys)
+          (let ((sounds (string-append
+                         (assoc-ref inputs "sound-theme-freedesktop")
+                         "/share")))
+            (substitute* "src/sound-theme-spec.c"
+              (("return \"/usr/local/share:/usr/share\";")
+               (string-append "return \"" sounds "\";\n"
+                              "             else {\n"
+                              "                const char *stp = \":"
+                              sounds "\";\n"
+                              "                size_t len =  strlen(stp) + "
+                              "strlen(g) + 1;\n"
+                              "                "
+                              "char *g2 = (char*) malloc(len);\n"
+                              "                return "
+                              "strcat(strcpy(g2, g), stp);\n"
+                              "        }")))))
+          %standard-phases)))
     (home-page "http://0pointer.de/lennart/projects/libcanberra/")
     (synopsis
      "Implementation of the XDG Sound Theme and Name Specifications")
-- 
1.8.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2015-01-10 11:15           ` Federico Beffa
@ 2015-01-10 21:13             ` Ludovic Courtès
  2015-01-11  8:34               ` Federico Beffa
  0 siblings, 1 reply; 14+ messages in thread
From: Ludovic Courtès @ 2015-01-10 21:13 UTC (permalink / raw)
  To: Federico Beffa; +Cc: Guix-devel

Federico Beffa <beffa@ieee.org> skribis:

> On Thu, Jan 8, 2015 at 9:39 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>> That said, this kind of patch is not very different from the automated
>> shebang patching that we do, IMO.
>
> I do not see it this way because the shebang concept is fully
> standardized, while here we are patching a "random" piece of code.

Yeah, but it’s in the same “spirit”.  :-)

>> The reason I’m not thrilled by the use of ‘propagated-inputs’ here is
>> that that’s not what they were designed for at the beginning.
>>
>> It may also lead to usability problems.  Remember that when A is a
>> propagated input of B, installing B in a profile also installs A in that
>> profile.  So, if each GTKish package propagates sound-theme-freedesktop,
>> then installing several such packages in the same profile is likely to
>> lead to collisions just because the themes differ (those collisions may
>> be harmless, but they will at least trigger a bunch of warnings every
>> time the user operates on their profile, which is not desirable.)
>
> Thanks for explaining your concern. But doesn't this problem exist
> with most uses of propagated-inputs? As one example libssh2 propagates
> libgcrypt. If the user installs a different version of libgcrypt (with
> a possibly different ABI), doesn't this causes problems (required
> version not found, or worse)?

Yes, you’re right.  But the difference is that in the use case we’re
discussing here, we know it would systematically collide.

> In any case I've updated libcanberra with a patch to find the default
> sounds without the need for propagated-inputs. Differently from your
> suggested patch I'm proposing to add the default sounds store
> directory to the code dealing with XDG_DATA_DIRS. This is because
> XDG_DATA_HOME can only be a single directory and is inspected first.
> XDG_DATA_DIRS can list an arbitrary number of directories and is only
> inspected later. This is designed to allows the user to modify any
> theme at his pleasure.

Very good!  Could you add the story about XDG_DATA_DIRS
vs. XDG_DATA_HOME as a comment above the phase?

> +          (let ((sounds (string-append
> +                         (assoc-ref inputs "sound-theme-freedesktop")
> +                         "/share")))
> +            (substitute* "src/sound-theme-spec.c"
> +              (("return \"/usr/local/share:/usr/share\";")
> +               (string-append "return \"" sounds "\";\n"
> +                              "             else {\n"
> +                              "                const char *stp = \":"
> +                              sounds "\";\n"
> +                              "                size_t len =  strlen(stp) + "
> +                              "strlen(g) + 1;\n"
> +                              "                "
> +                              "char *g2 = (char*) malloc(len);\n"
> +                              "                return "
> +                              "strcat(strcpy(g2, g), stp);\n"
> +                              "        }")))))

Why not just:

  (string-append "return \"" sounds "\";")

?

Thanks,
Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2015-01-10 21:13             ` Ludovic Courtès
@ 2015-01-11  8:34               ` Federico Beffa
  2015-01-11 10:59                 ` Ludovic Courtès
  0 siblings, 1 reply; 14+ messages in thread
From: Federico Beffa @ 2015-01-11  8:34 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: Guix-devel

On Sat, Jan 10, 2015 at 10:13 PM, Ludovic Courtès <ludo@gnu.org> wrote:
> Very good!  Could you add the story about XDG_DATA_DIRS
> vs. XDG_DATA_HOME as a comment above the phase?

Will do.

>
>> +          (let ((sounds (string-append
>> +                         (assoc-ref inputs "sound-theme-freedesktop")
>> +                         "/share")))
>> +            (substitute* "src/sound-theme-spec.c"
>> +              (("return \"/usr/local/share:/usr/share\";")
>> +               (string-append "return \"" sounds "\";\n"
>> +                              "             else {\n"
>> +                              "                const char *stp = \":"
>> +                              sounds "\";\n"
>> +                              "                size_t len =  strlen(stp) + "
>> +                              "strlen(g) + 1;\n"
>> +                              "                "
>> +                              "char *g2 = (char*) malloc(len);\n"
>> +                              "                return "
>> +                              "strcat(strcpy(g2, g), stp);\n"
>> +                              "        }")))))
>
> Why not just:
>
>   (string-append "return \"" sounds "\";")
>
> ?

The first "return" is used in an "if" clause that checks that
XDG_DATA_DIRS is either empty, or not defined.  The variable "g" is a
pointer to a string contains the value of XDG_DATA_DIRS. I'm adding an
"else" with a second "return" to add the sounds directory even when
XDG_DATA_DIRS is defined (the unpatched program does returns "g" in
this case and therefore the sounds would not be found).

Regards,
Fede

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2015-01-11  8:34               ` Federico Beffa
@ 2015-01-11 10:59                 ` Ludovic Courtès
  2015-01-11 13:04                   ` Federico Beffa
  0 siblings, 1 reply; 14+ messages in thread
From: Ludovic Courtès @ 2015-01-11 10:59 UTC (permalink / raw)
  To: Federico Beffa; +Cc: Guix-devel

Federico Beffa <beffa@ieee.org> skribis:

> On Sat, Jan 10, 2015 at 10:13 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>> +          (let ((sounds (string-append
>>> +                         (assoc-ref inputs "sound-theme-freedesktop")
>>> +                         "/share")))
>>> +            (substitute* "src/sound-theme-spec.c"
>>> +              (("return \"/usr/local/share:/usr/share\";")
>>> +               (string-append "return \"" sounds "\";\n"
>>> +                              "             else {\n"
>>> +                              "                const char *stp = \":"
>>> +                              sounds "\";\n"
>>> +                              "                size_t len =  strlen(stp) + "
>>> +                              "strlen(g) + 1;\n"
>>> +                              "                "
>>> +                              "char *g2 = (char*) malloc(len);\n"
>>> +                              "                return "
>>> +                              "strcat(strcpy(g2, g), stp);\n"
>>> +                              "        }")))))
>>
>> Why not just:
>>
>>   (string-append "return \"" sounds "\";")
>>
>> ?
>
> The first "return" is used in an "if" clause that checks that
> XDG_DATA_DIRS is either empty, or not defined.  The variable "g" is a
> pointer to a string contains the value of XDG_DATA_DIRS. I'm adding an
> "else" with a second "return" to add the sounds directory even when
> XDG_DATA_DIRS is defined (the unpatched program does returns "g" in
> this case and therefore the sounds would not be found).

Hmm OK.

I’m a bit concerned about readability.  What about doing it this way:

  1. Make an actual patch for libcanberra, with @SOUND_THEME_DIRECTORY@
     as a placeholder for the sound-theme-freedesktop directory.

  2. Add this patch to libcanberra’s ‘patches’ field.

  3. Add a phase that replaces @SOUND_THEME_DIRECTORY@ with the actual
     directory.

?

Thanks,
Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2015-01-11 10:59                 ` Ludovic Courtès
@ 2015-01-11 13:04                   ` Federico Beffa
  2015-01-11 13:34                     ` Federico Beffa
  0 siblings, 1 reply; 14+ messages in thread
From: Federico Beffa @ 2015-01-11 13:04 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: Guix-devel

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

On Sun, Jan 11, 2015 at 11:59 AM, Ludovic Courtès <ludo@gnu.org> wrote:
> I’m a bit concerned about readability.  What about doing it this way:
>
>   1. Make an actual patch for libcanberra, with @SOUND_THEME_DIRECTORY@
>      as a placeholder for the sound-theme-freedesktop directory.
>
>   2. Add this patch to libcanberra’s ‘patches’ field.
>
>   3. Add a phase that replaces @SOUND_THEME_DIRECTORY@ with the actual
>      directory.
>
> ?

Good idea! Here the updated patch.

Thanks,
Fede

[-- Attachment #2: 0003-gnu-libcanberra-Add-default-sounds-support.patch --]
[-- Type: text/x-patch, Size: 4029 bytes --]

From e73c0f3c9eaf2e0824f6fa0eda482e0c8b8544bb Mon Sep 17 00:00:00 2001
From: Federico Beffa <beffa@fbengineering.ch>
Date: Thu, 18 Dec 2014 21:32:34 +0100
Subject: [PATCH 3/4] gnu: libcanberra: Add default sounds support.

* gnu/packages/libcanberra.scm (libcanberra): Add input
  'sound-theme-freedesktop'.  Add "libcanberra-sound-theme-freedesktop.patch"
  and related phase 'patch-default-sounds-directory to patch the default
  sounds directory.
---
 gnu/packages/libcanberra.scm                       | 31 ++++++++++++++++++++--
 .../libcanberra-sound-theme-freedesktop.patch      | 19 +++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100644 gnu/packages/patches/libcanberra-sound-theme-freedesktop.patch

diff --git a/gnu/packages/libcanberra.scm b/gnu/packages/libcanberra.scm
index 764c327..3d43c4f 100644
--- a/gnu/packages/libcanberra.scm
+++ b/gnu/packages/libcanberra.scm
@@ -19,6 +19,7 @@
 
 (define-module (gnu packages libcanberra)
   #:use-module ((guix licenses) #:select (lgpl2.1+))
+  #:use-module (gnu packages)
   #:use-module (guix packages)
   #:use-module (guix download)
   #:use-module (guix build-system gnu)
@@ -46,7 +47,21 @@
             version ".tar.xz"))
       (sha256
        (base32
-        "0wps39h8rx2b00vyvkia5j40fkak3dpipp1kzilqla0cgvk73dn2"))))
+        "0wps39h8rx2b00vyvkia5j40fkak3dpipp1kzilqla0cgvk73dn2"))
+      ;; "sound-theme-freedesktop" is the default and fall-back sound theme for
+      ;; XDG desktops and should always be present.
+      ;; http://www.freedesktop.org/wiki/Specifications/sound-theme-spec/
+      ;; We make sure libcanberra will find it.
+      ;;
+      ;; We add the default sounds store directory to the code dealing with
+      ;; XDG_DATA_DIRS and not XDG_DATA_HOME. This is because XDG_DATA_HOME
+      ;; can only be a single directory and is inspected first.  XDG_DATA_DIRS
+      ;; can list an arbitrary number of directories and is only inspected
+      ;; later.  This is designed to allows the user to modify any theme at
+      ;; his pleasure.
+      (patch-flags '("-p0"))
+      (patches
+       (list (search-patch "libcanberra-sound-theme-freedesktop.patch")))))
     (build-system gnu-build-system)
     (inputs
      `(("alsa-lib" ,alsa-lib)
@@ -55,9 +70,21 @@
        ("libtool" ,libtool)
        ("libvorbis" ,libvorbis)
        ("pulseaudio" ,pulseaudio)
-       ("udev" ,eudev)))
+       ("udev" ,eudev)
+       ("sound-theme-freedesktop" ,sound-theme-freedesktop)))
     (native-inputs
      `(("pkg-config" ,pkg-config)))
+    (arguments
+     `(#:phases
+       (alist-cons-before
+        'build 'patch-default-sounds-directory
+        (lambda* (#:key inputs #:allow-other-keys)
+          (substitute* "src/sound-theme-spec.c"
+            (("@SOUND_THEME_DIRECTORY@")
+             (string-append
+              (assoc-ref inputs "sound-theme-freedesktop")
+              "/share"))))
+        %standard-phases)))
     (home-page "http://0pointer.de/lennart/projects/libcanberra/")
     (synopsis
      "Implementation of the XDG Sound Theme and Name Specifications")
diff --git a/gnu/packages/patches/libcanberra-sound-theme-freedesktop.patch b/gnu/packages/patches/libcanberra-sound-theme-freedesktop.patch
new file mode 100644
index 0000000..212530e
--- /dev/null
+++ b/gnu/packages/patches/libcanberra-sound-theme-freedesktop.patch
@@ -0,0 +1,19 @@
+--- src/sound-theme-spec.c.orig	2015-01-11 13:13:29.520527358 +0100
++++ src/sound-theme-spec.c	2015-01-11 13:18:40.927247628 +0100
+@@ -321,9 +321,13 @@
+         const char *g;
+ 
+         if (!(g = getenv("XDG_DATA_DIRS")) || *g == 0)
+-                return "/usr/local/share:/usr/share";
+-
+-        return g;
++                return "@SOUND_THEME_DIRECTORY@";
++	else {
++		const char *stp = ":@SOUND_THEME_DIRECTORY@";
++		size_t len =  strlen(stp) + strlen(g) + 1;
++		char *g2 = (char*) malloc(len);
++		return strcat(strcpy(g2, g), stp);
++	}
+ }
+ 
+ static int load_theme_dir(ca_theme_data *t, const char *name) {
-- 
1.8.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2015-01-11 13:04                   ` Federico Beffa
@ 2015-01-11 13:34                     ` Federico Beffa
  2015-01-12  9:22                       ` Ludovic Courtès
  0 siblings, 1 reply; 14+ messages in thread
From: Federico Beffa @ 2015-01-11 13:34 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: Guix-devel

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

On Sun, Jan 11, 2015 at 2:04 PM, Federico Beffa <beffa@ieee.org> wrote:
> Good idea! Here the updated patch.

seeing my own post I noticed that I had tabs instead of spaces in the
patch. Here I've corrected that.

Fede

[-- Attachment #2: 0003-gnu-libcanberra-Add-default-sounds-support.patch --]
[-- Type: text/x-patch, Size: 4098 bytes --]

From 14abfc26e7b45aa5644104fa661f34f3a70323ae Mon Sep 17 00:00:00 2001
From: Federico Beffa <beffa@fbengineering.ch>
Date: Thu, 18 Dec 2014 21:32:34 +0100
Subject: [PATCH 3/4] gnu: libcanberra: Add default sounds support.

* gnu/packages/libcanberra.scm (libcanberra): Add input
  'sound-theme-freedesktop'.  Add "libcanberra-sound-theme-freedesktop.patch"
  and related phase 'patch-default-sounds-directory to patch the default
  sounds directory.
---
 gnu/packages/libcanberra.scm                       | 31 ++++++++++++++++++++--
 .../libcanberra-sound-theme-freedesktop.patch      | 19 +++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100644 gnu/packages/patches/libcanberra-sound-theme-freedesktop.patch

diff --git a/gnu/packages/libcanberra.scm b/gnu/packages/libcanberra.scm
index 764c327..3d43c4f 100644
--- a/gnu/packages/libcanberra.scm
+++ b/gnu/packages/libcanberra.scm
@@ -19,6 +19,7 @@
 
 (define-module (gnu packages libcanberra)
   #:use-module ((guix licenses) #:select (lgpl2.1+))
+  #:use-module (gnu packages)
   #:use-module (guix packages)
   #:use-module (guix download)
   #:use-module (guix build-system gnu)
@@ -46,7 +47,21 @@
             version ".tar.xz"))
       (sha256
        (base32
-        "0wps39h8rx2b00vyvkia5j40fkak3dpipp1kzilqla0cgvk73dn2"))))
+        "0wps39h8rx2b00vyvkia5j40fkak3dpipp1kzilqla0cgvk73dn2"))
+      ;; "sound-theme-freedesktop" is the default and fall-back sound theme for
+      ;; XDG desktops and should always be present.
+      ;; http://www.freedesktop.org/wiki/Specifications/sound-theme-spec/
+      ;; We make sure libcanberra will find it.
+      ;;
+      ;; We add the default sounds store directory to the code dealing with
+      ;; XDG_DATA_DIRS and not XDG_DATA_HOME. This is because XDG_DATA_HOME
+      ;; can only be a single directory and is inspected first.  XDG_DATA_DIRS
+      ;; can list an arbitrary number of directories and is only inspected
+      ;; later.  This is designed to allows the user to modify any theme at
+      ;; his pleasure.
+      (patch-flags '("-p0"))
+      (patches
+       (list (search-patch "libcanberra-sound-theme-freedesktop.patch")))))
     (build-system gnu-build-system)
     (inputs
      `(("alsa-lib" ,alsa-lib)
@@ -55,9 +70,21 @@
        ("libtool" ,libtool)
        ("libvorbis" ,libvorbis)
        ("pulseaudio" ,pulseaudio)
-       ("udev" ,eudev)))
+       ("udev" ,eudev)
+       ("sound-theme-freedesktop" ,sound-theme-freedesktop)))
     (native-inputs
      `(("pkg-config" ,pkg-config)))
+    (arguments
+     `(#:phases
+       (alist-cons-before
+        'build 'patch-default-sounds-directory
+        (lambda* (#:key inputs #:allow-other-keys)
+          (substitute* "src/sound-theme-spec.c"
+            (("@SOUND_THEME_DIRECTORY@")
+             (string-append
+              (assoc-ref inputs "sound-theme-freedesktop")
+              "/share"))))
+        %standard-phases)))
     (home-page "http://0pointer.de/lennart/projects/libcanberra/")
     (synopsis
      "Implementation of the XDG Sound Theme and Name Specifications")
diff --git a/gnu/packages/patches/libcanberra-sound-theme-freedesktop.patch b/gnu/packages/patches/libcanberra-sound-theme-freedesktop.patch
new file mode 100644
index 0000000..10bfbdf
--- /dev/null
+++ b/gnu/packages/patches/libcanberra-sound-theme-freedesktop.patch
@@ -0,0 +1,19 @@
+--- src/sound-theme-spec.c.orig	2015-01-11 13:13:29.520527358 +0100
++++ src/sound-theme-spec.c	2015-01-11 14:27:23.035046849 +0100
+@@ -321,9 +321,13 @@
+         const char *g;
+ 
+         if (!(g = getenv("XDG_DATA_DIRS")) || *g == 0)
+-                return "/usr/local/share:/usr/share";
+-
+-        return g;
++                return "@SOUND_THEME_DIRECTORY@";
++        else {
++                const char *stp = ":@SOUND_THEME_DIRECTORY@";
++                size_t len = strlen(stp) + strlen(g) + 1;
++                char *g2 = (char*) malloc(len);
++                return strcat(strcpy(g2, g), stp);
++        }
+ }
+ 
+ static int load_theme_dir(ca_theme_data *t, const char *name) {
-- 
1.8.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] gnu: libcanberra: Add propagated-input.
  2015-01-11 13:34                     ` Federico Beffa
@ 2015-01-12  9:22                       ` Ludovic Courtès
  0 siblings, 0 replies; 14+ messages in thread
From: Ludovic Courtès @ 2015-01-12  9:22 UTC (permalink / raw)
  To: Federico Beffa; +Cc: Guix-devel

Federico Beffa <beffa@ieee.org> skribis:

> From 14abfc26e7b45aa5644104fa661f34f3a70323ae Mon Sep 17 00:00:00 2001
> From: Federico Beffa <beffa@fbengineering.ch>
> Date: Thu, 18 Dec 2014 21:32:34 +0100
> Subject: [PATCH 3/4] gnu: libcanberra: Add default sounds support.
>
> * gnu/packages/libcanberra.scm (libcanberra): Add input
>   'sound-theme-freedesktop'.  Add "libcanberra-sound-theme-freedesktop.patch"
>   and related phase 'patch-default-sounds-directory to patch the default
>   sounds directory.

[...]

> diff --git a/gnu/packages/patches/libcanberra-sound-theme-freedesktop.patch b/gnu/packages/patches/libcanberra-sound-theme-freedesktop.patch
> new file mode 100644
> index 0000000..10bfbdf
> --- /dev/null
> +++ b/gnu/packages/patches/libcanberra-sound-theme-freedesktop.patch
> @@ -0,0 +1,19 @@
> +--- src/sound-theme-spec.c.orig	2015-01-11 13:13:29.520527358 +0100
> ++++ src/sound-theme-spec.c	2015-01-11 14:27:23.035046849 +0100
> +@@ -321,9 +321,13 @@
> +         const char *g;
> + 
> +         if (!(g = getenv("XDG_DATA_DIRS")) || *g == 0)
> +-                return "/usr/local/share:/usr/share";
> +-
> +-        return g;
> ++                return "@SOUND_THEME_DIRECTORY@";
> ++        else {
> ++                const char *stp = ":@SOUND_THEME_DIRECTORY@";
> ++                size_t len = strlen(stp) + strlen(g) + 1;
> ++                char *g2 = (char*) malloc(len);
> ++                return strcat(strcpy(g2, g), stp);
> ++        }
> + }
> + 
> + static int load_theme_dir(ca_theme_data *t, const char *name) {

Nice, much more readable IMO.  :-)

Before committing, could you just add a short description at the top of
the patch?

Thanks!

Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-01-12  9:22 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-18 21:14 [PATCH 3/4] gnu: libcanberra: Add propagated-input Federico Beffa
2014-12-21 11:06 ` Ludovic Courtès
2014-12-21 15:33   ` Federico Beffa
2015-01-07 17:20     ` Federico Beffa
2015-01-07 20:11     ` Ludovic Courtès
2015-01-08 17:08       ` Federico Beffa
2015-01-08 20:39         ` Ludovic Courtès
2015-01-10 11:15           ` Federico Beffa
2015-01-10 21:13             ` Ludovic Courtès
2015-01-11  8:34               ` Federico Beffa
2015-01-11 10:59                 ` Ludovic Courtès
2015-01-11 13:04                   ` Federico Beffa
2015-01-11 13:34                     ` Federico Beffa
2015-01-12  9:22                       ` Ludovic Courtès

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).