Re: 01/02: gnu: rhythmbox: Update hash of patch.

Re: 01/02: gnu: rhythmbox: Update hash of patch.

[Ludovic Courtès]

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

Hello Mark,

guix-commits@gnu.org skribis:

> commit e1d97c4efc42a661bd2772041371303e4070aa4f
> Author: Mark H Weaver <mhw@netris.org>
> Date:   Mon Nov 5 04:04:45 2018 -0500
>
>     gnu: rhythmbox: Update hash of patch.
>     
>     * gnu/packages/gnome.scm (rhythmbox)[source]: Update hash of the applied
>     patch.
> ---
>  gnu/packages/gnome.scm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gnu/packages/gnome.scm b/gnu/packages/gnome.scm
> index 06ec28b..376cf5a 100644
> --- a/gnu/packages/gnome.scm
> +++ b/gnu/packages/gnome.scm
> @@ -3746,7 +3746,7 @@ which can read a large number of file formats.")
>                        "b182c6b9e1d09e601bac0b703cc5f8b159ebbc3a.patch"))
>                  (sha256
>                   (base32
> -                  "17j45vyyr071ka3nckj2gycgyyv1j08fyrxw89jfdq2442nzrsiy")))))
> +                  "06n87xgf927djmv1vshal84nqx7g8nwgljza3g2vydhy7g2n1csq")))))

Could you explain the difference between the two patches?

In general I think we should keep providing detailed explanations in the
commit log (and possibly in the bug tracker) when such problems occur.

The former version of the patch can be found at
<https://mirror.hydra.gnu.org/file/b182c6b9e1d09e601bac0b703cc5f8b159ebbc3a.patch/sha256/17j45vyyr071ka3nckj2gycgyyv1j08fyrxw89jfdq2442nzrsiy>
(I’m attaching it here.)

TIA,
Ludo’.


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

From b182c6b9e1d09e601bac0b703cc5f8b159ebbc3a Mon Sep 17 00:00:00 2001
From: Bastien Nocera <hadess@hadess.net>
Date: Mon, 9 Oct 2017 13:11:47 +0200
Subject: [PATCH] fmradio: Fix build with GStreamer master

The plugin description is not supposed to be a string constant, but an
unescaped string, to be concatenated with function names.

This used to be acceptable (though would have warned), but breaks with
the GStreamer 1.13 development branch.

rb-fm-radio-gst-src.c:181:6: error: pasting ""rbsilencesrc"" and "_get_desc" does not give a valid preprocessing token
      "rbsilencesrc",
      ^

https://bugzilla.gnome.org/show_bug.cgi?id=788706
---
 plugins/fmradio/rb-fm-radio-gst-src.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/fmradio/rb-fm-radio-gst-src.c b/plugins/fmradio/rb-fm-radio-gst-src.c
index 09d709c..88abdaf 100644
--- a/plugins/fmradio/rb-fm-radio-gst-src.c
+++ b/plugins/fmradio/rb-fm-radio-gst-src.c
@@ -178,7 +178,7 @@ plugin_init (GstPlugin *plugin)
 
 GST_PLUGIN_DEFINE (GST_VERSION_MAJOR,
 		   GST_VERSION_MINOR,
-		   "rbsilencesrc",
+		   rbsilencesrc,
 		   "element to output silence",
 		   plugin_init,
 		   VERSION,
--
libgit2 0.27.1

Re: 01/02: gnu: rhythmbox: Update hash of patch.

[Mark H Weaver]

Hi Ludovic,

ludo@gnu.org (Ludovic Courtès) writes:

> guix-commits@gnu.org skribis:
>
>> commit e1d97c4efc42a661bd2772041371303e4070aa4f
>> Author: Mark H Weaver <mhw@netris.org>
>> Date:   Mon Nov 5 04:04:45 2018 -0500
>>
>>     gnu: rhythmbox: Update hash of patch.
>>     
>>     * gnu/packages/gnome.scm (rhythmbox)[source]: Update hash of the applied
>>     patch.
>> ---
>>  gnu/packages/gnome.scm | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gnu/packages/gnome.scm b/gnu/packages/gnome.scm
>> index 06ec28b..376cf5a 100644
>> --- a/gnu/packages/gnome.scm
>> +++ b/gnu/packages/gnome.scm
>> @@ -3746,7 +3746,7 @@ which can read a large number of file formats.")
>>                        "b182c6b9e1d09e601bac0b703cc5f8b159ebbc3a.patch"))
>>                  (sha256
>>                   (base32
>> -                  "17j45vyyr071ka3nckj2gycgyyv1j08fyrxw89jfdq2442nzrsiy")))))
>> +                  "06n87xgf927djmv1vshal84nqx7g8nwgljza3g2vydhy7g2n1csq")))))
>
> Could you explain the difference between the two patches?
>
> In general I think we should keep providing detailed explanations in the
> commit log (and possibly in the bug tracker) when such problems occur.

