[PATCH] Update emacs-direnv

Done

Details

4 participants

Katherine Cox-Buday
Oleg Pykhalov
Nicolas Goaziou
Maxim Cournoyer

Owner: unassigned

Submitted by: Katherine Cox-Buday

Severity: normal

Katherine Cox-Buday wrote on 10 Jun 2020 17:12

Recipients:(address . guix-patches@gnu.org)

Message-ID:CA+TvSRh2t0vx9qq+keg8CZc3nwxcrHxKNv9QojY4Ff_9dMndoQ@mail.gmail.com

10:11 kate says: guix refresh -l emacs-direnv

No dependents other than itself: emacs-direnv@2.0.0

From 22f556d67d8ec2f548859ecd20239a859d70d102 Mon Sep 17 00:00:00 2001
From: Katherine Cox-Buday <cox.katherine.e@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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Toggle diff (39 lines)diff --git a/gnu/packages/emacs-xyz.scm b/gnu/packages/emacs-xyz.scm
index 946d01cba5..caeee2d828 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,11 +2031,13 @@ Its features are:
        (file-name (git-file-name name version))
        (sha256
         (base32
-         "005ibyzsx1fdyrl5iyhqpb1bg83mphzahq7zvw58x00syyqi2z49"))))
+         "0xkqn4604k2imas6azy1www56br8ls4iv9a44pxcd8h94j1fp44d"))))
     (build-system emacs-build-system)
     (propagated-inputs
      `(("dash" ,emacs-dash)
-       ("with-editor" ,emacs-with-editor)))
+       ("with-editor" ,emacs-with-editor)
+       ;; Without the binary, this mode is inoperable.
+       ("direnv" ,direnv)))
     (home-page "https://github.com/wbolster/emacs-direnv")
     (synopsis "Direnv integration for Emacs")
     (description
-- 
2.26.2

Nicolas Goaziou wrote on 10 Jun 2020 18:11

Recipients:(name . Katherine Cox-Buday)(address . cox.katherine.e@gmail.com)(address . 41790@debbugs.gnu.org)

Message-ID:871rmmoq7u.fsf@nicolasgoaziou.fr

Hello,

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

Toggle quote (11 lines)> 10:11 kate says: guix refresh -l emacs-direnv
> No dependents other than itself: emacs-direnv@2.0.0
>
> From 22f556d67d8ec2f548859ecd20239a859d70d102 Mon Sep 17 00:00:00 2001
> From: Katherine Cox-Buday <cox.katherine.e@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

Katherine Cox-Buday wrote on 10 Jun 2020 18:26

Recipients:(name . Nicolas Goaziou)(address . mail@nicolasgoaziou.fr)(address . 41790@debbugs.gnu.org)

Message-ID:87pna6zy3l.fsf@gmail.com

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

Thank you for the review!

Toggle quote (4 lines)

> 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.

Toggle quote (3 lines)

> 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.

Toggle quote (3 lines)

> 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

Nicolas Goaziou wrote on 10 Jun 2020 20:05

Recipients:(name . Katherine Cox-Buday)(address . cox.katherine.e@gmail.com)(address . 41790@debbugs.gnu.org)

Message-ID:87zh9alrtz.fsf@nicolasgoaziou.fr

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

Toggle quote (3 lines)

> 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.

Toggle quote (2 lines)

> 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,

Katherine Cox-Buday wrote on 10 Jun 2020 20:31

Recipients:(name . Nicolas Goaziou)(address . mail@nicolasgoaziou.fr)(address . 41790@debbugs.gnu.org)

Message-ID:87lfkuzsb5.fsf@gmail.com

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

Toggle quote (8 lines)

> Katherine Cox-Buday <cox.katherine.e@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"?

Toggle quote (4 lines)

> 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)?

Toggle quote (4 lines)

> 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!

Toggle quote (3 lines)

> 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

Oleg Pykhalov wrote on 10 Jun 2020 20:46

Recipients:(name . Katherine Cox-Buday)(address . cox.katherine.e@gmail.com)

Message-ID:87k10e92tg.fsf@gmail.com

Hi.

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

[…]

Toggle quote (5 lines)

> 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.

Katherine Cox-Buday wrote on 11 Jun 2020 16:44

Recipients:(name . Oleg Pykhalov)(address . go.wigust@gmail.com)

Message-ID:87eeql8xws.fsf@gmail.com

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

Heya Oleg, thanks for chiming in.

Toggle quote (6 lines)

> 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)?

Toggle quote (6 lines)

> 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:

From 52a5541b8a44c6629f6e2a6d3d47184f2ed5169b Mon Sep 17 00:00:00 2001
From: Katherine Cox-Buday <cox.katherine.e@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(-)

Toggle diff (45 lines)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

Thank you both for your thoughtful reviews!

Katherine

Oleg Pykhalov wrote on 11 Jun 2020 22:03

Recipients:(name . Katherine Cox-Buday)(address . cox.katherine.e@gmail.com)(address . 41790@debbugs.gnu.org)

Message-ID:87sgf174km.fsf@gmail.com

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

Toggle quote (9 lines)

>> 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. ;-)

Toggle quote (28 lines)> 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?

Toggle quote (19 lines)>      (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:

--- /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))

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.

Oleg Pykhalov wrote on 11 Jun 2020 22:31

Recipients:(name . Katherine Cox-Buday)(address . cox.katherine.e@gmail.com)(address . 41790@debbugs.gnu.org)

Message-ID:87o8pp73a6.fsf@gmail.com

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

[…]

Toggle quote (7 lines)

> 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?

Maxim Cournoyer wrote on 6 Aug 2021 06:49

Re: bug#41790: [PATCH] Update emacs-direnv

Recipients:(name . Katherine Cox-Buday)(address . cox.katherine.e@gmail.com)

Message-ID:87sfznjhxr.fsf_-_@gmail.com

Hello,

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

[...]

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

Closed

Your comment

This issue is archived.

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

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date