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

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

[Katherine Cox-Buday]

[-- Attachment #1: Type: text/plain, Size: 98 bytes --]

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

[-- Attachment #2: 0001-gnu-emacs-direnv-Update-to-2.1.0.patch --]
[-- Type: text/x-patch, Size: 1744 bytes --]

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

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

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

[Nicolas Goaziou]

Hello,

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

> 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

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

[Katherine Cox-Buday]

Nicolas Goaziou <mail@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

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

[Nicolas Goaziou]

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.

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,

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

[Katherine Cox-Buday]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

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

> 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

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

[Oleg Pykhalov]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

Hi.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Katherine Cox-Buday]

[-- Attachment #1: Type: text/plain, Size: 1214 bytes --]

Oleg Pykhalov <go.wigust@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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-emacs-direnv-Update-to-2.1.0.patch --]
[-- Type: text/x-patch, Size: 1991 bytes --]

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

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


[-- Attachment #3: Type: text/plain, Size: 60 bytes --]


Thank you both for your thoughtful reviews!

-- 
Katherine

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

[Oleg Pykhalov]

[-- Attachment #1.1: Type: text/plain, Size: 2354 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: diff --]
[-- Type: text/x-patch, Size: 1121 bytes --]

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

[-- Attachment #1.3: Type: text/plain, Size: 411 bytes --]


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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Oleg Pykhalov]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

bug#41790: [PATCH] Update emacs-direnv

[Maxim Cournoyer]

Hello,

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