GNU bug report logs - #53238
password-store fails to build with tree version 2

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 53238 in the body.
You can then email your comments to 53238 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Olivier Dion <olivier.dion <at> polymtl.ca>
To: guix-patches <at> gnu.org
Cc: Olivier Dion <olivier.dion <at> polymtl.ca>
Subject: [PATCH] gnu: tree: Remove stddata feature.
Date: Thu, 13 Jan 2022 15:33:21 -0500

* gnu/packages/admin.scm (tree)
[arguments]: Add 'remove-stddata-feature phase after 'unpack phase.

Since version 2.0.0, there's a new feature call `stddata`.

From the ChangeLog:
--------------------------------------------------------------------------------
Output un-indented JSON on file descriptor 3 ("stddata") automatically if file
descriptor 3 is present (currently Linux only.) Maybe switch to BSON.
--------------------------------------------------------------------------------

This feature breaks some UNIX utilities.  Fix it by disabling the feature.
---
 gnu/packages/admin.scm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gnu/packages/admin.scm b/gnu/packages/admin.scm
index f11374a439..3d4909176a 100644
--- a/gnu/packages/admin.scm
+++ b/gnu/packages/admin.scm
@@ -2408,6 +2408,14 @@ (define-public tree
      (list
        #:phases
        #~(modify-phases %standard-phases
+           (add-after 'unpack 'remove-stddata-feature
+             (lambda _
+               (substitute* "tree.h"
+                 (("#  define STDDATA_FILENO 3")
+                  ""))
+               (substitute* "tree.c"
+                 (("#ifdef __linux__")
+                  "#ifdef STDDATA_FILENO"))))
            (delete 'configure))         ; No configure script.
        #:tests? #f                      ; No check target.
        #:make-flags
-- 
2.34.0

Message #8 received at 53238 <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Olivier Dion <olivier.dion <at> polymtl.ca>
Cc: 53238 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#53238] [PATCH] gnu: tree: Remove stddata feature.
Date: Thu, 13 Jan 2022 21:44:31 +0100

[Message part 1 (text/plain, inline)]

Olivier,

Thanks again for tracking down this weird bug!

Olivier Dion via Guix-patches via 写道：
> This feature breaks some UNIX utilities.  Fix it by disabling 
> the feature.

Hm…  How long would we have to carry this fork?  My fear is we'd 
do so indefinitely.

How about creating a (possibly hidden) tree-without-stddata 
package variant, to use as input to packages who currently break 
with this feature enabled?  That lets us refcount the need for it.

Kind regards,

T G-R

[signature.asc (application/pgp-signature, inline)]

Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Olivier Dion <olivier.dion <at> polymtl.ca>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>
Cc: 53238 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#53238] [PATCH] gnu: tree: Remove stddata feature.
Date: Thu, 13 Jan 2022 15:57:07 -0500

On Thu, 13 Jan 2022, Tobias Geerinckx-Rice <me <at> tobias.gr> wrote:
> Olivier,
>
> Thanks again for tracking down this weird bug!
>
> Olivier Dion via Guix-patches via 写道：
>> This feature breaks some UNIX utilities.  Fix it by disabling 
>> the feature.
>
> Hm…  How long would we have to carry this fork?  My fear is we'd 
> do so indefinitely.

I've contacted the maintainer asking for removal of the feature in its
next release.  I'm not sure if this will have some impact.  Feel free to
do the same at <ice+tree <at> mama.indstate.edu>, maybe adding more weight
in the balance would help.

> How about creating a (possibly hidden) tree-without-stddata 
> package variant, to use as input to packages who currently break 
> with this feature enabled?  That lets us refcount the need for it.

It's more than just packages, it's also user scripts that can be broken
and believe me when I say that this is not an easy bug to track down ;-).

-- 
Olivier Dion
Polymtl

Message #20 received at 53238 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Olivier Dion <olivier.dion <at> polymtl.ca>
Cc: Tobias Geerinckx-Rice <me <at> tobias.gr>, 53238 <at> debbugs.gnu.org
Subject: Re: bug#53238: [PATCH] gnu: tree: Remove stddata feature.
Date: Thu, 13 Jan 2022 17:26:19 -0500

[Message part 1 (text/plain, inline)]

Hi,

Olivier Dion <olivier.dion <at> polymtl.ca> writes:

> On Thu, 13 Jan 2022, Tobias Geerinckx-Rice <me <at> tobias.gr> wrote:
>> Olivier,
>>
>> Thanks again for tracking down this weird bug!
>>
>> Olivier Dion via Guix-patches via 写道：
>>> This feature breaks some UNIX utilities.  Fix it by disabling 
>>> the feature.
>>
>> Hm…  How long would we have to carry this fork?  My fear is we'd 
>> do so indefinitely.
>
> I've contacted the maintainer asking for removal of the feature in its
> next release.  I'm not sure if this will have some impact.  Feel free to
> do the same at <ice+tree <at> mama.indstate.edu>, maybe adding more weight
> in the balance would help.
>
>> How about creating a (possibly hidden) tree-without-stddata 
>> package variant, to use as input to packages who currently break 
>> with this feature enabled?  That lets us refcount the need for it.
>
> It's more than just packages, it's also user scripts that can be broken
> and believe me when I say that this is not an easy bug to track down ;-).

I'm on the fence about this, it does indeed seem an undesirable change,
especially since there's a --json option, but I am not the author of the
'tree' software.

Attached is an alternative that adjusts password-store instead of
removing this new tree "feature"...

[0001-gnu-password-store-Fix-test-failure-following-tree-u.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

Thanks,

Maxim

Message #23 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Olivier Dion <olivier.dion <at> polymtl.ca>
Cc: 53238 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#53238] [PATCH] gnu: tree: Remove stddata feature.
Date: Thu, 13 Jan 2022 23:33:29 +0100

[Message part 1 (text/plain, inline)]

Hullo Olivier,

I was going to apply the patch below to fix the password-store 
package, but Maxime just submitted another version which I prefer. 
I'd rather not provide two trees in Guix.

Olivier Dion 写道：
> I've contacted the maintainer asking for removal of the feature 
> in its
> next release.

After some consideration, I think it's an interesting feature. 
Something like this is long overdue.

I don't know if this approach is the right one, but I'll 
begrudgingly settle for JSON if it finally catches on…

> It's more than just packages, it's also user scripts that can be 
> broken

They can be fixed, or better yet rewritten.  tree(1) is not tr(1). 
‘Some lazy idiot could parse this with bash’ != ‘frozen API which 
upstream can never improve’.  Really.

…uh, I'm describing myself there, by the way ;-)  I feel quite 
seen.

Not that they needed to, but upstream even bumped the major 
revision along with this change.

> and believe me when I say that this is not an easy bug to track 
> down ;-).

Fully agree!  I wasted too much time trying to track it down 
myself.  I blame password-store's spaghetto of redirection more 
than tree.

Kind regards,

T G-R

[0004-gnu-password-store-Fix-failing-test-suite.patch (text/x-patch, inline)]

From e100fedb52df07738c2d535928c6c9f98042e07f Mon Sep 17 00:00:00 2001
From: Tobias Geerinckx-Rice <me <at> tobias.gr>
Date: Thu, 13 Jan 2022 13:45:25 +0000
Subject: [PATCH 04/26] gnu: password-store: Fix failing test suite.

* gnu/packages/admin.scm (tree-1): New public variable.
* gnu/packages/password-utils.scm (password-store)[inputs]:
Use it rather than the default tree <at> 2.

Reported by Maxim Cournoyer <maxim.cournoyer <at> gmail.com> and
Olivier Dion <olivier.dion <at> polymtl.ca>.
---
 gnu/packages/admin.scm          | 20 ++++++++++++++++++++
 gnu/packages/password-utils.scm |  3 ++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/admin.scm b/gnu/packages/admin.scm
index f11374a439..c2e656db1a 100644
--- a/gnu/packages/admin.scm
+++ b/gnu/packages/admin.scm
@@ -2421,6 +2421,26 @@ (define-public tree
     (home-page "http://mama.indstate.edu/users/ice/tree/")
     (license license:gpl2+)))
 
