password-store fails to build with tree version 2

DoneSubmitted by Olivier Dion.
Details
5 participants
  • Leo Famulari
  • Marius Bakke
  • Maxim Cournoyer
  • Tobias Geerinckx-Rice
  • Olivier Dion
Owner
unassigned
Severity
normal
Merged with
O
O
Olivier Dion wrote on 13 Jan 21:33 +0100
[PATCH] gnu: tree: Remove stddata feature.
(address . guix-patches@gnu.org)(name . Olivier Dion)(address . olivier.dion@polymtl.ca)
fee834af05b189e4efdf06977a2ccb8935aa1277.1642105304.git.olivier.dion@polymtl.ca
* 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(+)

Toggle diff (21 lines)
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
T
T
Tobias Geerinckx-Rice wrote on 13 Jan 21:44 +0100
(name . Olivier Dion)(address . olivier.dion@polymtl.ca)
87ee5bcqhy.fsf@nckx
Olivier,

Thanks again for tracking down this weird bug!

Olivier Dion via Guix-patches via 写道:
Toggle quote (3 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCYeCP2Q0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW15XQwA/imzo8+dkAo/YNPVnciTjEWoUaAbwmVpv071kZrU
7NpxAP918Nm095sz8+/N9WJCEpdKGhfo+4+hBf3+WvDcIAAwAw==
=SgBg
-----END PGP SIGNATURE-----

O
O
Olivier Dion wrote on 13 Jan 21:57 +0100
(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)
87ee5bwdzw.fsf@laura
On Thu, 13 Jan 2022, Tobias Geerinckx-Rice <me@tobias.gr> wrote:
Toggle quote (11 lines)
> 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@mama.indstate.edu>, maybe adding more weight
in the balance would help.

Toggle quote (4 lines)
> 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
M
M
Maxim Cournoyer wrote on 13 Jan 23:26 +0100
Re: bug#53238: [PATCH] gnu: tree: Remove stddata feature.
(name . Olivier Dion)(address . olivier.dion@polymtl.ca)
87r19bxofo.fsf_-_@gmail.com
Hi,

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

Toggle quote (24 lines)
> On Thu, 13 Jan 2022, Tobias Geerinckx-Rice <me@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@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"...
From 2a30d95c46ff1eb0bdac9307c5d6bb8e460de02f Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Thu, 13 Jan 2022 15:09:54 -0500
Subject: [PATCH] gnu: password-store: Fix test failure following 'tree'
update.

Thanks to Olivier Dion <olivier.dion@polymtl.ca> for diagnosing the source of
the problem!

* gnu/packages/password-utils.scm (password-store): Delete trailing #t.
[phases]{adjust-for-tree-2}: New phase.
---
gnu/packages/password-utils.scm | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)