Agreed.  Thanks for paying attention to these things :)

At the time that I first committed this patch to my private branch,
Hydra was offline, and it wasn't obvious to me how to get the old
version.  However, I could see that the patch was so trivial that I
didn't worry about it.  Still, I probably should have asked on the
mailing list.

Anyway, see below for the diff between the old patch and the new one.

      Thanks,
        Mark

--8<---------------cut here---------------start------------->8---
--- t.patch	2018-11-25 16:26:38.946968736 -0500
+++ b182c6b9e1d09e601bac0b703cc5f8b159ebbc3a.patch	2018-11-25 14:53:32.019264658 -0500
@@ -19,7 +19,7 @@
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/plugins/fmradio/rb-fm-radio-gst-src.c b/plugins/fmradio/rb-fm-radio-gst-src.c
-index 09d709c..88abdaf 100644
+index 09d709c28..88abdaf35 100644
 --- a/plugins/fmradio/rb-fm-radio-gst-src.c
 +++ b/plugins/fmradio/rb-fm-radio-gst-src.c
 @@ -178,7 +178,7 @@ plugin_init (GstPlugin *plugin)
@@ -31,6 +31,6 @@
  		   "element to output silence",
  		   plugin_init,
  		   VERSION,
---
-libgit2 0.27.1
+-- 
+2.18.1
 
--8<---------------cut here---------------end--------------->8---

Generated patches change over time

[Ludovic Courtès]

Hello Mark,

Mark H Weaver <mhw@netris.org> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> guix-commits@gnu.org skribis:
>>
>>> commit e1d97c4efc42a661bd2772041371303e4070aa4f
>>> Author: Mark H Weaver <mhw@netris.org>
>>> Date:   Mon Nov 5 04:04:45 2018 -0500
>>>
>>>     gnu: rhythmbox: Update hash of patch.
>>>     
>>>     * gnu/packages/gnome.scm (rhythmbox)[source]: Update hash of the applied
>>>     patch.

[...]

>> Could you explain the difference between the two patches?
>>
>> In general I think we should keep providing detailed explanations in the
>> commit log (and possibly in the bug tracker) when such problems occur.
>
> Agreed.  Thanks for paying attention to these things :)
>
> At the time that I first committed this patch to my private branch,
> Hydra was offline, and it wasn't obvious to me how to get the old
> version.  However, I could see that the patch was so trivial that I
> didn't worry about it.  Still, I probably should have asked on the
> mailing list.

Indeed, that’s reasonable.  :-)

> Anyway, see below for the diff between the old patch and the new one.
>
>       Thanks,
>         Mark
>
> --- t.patch	2018-11-25 16:26:38.946968736 -0500
> +++ b182c6b9e1d09e601bac0b703cc5f8b159ebbc3a.patch	2018-11-25 14:53:32.019264658 -0500
> @@ -19,7 +19,7 @@
>   1 file changed, 1 insertion(+), 1 deletion(-)
>  
>  diff --git a/plugins/fmradio/rb-fm-radio-gst-src.c b/plugins/fmradio/rb-fm-radio-gst-src.c
> -index 09d709c..88abdaf 100644
> +index 09d709c28..88abdaf35 100644
>  --- a/plugins/fmradio/rb-fm-radio-gst-src.c
>  +++ b/plugins/fmradio/rb-fm-radio-gst-src.c
>  @@ -178,7 +178,7 @@ plugin_init (GstPlugin *plugin)
> @@ -31,6 +31,6 @@
>   		   "element to output silence",
>   		   plugin_init,
>   		   VERSION,
> ---
> -libgit2 0.27.1
> +-- 
> +2.18.1

Thanks for checking.

Lesson learned: we should not rely at all on generated patches because
they are bound to change frequently (version string at the end, length
of commit hash prefixes, etc.)  It’s probably worse than tarballs
generated by Git hosting services.

So we should probably work towards using local copies of patches, unless
we find that the generated patches do not include any variable bits.