+(define-public tree-1
+  ;; tree 2.0.0 introduced a feature called ‘stddata’ that emits JSON when
+  ;; output is directed to file descriptor 3.  At least password-store still
+  ;; requires the old version.
+  (package
+    (inherit tree)
+    (version "1.8.0")
+    (source (origin
+              (method url-fetch)
+              (uri (string-append
+                    "http://mama.indstate.edu/users/ice/tree/src/tree-"
+                    version ".tgz"))
+              (sha256
+               (base32 "1hmpz6k0mr6salv0nprvm1g0rdjva1kx03bdf1scw8a38d5mspbi"))))
+    (arguments
+     (substitute-keyword-arguments (package-arguments tree)
+       ((#:make-flags flags '())
+        #~(append #$flags
+                  (list (string-append "prefix=" #$output))))))))
+
 (define-public lr
   (package
     (name "lr")
diff --git a/gnu/packages/password-utils.scm b/gnu/packages/password-utils.scm
index 0ff8608c9c..86af0deb47 100644
--- a/gnu/packages/password-utils.scm
+++ b/gnu/packages/password-utils.scm
@@ -552,7 +552,8 @@ (define-public password-store
        ("gnupg" ,gnupg)
        ("qrencode" ,qrencode)
        ("sed" ,sed)
-       ("tree" ,tree)
+       ;; XXX v1.7.4 tests are broken with tree <at> 2: <issues.guix.gnu.org/53238>.
+       ("tree" ,tree-1)
        ("which" ,which)
        ("wl-clipboard" ,wl-clipboard)
        ("xclip" ,xclip)
-- 
2.34.0

[signature.asc (application/pgp-signature, inline)]

Message #29 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Olivier Dion <olivier.dion <at> polymtl.ca>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>
Cc: 53238 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#53238] [PATCH] gnu: tree: Remove stddata feature.
Date: Thu, 13 Jan 2022 20:55:09 -0500

On Thu, 13 Jan 2022, Tobias Geerinckx-Rice <me <at> tobias.gr> wrote:
> Hullo Olivier,
>
> I was going to apply the patch below to fix the password-store 
> package, but Maxime just submitted another version which I prefer. 
> I'd rather not provide two trees in Guix.

I'm fine with both solutions.  In the end, password-store is not broken,
only its test suite.

> Olivier Dion 写道：
>> I've contacted the maintainer asking for removal of the feature in
>> its next release.
>
> After some consideration, I think it's an interesting feature. 
> Something like this is long overdue.
>
> I don't know if this approach is the right one, but I'll 
> begrudgingly settle for JSON if it finally catches on…

Just to be clear that the JSON is still there with the switch -J.  I
just think that using some random file descriptor like this is a path to
break many tools.  Any program that open a file and try to do a popen(3)
with "tree" for its output will get bitten by it.  It's not like if
`stddata` is some common knowledge outside of the PowerShell world.

>> and believe me when I say that this is not an easy bug to track 
>> down ;-).
>
> Fully agree!  I wasted too much time trying to track it down 
> myself.  I blame password-store's spaghetto of redirection more 
> than tree.

Happy to know I'm not the only one who spend way too much time on this ^^

-- 
Olivier Dion
Polymtl

Message #35 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Olivier Dion <olivier.dion <at> polymtl.ca>
Cc: 53238 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#53238] [PATCH] gnu: tree: Remove stddata feature.
Date: Fri, 14 Jan 2022 03:05:29 +0100

[Message part 1 (text/plain, inline)]

Olivier Dion 写道：
> It's not like if
> `stddata` is some common knowledge outside of the PowerShell 
> world.

FWIW I had never heard of it.  I'll admit it's not a good start in 
life.

Kind regards,

T G-R

[signature.asc (application/pgp-signature, inline)]

Message #47 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Olivier Dion <olivier.dion <at> polymtl.ca>
Cc: 53238 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#53238] [PATCH] gnu: tree: Remove stddata feature.
Date: Sat, 15 Jan 2022 15:37:04 +0100

[Message part 1 (text/plain, inline)]

Olivier, Maxim(no -e, sorry! :-),

Going by the number of bug reports, password-store is more popular 
than I thought.

Tobias Geerinckx-Rice 写道：
> I was going to apply the patch below to fix the password-store
> package, but Maxime just submitted another version which I 
> prefer. I'd
> rather not provide two trees in Guix.

I haven't changed my mind, but I did push the tree-1 solution as a 
‘temporary fix’ since it's the least invasive.

If Maxim's patch LGTeveryone, please go ahead and replace.

Kind regards,

T G-R

[signature.asc (application/pgp-signature, inline)]

Message #55 received at 53238 <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <marius <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Olivier Dion
 <olivier.dion <at> polymtl.ca>
Cc: Tobias Geerinckx-Rice <me <at> tobias.gr>, 53238 <at> debbugs.gnu.org
Subject: Re: [bug#53238] [PATCH] gnu: tree: Remove stddata feature.
Date: Sun, 16 Jan 2022 18:04:22 +0100

[Message part 1 (text/plain, inline)]

Hello!

Apologies for missing this discussion earlier...

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skriver:

> Hi,
>
> Olivier Dion <olivier.dion <at> polymtl.ca> writes:
>
>> On Thu, 13 Jan 2022, Tobias Geerinckx-Rice <me <at> tobias.gr> wrote:
>>> Olivier,
>>>
>>> Thanks again for tracking down this weird bug!
>>>
>>> Olivier Dion via Guix-patches via 写道：
>>>> This feature breaks some UNIX utilities.  Fix it by disabling 
>>>> the feature.
>>>
>>> Hm…  How long would we have to carry this fork?  My fear is we'd 
>>> do so indefinitely.
>>
>> I've contacted the maintainer asking for removal of the feature in its
>> next release.  I'm not sure if this will have some impact.  Feel free to
>> do the same at <ice+tree <at> mama.indstate.edu>, maybe adding more weight
>> in the balance would help.
>>
>>> How about creating a (possibly hidden) tree-without-stddata 
>>> package variant, to use as input to packages who currently break 
>>> with this feature enabled?  That lets us refcount the need for it.
>>
>> It's more than just packages, it's also user scripts that can be broken
>> and believe me when I say that this is not an easy bug to track down ;-).
>
> I'm on the fence about this, it does indeed seem an undesirable change,
> especially since there's a --json option, but I am not the author of the
> 'tree' software.

After some consideration (and emails with tree author), I think the best
solution is to patch 'password-store' so that it DTRT even in the
presence of fd 3.

I sent a patch to that effect upstream:

  https://lists.zx2c4.com/pipermail/password-store/2022-January/004563.html

...and have local patches to apply that in Guix and revert
bd4f314bbacaaa56751be3a4769f2082be747d24 and
a40ac6271578ea061a8a07b2adbd6032a690ca70.

WDYT?

[signature.asc (application/pgp-signature, inline)]

Message #58 received at 53238 <at> debbugs.gnu.org (full text, mbox):

From: Leo Famulari <leo <at> famulari.name>
To: Marius Bakke <marius <at> gnu.org>
Cc: Olivier Dion <olivier.dion <at> polymtl.ca>, 53238 <at> debbugs.gnu.org,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: Re: bug#53238: [PATCH] gnu: tree: Remove stddata feature.
Date: Sun, 16 Jan 2022 13:06:01 -0500

On Sun, Jan 16, 2022 at 06:04:22PM +0100, Marius Bakke wrote:
> After some consideration (and emails with tree author), I think the best
> solution is to patch 'password-store' so that it DTRT even in the
> presence of fd 3.
> 
> I sent a patch to that effect upstream:
> 
>   https://lists.zx2c4.com/pipermail/password-store/2022-January/004563.html
> 
> ...and have local patches to apply that in Guix and revert
> bd4f314bbacaaa56751be3a4769f2082be747d24 and
> a40ac6271578ea061a8a07b2adbd6032a690ca70.
> 
> WDYT?

Definitely, this is the right approach. I didn't participate in this
bugfix but I think that removing or adding features to packages is not
something we should be doing at the level of the distro, except with
upstream coordination. Reporting this issue to password-store should
have been one of the first steps we took.

Message #63 received at 53238-done <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <marius <at> gnu.org>
To: Leo Famulari <leo <at> famulari.name>
Cc: Olivier Dion <olivier.dion <at> polymtl.ca>, 53238-done <at> debbugs.gnu.org,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: Re: bug#53238: [PATCH] gnu: tree: Remove stddata feature.
Date: Mon, 17 Jan 2022 18:37:26 +0100

[Message part 1 (text/plain, inline)]

Upstream fix pushed in 5da4cbfbd94163f87f188355e5490f04dd6864c2.

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.