Toggle diff (48 lines)
diff --git a/gnu/packages/password-utils.scm b/gnu/packages/password-utils.scm
index 0ff8608c9c..16d889344b 100644
--- a/gnu/packages/password-utils.scm
+++ b/gnu/packages/password-utils.scm
@@ -487,6 +487,21 @@ (define-public password-store
     (arguments
      '(#:phases
        (modify-phases %standard-phases
+         (add-after 'unpack 'adjust-for-tree-2
+           (lambda _
+             ;; XXX: tree 2.0.1 has this new "stddata pipe" that is
+             ;; automatically used to output in JSON if the file descriptor 3
+             ;; is available.  This conflicts with the test harness use of
+             ;; file descriptor 3, causing one of the tests to fail.
+             ;; Increment the file descriptors used by the harness by one to
+             ;; avoid the conflict.
+             (substitute* "tests/sharness.sh"
+               (("exec 4>&2 3>&1")
+                "exec 5>&2 4>&1")
+               (("exec 4>/dev/null 3>/dev/null")
+                "exec 5>/dev/null 4>/dev/null")
+               (("&4") "&5")
+               (("&3") "&4"))))
          (delete 'configure)
          (delete 'build)
          (add-before 'install 'patch-system-extension-dir
@@ -500,8 +515,7 @@ (define-public password-store
                   (string-append " SYSTEM_EXTENSION_DIR=\""
                                  "${PASSWORD_STORE_SYSTEM_EXTENSION_DIR:-"
                                  extension-dir
-                                 "}\"\n"))))
-             #t))
+                                 "}\"\n"))))))
          (add-before 'install 'patch-passmenu-path
            ;; FIXME Wayland support requires ydotool and dmenu-wl packages
            ;; We are ignoring part of the script that gets executed if
@@ -530,8 +544,7 @@ (define-public password-store
                               '("coreutils" "getopt" "git" "gnupg" "qrencode"
                                 "sed" "tree" "which" "wl-clipboard" "xclip"))))
                (wrap-program (string-append out "/bin/pass")
-                 `("PATH" ":" prefix (,(string-join path ":"))))
-               #t))))
+                 `("PATH" ":" prefix (,(string-join path ":"))))))))
        #:make-flags (list "CC=gcc" (string-append "PREFIX=" %output)
                           "WITH_ALLCOMP=yes"
                           (string-append "BASHCOMPDIR="
-- 
2.34.0
Thanks,

Maxim
T
T
Tobias Geerinckx-Rice wrote on 13 Jan 23:33 +0100
Re: [bug#53238] [PATCH] gnu: tree: Remove stddata feature.
(name . Olivier Dion)(address . olivier.dion@polymtl.ca)
87a6fzchid.fsf@nckx
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 写道:
Toggle quote (4 lines)
> 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…

Toggle quote (3 lines)
> 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.

Toggle quote (3 lines)
> 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
From e100fedb52df07738c2d535928c6c9f98042e07f Mon Sep 17 00:00:00 2001
From: Tobias Geerinckx-Rice <me@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@2.

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

Toggle diff (47 lines)
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@2: <issues.guix.gnu.org/53238>.
+       ("tree" ,tree-1)
        ("which" ,which)
        ("wl-clipboard" ,wl-clipboard)
        ("xclip" ,xclip)
-- 
2.34.0
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCYeC9Wg0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW15mZIBAMSOhfVhnZcXJiiLhaKMP8ZoKDViRaFPQdR7VmZV
+cgHAP9wwCSzoWCmEFZ6uTiGV9O3yAZz0IKShu35MYI1lHYqDA==
=rz+G
-----END PGP SIGNATURE-----

O
O
Olivier Dion wrote on 14 Jan 02:55 +0100
(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)
87bl0fw076.fsf@laura
On Thu, 13 Jan 2022, Tobias Geerinckx-Rice <me@tobias.gr> wrote:
Toggle quote (6 lines)
> 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.

Toggle quote (10 lines)
> 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.

Toggle quote (7 lines)
>> 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
T
T
Tobias Geerinckx-Rice wrote on 14 Jan 03:05 +0100
(name . Olivier Dion)(address . olivier.dion@polymtl.ca)
871r1bcbo8.fsf@nckx
Olivier Dion 写道:
Toggle quote (4 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCYeDa5w0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW15+9UA/0lq/rGykgkPZhHTZvysZeOMi8+Mf0qqZowIviRc
j1MWAQCTP8XYJhfBL53dJj7lUMMhNdo/thfJnVy2/BY2zhT1Aw==
=phbs
-----END PGP SIGNATURE-----

L
L
Leo Famulari wrote on 14 Jan 23:34 +0100
(no subject)
(name . GNU bug tracker automated control server)(address . control@debbugs.gnu.org)
YeH6dSRJuAYv9XJ1@jasmine.lan
reassign 53238 guix
retitle 53238 password-store fails to build with tree version 2
merge 53238 53272
T
T
Tobias Geerinckx-Rice wrote on 15 Jan 15:37 +0100
Re: [bug#53238] [PATCH] gnu: tree: Remove stddata feature.
(name . Olivier Dion)(address . olivier.dion@polymtl.ca)
87pmotm58x.fsf@nckx
Olivier, Maxim(no -e, sorry! :-),

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

Tobias Geerinckx-Rice 写道:
Toggle quote (5 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCYeLc7g0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW15bEoA/1uf887e6ABjyEGXn4UJXWrZ/Ctd3VRr9c+oD6bO
8B2YAP4zyT/AWS8diYrwDVd8z4Amd/q537363cLTw59pe2qzBw==
=+VHt
-----END PGP SIGNATURE-----

T
T
Tobias Geerinckx-Rice wrote on 15 Jan 20:50 +0100
(no subject)
(name . GNU Debbugs)(address . control@debbugs.gnu.org)
87zgnw4w23.fsf@nckx
merge 53238 53288
M
M
Marius Bakke wrote on 16 Jan 18:04 +0100
Re: [bug#53238] [PATCH] gnu: tree: Remove stddata feature.
877daz8ve1.fsf@gnu.org
Hello!

Apologies for missing this discussion earlier...

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

Toggle quote (32 lines)
> Hi,
>
> Olivier Dion <olivier.dion@polymtl.ca> writes:
>
>> On Thu, 13 Jan 2022, Tobias Geerinckx-Rice <me@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@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:


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

WDYT?
-----BEGIN PGP SIGNATURE-----

iIUEARYKAC0WIQRNTknu3zbaMQ2ddzTocYulkRQQdwUCYeRQFg8cbWFyaXVzQGdu
dS5vcmcACgkQ6HGLpZEUEHeJfgD+Okk6I9IHmRBtxcSgc1WNFLC0/Nwe2obqFYj+
A3fHQEkBALGIax0lOnjRXAtZkxx1B22uM9RXWwEE8w8pseblFqUC
=AHF+
-----END PGP SIGNATURE-----

L
L
Leo Famulari wrote on 16 Jan 19:06 +0100
Re: bug#53238: [PATCH] gnu: tree: Remove stddata feature.
(name . Marius Bakke)(address . marius@gnu.org)
YeReiaoZyCY/Xny4@jasmine.lan
On Sun, Jan 16, 2022 at 06:04:22PM +0100, Marius Bakke wrote:
Toggle quote (14 lines)
> 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.
M
M
Marius Bakke wrote on 17 Jan 18:37 +0100
(name . Leo Famulari)(address . leo@famulari.name)
87sftm6z6x.fsf@gnu.org
Upstream fix pushed in 5da4cbfbd94163f87f188355e5490f04dd6864c2.
-----BEGIN PGP SIGNATURE-----

iIUEARYKAC0WIQRNTknu3zbaMQ2ddzTocYulkRQQdwUCYeWpVg8cbWFyaXVzQGdu
dS5vcmcACgkQ6HGLpZEUEHcAigD9ENn/9k1Z5q3KCnbTFicAQy9hpYIDTx+fFlQk
sjZqRwkBAP8tU9U8hVNjAnbzzH/pX9yYGdRDJ+Vb6EyqPn42ziAB
=yXs0
-----END PGP SIGNATURE-----

Closed
?
Your comment

This issue is archived.

To comment on this conversation send email to 53238@debbugs.gnu.org