password-store fails to build with tree version 2

  • Done
  • quality assurance status badge
Details
5 participants
  • Leo Famulari
  • Marius Bakke
  • Maxim Cournoyer
  • Tobias Geerinckx-Rice
  • Olivier Dion
Owner
unassigned
Submitted by
Olivier Dion
Severity
normal
Merged with
O
O
Olivier Dion wrote on 13 Jan 2022 21:33
[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 2022 21:44
(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 2022 21:57
(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 2022 23:26
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 2022 23:33
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 2022 02:55
(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 2022 03:05
(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 2022 23:34
(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 2022 15:37
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 2022 20:50
(no subject)
(name . GNU Debbugs)(address . control@debbugs.gnu.org)
87zgnw4w23.fsf@nckx
merge 53238 53288
M
M
Marius Bakke wrote on 16 Jan 2022 18:04
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 2022 19:06
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 2022 18:37
(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 an email to 53238@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 53238
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch