[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[Jai Vetrivelan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-gnu-lemonbar-Update-to-1.4.patch --]
[-- Type: text/patch, Size: 3461 bytes --]

From dedefb5d5f0389d1e6fb190c9d47a6f8ca398791 Mon Sep 17 00:00:00 2001
From: Jai Vetrivelan <jaivetrivelan@gmail.com>
Date: Sat, 5 Mar 2022 11:17:57 +0530
Subject: [PATCH] gnu: lemonbar: Update to 1.4.

* gnu/packages/wm.scm (lemonbar): Update to 1.4.
---
 gnu/packages/wm.scm | 62 ++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 32 deletions(-)

diff --git a/gnu/packages/wm.scm b/gnu/packages/wm.scm
index a4fda11cfe..1874040eb3 100644
--- a/gnu/packages/wm.scm
+++ b/gnu/packages/wm.scm
@@ -2247,41 +2247,39 @@ (define-public sbcl-stumpwm-screenshot
     (license license:gpl3+)))
 
 (define-public lemonbar
-  (let ((commit "35183ab81d2128dbb7b6d8e119cc57846bcefdb4")
-        (revision "1"))
-    (package
-      (name "lemonbar")
-      (version (git-version "1.3" revision commit))
-      (source (origin
-                (method git-fetch)
-                (uri (git-reference
-                      (url "https://github.com/LemonBoy/bar")
-                      (commit commit)))
-                (file-name (git-file-name name version))
-                (sha256
-                 (base32
-                  "1wwqbph392iwz8skaqxb0xpklb1l6yganqz80g4x1fhrnz7idmlh"))))
-      (build-system gnu-build-system)
-      (arguments
-       `(#:tests? #f                    ; no test suite
-         #:make-flags
-         (list ,(string-append "CC=" (cc-for-target))
-               (string-append "PREFIX=" %output))
-         #:phases
-         (modify-phases %standard-phases
-           (delete 'configure))))
-      (inputs
-       (list libxcb))
-      (native-inputs
-       (list perl))
-      (home-page "https://github.com/LemonBoy/bar")
-      (synopsis "Featherweight status bar")
-      (description
-       "@code{lemonbar} (formerly known as @code{bar}) is a lightweight
+  (package
+    (name "lemonbar")
+    (version "1.4")
+    (source (origin
+              (method git-fetch)
+              (uri (git-reference
+                    (url "https://github.com/LemonBoy/bar")
+                    (commit (string-append "v" version))))
+            (file-name (git-file-name name version))
+            (sha256
+             (base32
+              "0sm1lxxf0y2n87nvc8mz6i6mzb32f4qab80ppb28ibrwfir6jsln"))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:tests? #f                    ; no test suite
+                #:make-flags
+                (list ,(string-append "CC=" (cc-for-target))
+                      (string-append "PREFIX=" %output))
+                #:phases
+                (modify-phases %standard-phases
+                  (delete 'configure))))
+    (inputs
+     (list libxcb))
+    (native-inputs
+     (list perl))
+    (home-page "https://github.com/LemonBoy/bar")
+    (synopsis "Featherweight status bar")
+    (description
+     "@code{lemonbar} (formerly known as @code{bar}) is a lightweight
 bar entirely based on XCB.  Provides full UTF-8 support, basic
 formatting, RandR and Xinerama support and EWMH compliance without
 wasting your precious memory.")
-      (license license:x11))))
+    (license license:x11)))
 
 (define-public lemonbar-xft
   ;; Upstream v2.0 tag is several years behind HEAD

base-commit: 97065f9fe3da74adab54b70e28bd2cd59c06ee07
prerequisite-patch-id: 112e00ce191deb18c3866dc5f17e4c761a3b19af
prerequisite-patch-id: f347aa2bfdef7aeb7ff3fb8e157e4acb2e1f78ae
prerequisite-patch-id: 17ecf5c03e82c4231c8ae277ffd577b59b107c43
-- 
2.34.0

[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[Maxime Devos]

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

Jai Vetrivelan schreef op za 05-03-2022 om 11:35 [+0530]:
> +  (package
> +    (name "lemonbar")
> +    (version "1.4")
> +    (source (origin
> +              (method git-fetch)
> +              (uri (git-reference
> +                    (url "https://github.com/LemonBoy/bar")
> +                    (commit (string-append "v" version))))
> +            (file-name (git-file-name name version))

At least in the 'master' branch (and presumably also in v1.4), there
are a few bugs.  At line 789, the return value of 'malloc' is not
checked, potentially causing a null-pointer dereference (and hence, a
segfault) at line 790 (in lemonbar.c).

Could this be fixed?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[Maxime Devos]

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

Jai Vetrivelan schreef op za 05-03-2022 om 11:35 [+0530]:
> +    (version "1.4")
> +    (source (origin
> +              (method git-fetch)
> +              (uri (git-reference
> +                    (url "https://github.com/LemonBoy/bar")
> +                    (commit (string-append "v" version))))

Also, on line 1248, the return value of 'strdup' isn't checked.
Likewise on line 1475 and 974.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[Maxime Devos]

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

[Please keep 54252@debbugs.gnu.org in CC or To, such that other people
can respond as well, such that discussion is not lost even if the
original reviewers disappear, for transparancy reasons, etc.]

Jai Vetrivelan schreef op za 05-03-2022 om 18:32 [+0530]:
> Hello Maxime,
> 
> On 2022-03-05, 10:59 +0100, Maxime Devos <maximedevos@telenet.be> wrote:
> 
> > At least in the 'master' branch (and presumably also in v1.4), there
> > are a few bugs.  At line 789, the return value of 'malloc' is not
> > checked, potentially causing a null-pointer dereference (and hence, a
> > segfault) at line 790 (in lemonbar.c).
> 
> Do you mean something like this?
> 
> ┌────
> │ diff --git a/lemonbar.c b/lemonbar.c
> │ index 4da59ee..93fcb20 100644
> │ --- a/lemonbar.c
> │ +++ b/lemonbar.c
> │ @@ -785,8 +785,8 @@ font_load (const char *pattern)
> │ 
> │      // Copy over the width lut as it's part of font_info
> │      int lut_size = sizeof(xcb_charinfo_t) * xcb_query_font_char_infos_length(font_info);
> │ -    if (lut_size) {
> │ -        ret->width_lut = malloc(lut_size);
> │ +    ret->width_lut = malloc(lut_size);
> │ +    if (lut_size && ret->width_lut) {
> │          memcpy(ret->width_lut, xcb_query_font_char_infos(font_info), lut_size);
> │      }
> └────

That just ignores the allocation failure, even though possibly the code
will use ret->width_lut later, expecting it to be non-NULL (or maybe it
actually handles NULL, I don't know).  Probably the allocation failure
needs to be reported (printf + exit, or return NULL, or something
else).  The exact mechanism used in C for handling allocation failures
depends on the application.

> > Could this be fixed?
> 
> I am not familiar with the C programming language, so I might not be
> able to fix this.

Ideally, things would be fixed upstream (it's on GitHub so you could
try submitting a pull request).  For Guix though, I believe simply
reporting the potential issue upstream (at
<https://github.com/LemonBoy/bar/issues>) would be sufficient.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

bug#54252: [PATCH] gnu: lemonbar: Update to 1.4.

[Nicolas Goaziou]

Hello,

Jai Vetrivelan <jaivetrivelan@gmail.com> writes:

> From dedefb5d5f0389d1e6fb190c9d47a6f8ca398791 Mon Sep 17 00:00:00 2001
> From: Jai Vetrivelan <jaivetrivelan@gmail.com>
> Date: Sat, 5 Mar 2022 11:17:57 +0530
> Subject: [PATCH] gnu: lemonbar: Update to 1.4.

Applied. Thank you.

Regards,
-- 
Nicolas Goaziou

[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[Maxime Devos]

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

Nicolas Goaziou schreef op vr 11-03-2022 om 01:47 [+0100]:
> Jai Vetrivelan <jaivetrivelan@gmail.com> writes:
> 
> > From dedefb5d5f0389d1e6fb190c9d47a6f8ca398791 Mon Sep 17 00:00:00 2001
> > From: Jai Vetrivelan <jaivetrivelan@gmail.com>
> > Date: Sat, 5 Mar 2022 11:17:57 +0530
> > Subject: [PATCH] gnu: lemonbar: Update to 1.4.
> 
> Applied. Thank you.

I don't think the null pointer dereferences have been reported upstream
yet.

Greetings,
Maxime

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[Nicolas Goaziou]

Hello,

Maxime Devos <maximedevos@telenet.be> writes:

> Nicolas Goaziou schreef op vr 11-03-2022 om 01:47 [+0100]:
>> Jai Vetrivelan <jaivetrivelan@gmail.com> writes:
>> 
>> > From dedefb5d5f0389d1e6fb190c9d47a6f8ca398791 Mon Sep 17 00:00:00 2001
>> > From: Jai Vetrivelan <jaivetrivelan@gmail.com>
>> > Date: Sat, 5 Mar 2022 11:17:57 +0530
>> > Subject: [PATCH] gnu: lemonbar: Update to 1.4.
>> 
>> Applied. Thank you.
>
> I don't think the null pointer dereferences have been reported upstream
> yet.

It would probably be a nice thing to do, but I don't think we can
request packagers to also take care of reporting bugs upstream (or
fixing them!). It is a different task with a different required skill
set.

Regards,
-- 
Nicolas Goaziou

[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[Maxime Devos]

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

Maxime Devos schreef op za 05-03-2022 om 10:59 [+0100]:
> Jai Vetrivelan schreef op za 05-03-2022 om 11:35 [+0530]:
> > +  (package
> > +    (name "lemonbar")
> > +    (version "1.4")
> > +    (source (origin
> > +              (method git-fetch)
> > +              (uri (git-reference
> > +                    (url "https://github.com/LemonBoy/bar")
> > +                    (commit (string-append "v" version))))
> > +            (file-name (git-file-name name version))
> 
> At least in the 'master' branch (and presumably also in v1.4), there
> are a few bugs.  At line 789, the return value of 'malloc' is not
> checked, potentially causing a null-pointer dereference (and hence, a
> segfault) at line 790 (in lemonbar.c).

Now reported upstream at <https://github.com/LemonBoy/bar/issues/240>.



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[Maxim Cournoyer]

Hi Nicolas,

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Hello,
>
> Maxime Devos <maximedevos@telenet.be> writes:
>
>> Nicolas Goaziou schreef op vr 11-03-2022 om 01:47 [+0100]:
>>> Jai Vetrivelan <jaivetrivelan@gmail.com> writes:
>>> 
>>> > From dedefb5d5f0389d1e6fb190c9d47a6f8ca398791 Mon Sep 17 00:00:00 2001
>>> > From: Jai Vetrivelan <jaivetrivelan@gmail.com>
>>> > Date: Sat, 5 Mar 2022 11:17:57 +0530
>>> > Subject: [PATCH] gnu: lemonbar: Update to 1.4.
>>> 
>>> Applied. Thank you.
>>
>> I don't think the null pointer dereferences have been reported upstream
>> yet.
>
> It would probably be a nice thing to do, but I don't think we can
> request packagers to also take care of reporting bugs upstream (or
> fixing them!). It is a different task with a different required skill
> set.

I don't have the full context, but I'll chip in in case it is useful.
When problems in the packaging are discovered as part of our activities,
I think it is part of our duty to at least report them and
cross-reference them via a comment in the package source if possible.

The reason I find this important is that over time, upstream can fix the
problems reported, and we can get rid of substitutions and other
Guix-specific patches, which should help reduce maintenance burden.

I hope this makes sense,

Thank you!

Maxim

[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[Nicolas Goaziou]

Hello,

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

> I don't have the full context, but I'll chip in in case it is useful.
> When problems in the packaging are discovered as part of our activities,
> I think it is part of our duty to at least report them and
> cross-reference them via a comment in the package source if possible.

As stated previously, it is very good if the packager also has
sufficient knowledge to find and understand a bug in the source code
they are packaging. This can help both Guix and upstream.

However, in this thread the packager claimed they did not have
sufficient knowledge in C to do so. I think it is not a reasonable
expectation to ask them to report a problem they may not understand
sufficiently in order to write a proper report and possibly take part to
the discussion that would entail.

Moreover, in this particular situation, Jai Vetrivelan is not the
original packager for lemonbar; they are merely updating it. This is
another reason for not pressuring them to realize a report that might
put them in an uncomfortable position.

I hope that makes sense.

Regards,
-- 
Nicolas Goaziou

[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[Maxime Devos]

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

Nicolas Goaziou schreef op vr 11-03-2022 om 23:09 [+0100]:
> However, in this thread the packager claimed they did not have
> sufficient knowledge in C to do so. I think it is not a reasonable
> expectation to ask them to report a problem they may not understand
> sufficiently in order to write a proper report and possibly take part to
> the discussion that would entail.

I think it's reasonable to ask them, but unreasonable to make it a
requirement; it should be an optional, albeit a nice bonus making
reviewing and committing go a tiny bit smoother.

However, this seems a bit irrelevant to the issue of the patch being
committed before this issue was reported -- basically, I make the
following assumptions:

  1. there is a known issue with the package
  2. known issues have to be reported upstream in order for a package
     to be included in Guix
  3. the packager did not report the issue
  4. a reviewer or the committer can report the issue

If no-one reported the issue, then from (1) and (2) it follows that the
package cannot (yet) be included in Guix.

Now, when can it be included in Guix?  Assume the extra assumption:

  6. the package is included in Guix.

then from (1) and (2) it follows that _someone_ must have had reported
the issue upstream.  In this case, this 'someone' is not the packager,
so it must have been a reviewer or committer (due to 4, assuming for
simplicity that it wasn't some bystander or someone from another distro
or something).

My conclusion is: to include a package in Guix, the issue must first be
reported upstream.  Additionally, if the packager did not report the
issue, that does not override the (undocumented?) requirement; if the
packager did not report it, that merely implies the reviewers or
committer will have to do it. 

Greetings,
Maxime.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[Nicolas Goaziou]

Hello,

Maxime Devos <maximedevos@telenet.be> writes:

> However, this seems a bit irrelevant to the issue of the patch being
> committed before this issue was reported -- basically, I make the
> following assumptions:
>
>   1. there is a known issue with the package
>   2. known issues have to be reported upstream in order for a package
>      to be included in Guix

I understand these are your assumptions, but I don't see anything like
that in the packaging guidelines. It may be our duty to report them, as
Maxime Cournoyer wrote, but I don't see this as a blocker either.

Besides, sorry for being bold, but Jai Vetrivelan is not packaging
anything here; they are just bumping a version. The issue with the
package itself is not really of their concern.

>   3. the packager did not report the issue
>   4. a reviewer or the committer can report the issue

Anyone can. 

As a data point, even if I understand the importance of working with
upstream, I consider making that report is way above my pay grade.

> If no-one reported the issue, then from (1) and (2) it follows that the
> package cannot (yet) be included in Guix.

Yes, according to your own assumptions, this is correct.

> Now, when can it be included in Guix?

It is already included: this is a version bump. I assume you're talking
about a hypothetical new package.

> My conclusion is: to include a package in Guix, the issue must first be
> reported upstream.

So your conclusion is your initial assumption? Sorry, but you lost me.

> Additionally, if the packager did not report the
> issue, that does not override the (undocumented?) requirement; if the
> packager did not report it, that merely implies the reviewers or
> committer will have to do it. 

If they have the will, the time, and the skill to do so, absolutely.

However, please remember everyone—including you, of course—is doing
their best, as a benevolent. In this context, I consider "will have to"
to be strong words.

This is all personal but I don't think putting more pressure on
committers—or reviewers for that matter—would be doing any good to the
patch reviewing process, either.

Regards,
-- 
Nicolas Goaziou

[bug#54252] [PATCH] gnu: lemonbar: Update to 1.4.

[Maxime Devos]

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

Nicolas Goaziou schreef op za 12-03-2022 om 00:14 [+0100]:
> As a data point, even if I understand the importance of working with
> upstream, I consider making that report is way above my pay grade.

You don't have to make the report yourself, you could have delegated
(*) making the report to me as I was the one that noticed the issue.

(*) TBC, I'm thinking of suggestions here, not some hard commands.

I'll look into the other parts your response later.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]