GNU bug report logs - #41790
[PATCH] Update emacs-direnv

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 41790 in the body.
You can then email your comments to 41790 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: Katherine Cox-Buday <cox.katherine.e <at> gmail.com>
To: guix-patches <at> gnu.org
Subject: [PATCH] Update emacs-direnv
Date: Wed, 10 Jun 2020 10:12:03 -0500

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

10:11 kate says: guix refresh -l emacs-direnv
No dependents other than itself: emacs-direnv <at> 2.0.0

[0001-gnu-emacs-direnv-Update-to-2.1.0.patch (text/x-patch, attachment)]

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

From: Nicolas Goaziou <mail <at> nicolasgoaziou.fr>
To: Katherine Cox-Buday <cox.katherine.e <at> gmail.com>
Cc: 41790 <at> debbugs.gnu.org
Subject: Re: [bug#41790] [PATCH] Update emacs-direnv
Date: Wed, 10 Jun 2020 18:11:49 +0200

Hello,

Katherine Cox-Buday <cox.katherine.e <at> gmail.com> writes:

> 10:11 kate says: guix refresh -l emacs-direnv
> No dependents other than itself: emacs-direnv <at> 2.0.0
>
> From 22f556d67d8ec2f548859ecd20239a859d70d102 Mon Sep 17 00:00:00 2001
> From: Katherine Cox-Buday <cox.katherine.e <at> gmail.com>
> Date: Wed, 10 Jun 2020 10:09:48 -0500
> Subject: [PATCH] gnu: emacs-direnv: Update to 2.1.0.
>
> * gnu/packages/emacs-xyz.scm (emacs-direnv): Update to 2.1.0 and make direnv a
>   propagated-input.

Thank you.

I wonder if propagating direnv is a good idea, tho.

I understand that the Emacs library is not going to be useful if direnv
is not available in user's profile. OTOH, installing this package from,
e.g., M-x list-packages, wouldn't install direnv either. Moreover,
I assume anyone installing this Emacs package would have direnv
available already.

Considering the rule of thumb is to limit propagated inputs, I suggest
to remove direnv.

WDYT?

Regards,

-- 
Nicolas Goaziou

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

From: Katherine Cox-Buday <cox.katherine.e <at> gmail.com>
To: Nicolas Goaziou <mail <at> nicolasgoaziou.fr>
Cc: 41790 <at> debbugs.gnu.org
Subject: Re: [bug#41790] [PATCH] Update emacs-direnv
Date: Wed, 10 Jun 2020 11:26:06 -0500

Nicolas Goaziou <mail <at> nicolasgoaziou.fr> writes:

Thank you for the review!

> I understand that the Emacs library is not going to be useful if
> direnv is not available in user's profile. OTOH, installing this
> package from, e.g., M-x list-packages, wouldn't install direnv either.

The difference I see is that emacs's package manager is specifically for
emacs -- not for a user's system. Guix is a package manager for a user's
system which encompasses binaries and native libraries. This is one of
the reasons I prefer Guix packages to various language/tool package
managers because Guix can handle load paths and dependencies for me.

> Moreover, I assume anyone installing this Emacs package would have
> direnv available already.

I did not, nor did I know that I had to until it wasn't working and I
went and read the documentation.

> Considering the rule of thumb is to limit propagated inputs, I suggest
> to remove direnv.

I disagree. If propagated inputs are not for this -- making the package
even functional -- what are they for?

But! I am open to discussion.

-- 
Katherine

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

From: Nicolas Goaziou <mail <at> nicolasgoaziou.fr>
To: Katherine Cox-Buday <cox.katherine.e <at> gmail.com>
Cc: 41790 <at> debbugs.gnu.org
Subject: Re: [bug#41790] [PATCH] Update emacs-direnv
Date: Wed, 10 Jun 2020 20:05:12 +0200

Katherine Cox-Buday <cox.katherine.e <at> gmail.com> writes:

> I disagree. If propagated inputs are not for this -- making the package
> even functional -- what are they for?

IIUC, they are to be used as a last resort, e.g., when the package
cannot possibly build without them.

Reason is propagated inputs pollute user's profile. E.g., someone may
want to use a different direnv, and this propagated input would conflict
with their package.

In this case, it is reasonable to expect users to install direnv
themselves.

> But! I am open to discussion.

I hear your arguments. 

I'll let maintainers decide about this, and hopefully clarify what can
and cannot be propagated, at least in Emacs packages. This will be
useful feedback for future reviews.

Meanwhile, you can still provide a patch only bumping emacs-direnv. It
is also fine in you prefer to wait.

Regards,

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

From: Katherine Cox-Buday <cox.katherine.e <at> gmail.com>
To: Nicolas Goaziou <mail <at> nicolasgoaziou.fr>
Cc: 41790 <at> debbugs.gnu.org
Subject: Re: [bug#41790] [PATCH] Update emacs-direnv
Date: Wed, 10 Jun 2020 13:31:10 -0500

Nicolas Goaziou <mail <at> nicolasgoaziou.fr> writes:

> Katherine Cox-Buday <cox.katherine.e <at> gmail.com> writes:
>
>> I disagree. If propagated inputs are not for this -- making the package
>> even functional -- what are they for?
>
> IIUC, they are to be used as a last resort, e.g., when the package
> cannot possibly build without them.

I checked the Guix manual for the intended use of propagated inputs
since I didn't completely understand how what is in the runtime
environment would affect the build environment. I found something which
maybe is open to interpretation:

  "To ensure that libraries written in those languages can find library
  code they depend on at run time, run-time dependencies must be listed
  in propagated-inputs rather than inputs."

Maybe a binary required to operate is no different than "library code
they depend on at run time"?

> Reason is propagated inputs pollute user's profile. E.g., someone may
> want to use a different direnv, and this propagated input would
> conflict with their package.

That's another good point I hadn't considered: what if for some reason a
user wants a different version of the tool (presumably provided by Guix
as well)?

> I'll let maintainers decide about this, and hopefully clarify what can
> and cannot be propagated, at least in Emacs packages. This will be
> useful feedback for future reviews.

That's a good idea. I have an opinion, but it is not fully informed. I
appreciate the review, conversation, and appeal to authority!

> Meanwhile, you can still provide a patch only bumping emacs-direnv. It
> is also fine in you prefer to wait.

I'll let this one sit. The version bump was a side-effect of me
believing the package should also install the tool.

-- 
Katherine

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

From: Oleg Pykhalov <go.wigust <at> gmail.com>
To: Katherine Cox-Buday <cox.katherine.e <at> gmail.com>
Cc: 41790 <at> debbugs.gnu.org, Nicolas Goaziou <mail <at> nicolasgoaziou.fr>
Subject: Re: [bug#41790] [PATCH] Update emacs-direnv
Date: Wed, 10 Jun 2020 21:46:19 +0300

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

Hi.

Katherine Cox-Buday <cox.katherine.e <at> gmail.com> writes:

[…]

> I disagree. If propagated inputs are not for this -- making the package
> even functional -- what are they for?
>
> But! I am open to discussion.

Propagated inputs could lead to conflicts in a Guix profile.  The
simplest example I could remember is - you want upgrade package ‘A’
which propagates ‘direnv’, but you cannot because package ‘B’ propagates
it too.  In this case you need to upgrade both ‘A’ and ‘B’ or delete ‘A’
(or ‘B’).

Instead we could make the package functional by substituting in
/gnu/store/…-emacs-direnv-…-checkout/direnv.el file ‘direnv--detect’
("Detect the direnv executable.") procedure which could return a path to
‘direnv’ binary as a string directly without calling ‘executable-find’.
WDYT?

Oleg.

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

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

From: Katherine Cox-Buday <cox.katherine.e <at> gmail.com>
To: Oleg Pykhalov <go.wigust <at> gmail.com>
Cc: 41790 <at> debbugs.gnu.org, Nicolas Goaziou <mail <at> nicolasgoaziou.fr>
Subject: Re: [bug#41790] [PATCH] Update emacs-direnv
Date: Thu, 11 Jun 2020 09:44:35 -0500

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

Oleg Pykhalov <go.wigust <at> gmail.com> writes:

Heya Oleg, thanks for chiming in.

> Propagated inputs could lead to conflicts in a Guix profile.  The
> simplest example I could remember is - you want upgrade package ‘A’
> which propagates ‘direnv’, but you cannot because package ‘B’ propagates
> it too.  In this case you need to upgrade both ‘A’ and ‘B’ or delete ‘A’
> (or ‘B’).

Would there be a conflict if they both propagated the same input (in
this case the direnv binary)?

> Instead we could make the package functional by substituting in
> /gnu/store/…-emacs-direnv-…-checkout/direnv.el file ‘direnv--detect’
> ("Detect the direnv executable.") procedure which could return a path to
> ‘direnv’ binary as a string directly without calling ‘executable-find’.
> WDYT?

In general, I like to keep packages as close to their source as
possible, but I'm slowly learning that this is not often the case when
packaging things (which is a shame and a risk in my opinion).

But all things considered, I think this is probably the right approach
here given the feedback I'm getting.

Here's a patch which should supersede the previous patch:

[0001-gnu-emacs-direnv-Update-to-2.1.0.patch (text/x-patch, inline)]

From 52a5541b8a44c6629f6e2a6d3d47184f2ed5169b Mon Sep 17 00:00:00 2001
From: Katherine Cox-Buday <cox.katherine.e <at> gmail.com>
Date: Wed, 10 Jun 2020 10:09:48 -0500
Subject: [PATCH] gnu: emacs-direnv: Update to 2.1.0.

* gnu/packages/emacs-xyz.scm (emacs-direnv): Update to 2.1.0 and make direnv a
  propagated-input.
---
 gnu/packages/emacs-xyz.scm | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/gnu/packages/emacs-xyz.scm b/gnu/packages/emacs-xyz.scm
index 946d01cba5..6eb5bc9d39 100644
--- a/gnu/packages/emacs-xyz.scm
+++ b/gnu/packages/emacs-xyz.scm
@@ -166,6 +166,7 @@
   #:use-module (gnu packages sphinx)
   #:use-module (gnu packages xdisorg)
   #:use-module (gnu packages shells)
+  #:use-module (gnu packages shellutils)
   #:use-module (gnu packages sqlite)
   #:use-module (gnu packages gnupg)
   #:use-module (gnu packages video)
@@ -2020,7 +2021,7 @@ Its features are:
 (define-public emacs-direnv
   (package
     (name "emacs-direnv")
-    (version "2.0.0")
+    (version "2.1.0")
     (source
      (origin
        (method git-fetch)
@@ -2030,8 +2031,20 @@ Its features are:
        (file-name (git-file-name name version))
        (sha256
         (base32
-         "005ibyzsx1fdyrl5iyhqpb1bg83mphzahq7zvw58x00syyqi2z49"))))
+         "0xkqn4604k2imas6azy1www56br8ls4iv9a44pxcd8h94j1fp44d"))))
     (build-system emacs-build-system)
+    (arguments
+     `(#:phases
+       (modify-phases %standard-phases
+         (add-after 'unpack 'patch-in-direnv
+           (lambda* (#:key inputs #:allow-other-keys)
+             (let* ((direnv-path (assoc-ref inputs "direnv"))
+                    (direnv-bin (string-append
+                                 "\"" direnv-path "/bin/direnv\"")))
+               (substitute* "direnv.el"
+                 (("\"direnv\"") direnv-bin))))))))
+    (inputs
+     `(("direnv" ,direnv)))
     (propagated-inputs
      `(("dash" ,emacs-dash)
        ("with-editor" ,emacs-with-editor)))
-- 
2.26.2

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

Thank you both for your thoughtful reviews!

-- 
Katherine

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

From: Oleg Pykhalov <go.wigust <at> gmail.com>
To: Katherine Cox-Buday <cox.katherine.e <at> gmail.com>
Cc: 41790 <at> debbugs.gnu.org
Subject: Re: [bug#41790] [PATCH] Update emacs-direnv
Date: Thu, 11 Jun 2020 23:03:37 +0300

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

Katherine Cox-Buday <cox.katherine.e <at> gmail.com> writes:

>> Propagated inputs could lead to conflicts in a Guix profile.  The
>> simplest example I could remember is - you want upgrade package ‘A’
>> which propagates ‘direnv’, but you cannot because package ‘B’ propagates
>> it too.  In this case you need to upgrade both ‘A’ and ‘B’ or delete ‘A’
>> (or ‘B’).
>
> Would there be a conflict if they both propagated the same input (in
> this case the direnv binary)?

No conflict for the same input.  ;-)

> diff --git a/gnu/packages/emacs-xyz.scm b/gnu/packages/emacs-xyz.scm
> index 946d01cba5..6eb5bc9d39 100644
> --- a/gnu/packages/emacs-xyz.scm
> +++ b/gnu/packages/emacs-xyz.scm
> @@ -166,6 +166,7 @@
>    #:use-module (gnu packages sphinx)
>    #:use-module (gnu packages xdisorg)
>    #:use-module (gnu packages shells)
> +  #:use-module (gnu packages shellutils)
>    #:use-module (gnu packages sqlite)
>    #:use-module (gnu packages gnupg)
>    #:use-module (gnu packages video)
> @@ -2020,7 +2021,7 @@ Its features are:
>  (define-public emacs-direnv
>    (package
>      (name "emacs-direnv")
> -    (version "2.0.0")
> +    (version "2.1.0")
>      (source
>       (origin
>         (method git-fetch)
> @@ -2030,8 +2031,20 @@ Its features are:
>         (file-name (git-file-name name version))
>         (sha256
>          (base32
> -         "005ibyzsx1fdyrl5iyhqpb1bg83mphzahq7zvw58x00syyqi2z49"))))
> +         "0xkqn4604k2imas6azy1www56br8ls4iv9a44pxcd8h94j1fp44d"))))

Could you send a separate patch for direnv update, please?

>      (build-system emacs-build-system)
> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (add-after 'unpack 'patch-in-direnv
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             (let* ((direnv-path (assoc-ref inputs "direnv"))
> +                    (direnv-bin (string-append
> +                                 "\"" direnv-path "/bin/direnv\"")))
> +               (substitute* "direnv.el"
> +                 (("\"direnv\"") direnv-bin))))))))
> +    (inputs
> +     `(("direnv" ,direnv)))
>      (propagated-inputs
>       `(("dash" ,emacs-dash)
>         ("with-editor" ,emacs-with-editor)))
> -- 
> 2.26.2

This will make the following change:

[out.diff (text/x-patch, inline)]

--- /gnu/store/lxi6pqc97fc17v5gkk1pmcq8fazn85sx-emacs-direnv-2.1.0/share/emacs/site-lisp/direnv.el	1970-01-01 03:00:01.000000000 +0300
+++ /gnu/store/f7z69fmfijf9babkk2r4lfaljk4rck4h-emacs-direnv-2.1.0/share/emacs/site-lisp/direnv.el	1970-01-01 03:00:01.000000000 +0300
@@ -30,7 +30,7 @@
 
 (defun direnv--detect ()
   "Detect the direnv executable."
-  (executable-find "direnv"))
+  (executable-find "/gnu/store/qj4p17czqbmwjx56h7jbf1c245kp8p47-direnv-2.15.2/bin/direnv"))
 
 (defvar direnv--output-buffer-name "*direnv*"
   "Name of the buffer filled with the last direnv output.")
@@ -99,7 +99,7 @@
           (erase-buffer)
           (let* ((default-directory directory)
                  (process-environment environment)
-                 (exit-code (call-process "direnv" nil `(t ,stderr-tempfile) nil "export" "json"))
+                 (exit-code (call-process "/gnu/store/qj4p17czqbmwjx56h7jbf1c245kp8p47-direnv-2.15.2/bin/direnv" nil `(t ,stderr-tempfile) nil "export" "json"))
                  (json-key-type 'string))
             (prog1
                 (unless (zerop (buffer-size))

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

In the first hunk we don't need a ‘executable-find’ call on
‘/gnu/store/…-direnv-…/bin/direnv’.

The second hunk is not good, because it will require to redefine both
direnv--detect and direnv--export in case user needs a custom direnv
binary.  direnv--export on upstream's master branch is different in way
we could avoid the second hunk.  Could you take a look on this?

Thanks,
Oleg.

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

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

From: Oleg Pykhalov <go.wigust <at> gmail.com>
To: Katherine Cox-Buday <cox.katherine.e <at> gmail.com>
Cc: 41790 <at> debbugs.gnu.org
Subject: Re: [bug#41790] [PATCH] Update emacs-direnv
Date: Thu, 11 Jun 2020 23:31:29 +0300

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

Katherine Cox-Buday <cox.katherine.e <at> gmail.com> writes:

[…]

> In general, I like to keep packages as close to their source as
> possible, but I'm slowly learning that this is not often the case when
> packaging things (which is a shame and a risk in my opinion).
>
> But all things considered, I think this is probably the right approach
> here given the feedback I'm getting.

It depends.  If you take a look on magit-git-executable commentary, it
specifically doesn't use a full path to git binary, because of remote
systems could have it in different locations.

Does emacs-direnv could be used on remote machines?

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

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

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Katherine Cox-Buday <cox.katherine.e <at> gmail.com>
Cc: Oleg Pykhalov <go.wigust <at> gmail.com>, 41790-done <at> debbugs.gnu.org,
 Nicolas Goaziou <mail <at> nicolasgoaziou.fr>
Subject: Re: bug#41790: [PATCH] Update emacs-direnv
Date: Fri, 06 Aug 2021 00:49:52 -0400

Hello,

Katherine Cox-Buday <cox.katherine.e <at> gmail.com> writes:

[...]

>      (build-system emacs-build-system)
> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (add-after 'unpack 'patch-in-direnv
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             (let* ((direnv-path (assoc-ref inputs "direnv"))
> +                    (direnv-bin (string-append
> +                                 "\"" direnv-path "/bin/direnv\"")))
> +               (substitute* "direnv.el"
> +                 (("\"direnv\"") direnv-bin))))))))
> +    (inputs
> +     `(("direnv" ,direnv)))
>      (propagated-inputs
>       `(("dash" ,emacs-dash)
>         ("with-editor" ,emacs-with-editor)))

Thanks to Nicolas and Oleg for the thoughtful review.  They had good
comments about was propagation was not the best way to make the package
usable out of the box.  Now that the above does the same but in a
functional way, I think it is a good addition.

I applied the above hunk (the package had already been updated) as
commit 0d72f24ac084acf9d69e147a692e5d8bcb2ea21b.

Thank you!

Closing.

Maxim

Previous Next

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