Currently packages that include downloaded patches are¹:

  ("bash@4.4.19"
   "bash-minimal@4.4.19"
   "bash-static@4.4.19"
   "openttd@1.7.1"
   "telepathy-glib@0.24.1"
   "rhythmbox@3.4.2"
   "icecat@60.3.0-gnu1"
   "linux-libre-arm-generic@4.19.4"
   "cpupower@4.19.4"
   "linux-libre@4.19.4"
   "linux-libre-arm-generic@4.14.83"
   "linux-libre-arm-omap2plus@4.19.4"
   "linux-libre@4.14.83"
   "freefall@4.19.4"
   "linux-libre@4.4.164"
   "linux-libre-arm-omap2plus@4.14.83"
   "linux-libre@4.9.140"
   "john-the-ripper-jumbo@1.8.0-1"
   "readline@7.0.3"
   "texlive-bin@20170524"
   "vlc@3.0.4"
   "xcalib@0.10"
   "libebml@1.3.6"
   "xorg-server-xwayland@1.20.3"
   "xorg-server@1.20.3"
   "xf86-video-voodoo@1.2.5")

In the case of bash and texlive, the patches are not generated so we’re
fine.  That of vlc at
<https://git.videolan.org/?p=vlc.git;a=patch;h=a8953ba707cca1f2de372ca24513296bcfcdaaa8>
is generated though.

Needs more investigation!

Thanks,
Ludo’.

¹ I used:
   (fold-packages (lambda (p r)
                    (let ((source (package-source p)))
                      (if (origin? source)
                          (let ((patches (origin-patches source)))
                            (if (any origin? patches)
                                (cons p r)
                                r))
                          r)))
                  '())

Re: Generated patches change over time

[Mark H Weaver]

Hi Ludovic,

ludo@gnu.org (Ludovic Courtès) writes:

> Mark H Weaver <mhw@netris.org> skribis:
>
>> --- t.patch	2018-11-25 16:26:38.946968736 -0500
>> +++ b182c6b9e1d09e601bac0b703cc5f8b159ebbc3a.patch	2018-11-25 14:53:32.019264658 -0500
>> @@ -19,7 +19,7 @@
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>  
>>  diff --git a/plugins/fmradio/rb-fm-radio-gst-src.c b/plugins/fmradio/rb-fm-radio-gst-src.c
>> -index 09d709c..88abdaf 100644
>> +index 09d709c28..88abdaf35 100644
>>  --- a/plugins/fmradio/rb-fm-radio-gst-src.c
>>  +++ b/plugins/fmradio/rb-fm-radio-gst-src.c
>>  @@ -178,7 +178,7 @@ plugin_init (GstPlugin *plugin)
>> @@ -31,6 +31,6 @@
>>   		   "element to output silence",
>>   		   plugin_init,
>>   		   VERSION,
>> ---
>> -libgit2 0.27.1
>> +-- 
>> +2.18.1
>
> Thanks for checking.
>
> Lesson learned: we should not rely at all on generated patches because
> they are bound to change frequently (version string at the end, length
> of commit hash prefixes, etc.)  It’s probably worse than tarballs
> generated by Git hosting services.

I guess the length of the commit hashes probably won't change very
often, so the version number is the most pressing issue here.  I wonder
if it would be worth adding a special 'origin' type that removes the
version number from the end of git patches.

> So we should probably work towards using local copies of patches, unless
> we find that the generated patches do not include any variable bits.

It might still be best to work towards using local copies of patches,
although in the case of IceCat the set of patches is often quite large.

Another issue to consider is that the use of local copies of patches
involves putting more trust in contributors, in practice, or at least it
seems so to me.  When someone adds a non-obvious patch from upstream as
a local file, it leads me to want to check to make sure it hasn't been
modified from the upstream version, whereas if the package recipe
fetches a patch from a URL that is clearly from the upstream git
repository, it's somewhat more transparent what's going on.

That's not to say that I've reached a conclusion on this issue, but it's
another factor to consider.

     Regards,
       Mark

Re: Generated patches change over time

[Ludovic Courtès]

Hello Mark,

Mark H Weaver <mhw@netris.org> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:

[...]

>> Lesson learned: we should not rely at all on generated patches because
>> they are bound to change frequently (version string at the end, length
>> of commit hash prefixes, etc.)  It’s probably worse than tarballs
>> generated by Git hosting services.
>
> I guess the length of the commit hashes probably won't change very
> often, so the version number is the most pressing issue here.

Overall though such patches may typically change several times a year.

> I wonder if it would be worth adding a special 'origin' type that
> removes the version number from the end of git patches.

As I replied to Maxim, this is not really possible.

>> So we should probably work towards using local copies of patches, unless
>> we find that the generated patches do not include any variable bits.
>
> It might still be best to work towards using local copies of patches,
> although in the case of IceCat the set of patches is often quite large.
>
> Another issue to consider is that the use of local copies of patches
> involves putting more trust in contributors, in practice, or at least it
> seems so to me.  When someone adds a non-obvious patch from upstream as
> a local file, it leads me to want to check to make sure it hasn't been
> modified from the upstream version, whereas if the package recipe
> fetches a patch from a URL that is clearly from the upstream git
> repository, it's somewhat more transparent what's going on.

Yeah I agree with this.  Another concern is unbounded growth of the Git
repo.

OTOH a patch that changes over time becomes non auditable: you’ll notice
it has changed, and maybe that’s because of a benign change like those
discussed above, but maybe it’s something else; if you can’t retrieve or
reproduce the original patch, you can’t tell what the difference is.

So I don’t have a good solution, but I think we should avoid upstream
patches when we know they are generated in a non-reproducible fashion.

Thanks,
Ludo’.

Re: Generated patches change over time

[Maxim Cournoyer]

Hello!


>> ludo@gnu.org (Ludovic Courtès) writes:

>Lesson learned: we should not rely at all on generated patches because
>they are bound to change frequently (version string at the end, length
>of commit hash prefixes, etc.)  It’s probably worse than tarballs
>generated by Git hosting services.
>
>So we should probably work towards using local copies of patches,
>unless
>we find that the generated patches do not include any variable bits.
>

Maybe we could pass the patches through some sanitizer to strip any metadata? I guess the content itself shouldn't change?

Maxim

Re: Generated patches change over time

[Ludovic Courtès]

Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

>>> ludo@gnu.org (Ludovic Courtès) writes:
>
>>Lesson learned: we should not rely at all on generated patches because
>>they are bound to change frequently (version string at the end, length
>>of commit hash prefixes, etc.)  It’s probably worse than tarballs
>>generated by Git hosting services.
>>
>>So we should probably work towards using local copies of patches,
>>unless
>>we find that the generated patches do not include any variable bits.
>>
>
> Maybe we could pass the patches through some sanitizer to strip any metadata? I guess the content itself shouldn't change?

We can’t really do that, or the downloads would no longer be
fixed-output derivations and thus we wouldn’t be solving the problem.

Ludo’.

Re: Generated patches change over time

[Mark H Weaver]

ludo@gnu.org (Ludovic Courtès) writes:

> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>>>> ludo@gnu.org (Ludovic Courtès) writes:
>>
>>>Lesson learned: we should not rely at all on generated patches because
>>>they are bound to change frequently (version string at the end, length
>>>of commit hash prefixes, etc.)  It’s probably worse than tarballs
>>>generated by Git hosting services.
>>>
>>>So we should probably work towards using local copies of patches,
>>>unless
>>>we find that the generated patches do not include any variable bits.
>>>
>>
>> Maybe we could pass the patches through some sanitizer to strip any metadata? I guess the content itself shouldn't change?
>
> We can’t really do that, or the downloads would no longer be
> fixed-output derivations and thus we wouldn’t be solving the problem.

Can you elaborate on why it cannot be done?  If I understand correctly,
our 'git-fetch' origin type deletes the .git subdirectory after fetching
it, and yet it still creates fixed-output derivations, no?  I don't see
why stripping metadata from a patch is fundamentally any different.

       Mark

Re: Generated patches change over time

[Ludovic Courtès]

Mark H Weaver <mhw@netris.org> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>
>>>>> ludo@gnu.org (Ludovic Courtès) writes:
>>>
>>>>Lesson learned: we should not rely at all on generated patches because
>>>>they are bound to change frequently (version string at the end, length
>>>>of commit hash prefixes, etc.)  It’s probably worse than tarballs
>>>>generated by Git hosting services.
>>>>
>>>>So we should probably work towards using local copies of patches,
>>>>unless
>>>>we find that the generated patches do not include any variable bits.
>>>>
>>>
>>> Maybe we could pass the patches through some sanitizer to strip any metadata? I guess the content itself shouldn't change?
>>
>> We can’t really do that, or the downloads would no longer be
>> fixed-output derivations and thus we wouldn’t be solving the problem.
>
> Can you elaborate on why it cannot be done?  If I understand correctly,
> our 'git-fetch' origin type deletes the .git subdirectory after fetching
> it, and yet it still creates fixed-output derivations, no?  I don't see
> why stripping metadata from a patch is fundamentally any different.

You’re right, along the same lines, it could be a fixed-output
derivation.

The problem is rather that the workflow would be a bit awkward: ‘guix
download’ would download the raw, unprocessed patch, and thus it would
give you the “wrong” hash.

In essence you’d have to put a random hash in your package definition,
run “guix build -S”, copy the correct hash from the error message,
manually look at the patch, etc.

It’s possible, but it’s a bit awkward IMO.

Or we would need to add a ‘--strip-patch-metadata’ option to ‘guix
download’ so that it applies the exact same transformation when
downloading.

Thoughts?

Ludo’.

Re: Generated patches change over time

[Maxim Cournoyer]

ludo@gnu.org (Ludovic Courtès) writes:

[...]

> You’re right, along the same lines, it could be a fixed-output
> derivation.
>
> The problem is rather that the workflow would be a bit awkward: ‘guix
> download’ would download the raw, unprocessed patch, and thus it would
> give you the “wrong” hash.
>
> In essence you’d have to put a random hash in your package definition,
> run “guix build -S”, copy the correct hash from the error message,
> manually look at the patch, etc.
>
> It’s possible, but it’s a bit awkward IMO.
>
> Or we would need to add a ‘--strip-patch-metadata’ option to ‘guix
> download’ so that it applies the exact same transformation when
> downloading.

The way I see it, `guix download' should just do the right thing -- the
metadata stripping should be baked in and not user
controllable. Alternatively, it could be controllable, but enabled by
default. This would keep the workflow the same as it is now.

Maxim

Re: Generated patches change over time

[Mark H Weaver]

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> ludo@gnu.org (Ludovic Courtès) writes:
>
> [...]
>
>> You’re right, along the same lines, it could be a fixed-output
>> derivation.
>>
>> The problem is rather that the workflow would be a bit awkward: ‘guix
>> download’ would download the raw, unprocessed patch, and thus it would
>> give you the “wrong” hash.
>>
>> In essence you’d have to put a random hash in your package definition,
>> run “guix build -S”, copy the correct hash from the error message,
>> manually look at the patch, etc.
>>
>> It’s possible, but it’s a bit awkward IMO.
>>
>> Or we would need to add a ‘--strip-patch-metadata’ option to ‘guix
>> download’ so that it applies the exact same transformation when
>> downloading.
>
> The way I see it, `guix download' should just do the right thing -- the
> metadata stripping should be baked in and not user
> controllable. Alternatively, it could be controllable, but enabled by
> default. This would keep the workflow the same as it is now.

I *certainly* don't want "guix download" to try to automatically detect
that the downloaded file contains a patch, after some possibly
nontrivial amount of plain text which is typically present in patch
files, and to canonicalize the patch in that case.

However, we could add an optional flag to "guix download", or perhaps
add another 'guix' subcommand, to request the use of a specific 'origin'
type.  In addition to supporting canonicalized patches, this could also
improve the workflow for other origin types such as 'git-fetch' and
'hg-fetch'.

In general, the specified 'origin' type would determine the number and
meaning of the arguments, e.g. for 'git-fetch' an additional commit
argument would be needed.

Thoughts?

      Mark

Re: Generated patches change over time

[Maxim Cournoyer]

Hello,

Mark H Weaver <mhw@netris.org> writes:

> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

[...]

>> The way I see it, `guix download' should just do the right thing -- the
>> metadata stripping should be baked in and not user
>> controllable. Alternatively, it could be controllable, but enabled by
>> default. This would keep the workflow the same as it is now.
>
> I *certainly* don't want "guix download" to try to automatically detect
> that the downloaded file contains a patch, after some possibly
> nontrivial amount of plain text which is typically present in patch
> files, and to canonicalize the patch in that case.
>
> However, we could add an optional flag to "guix download", or perhaps
> add another 'guix' subcommand, to request the use of a specific 'origin'
> type.  In addition to supporting canonicalized patches, this could also
> improve the workflow for other origin types such as 'git-fetch' and
> 'hg-fetch'.
>
> In general, the specified 'origin' type would determine the number and
> meaning of the arguments, e.g. for 'git-fetch' an additional commit
> argument would be needed.
>
> Thoughts?

I like your idea, and it indeed sounds more straightforward from an
implementation perspective.

Maxim