Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

emacsWithPackages runs emacs with invocation-directory of nested package #145302

Closed
sheijk opened this issue Nov 10, 2021 · 5 comments · Fixed by #361145
Closed

emacsWithPackages runs emacs with invocation-directory of nested package #145302

sheijk opened this issue Nov 10, 2021 · 5 comments · Fixed by #361145
Labels
0.kind: bug Something is broken 6.topic: emacs Text editor

Comments

@sheijk
Copy link

sheijk commented Nov 10, 2021

Describe the bug

When wrapping Emacs using emacsPackagesFor the invocation-directory will point to the wrong Emacs package. This means restarting Emacs from inside Emacs will result in a binary which doesn't see the additional packages to be started and initialization can fail.

Steps To Reproduce

nix-build the following derivation (ran out of disk space trying to build master, but looking at the source the issue seems to be the same, see additional info section). The issue happens both with and without emacs-overlay.

{
  # nixpkgs ? ../nixpkgs,
  nixpkgs ? (builtins.fetchGit {
    name = "nixos-stable-21.05";
    url = "https://github.com/nixos/nixpkgs";
    ref = "refs/heads/nixos-21.05";
  }),
  # emacsOverlay ? (import (builtins.fetchGit {
  #   url = "https://github.com/nix-community/emacs-overlay/";
  #   ref = "master";
  #   rev = "64580e3ac034e2704895a272f341a0729d165b93";
  # })),
}:

with import nixpkgs { overlays = [ # emacsOverlay
                                 ]; };

let useUnstableForAllEmacsPkgs = false;
    myEmacsPkg = emacs;
    myPackages = epkgs:
      [ epkgs.melpaStablePackages.which-key ];
    myEmacs = (emacsPackagesFor myEmacsPkg).emacsWithPackages myPackages;
in
buildEnv {
  name = "my-emacs";

  paths = [ myEmacs ];
}

This will build a package my-emacs pulling in which-key and a package emacs-27.2 which is Emacs without the extra packages. The bin/emacs executable in my-emacs is a script setting up the environment so the wrapped executable will find which-key.

Run ./result/bin/emacs and press =C-h v invocation-directory= and check the resulting path. It should point inside the store path in target, but will be the path of the emacs-27.2 package.

Start a second Emacs instance from inside this one (variables need to be inserted by hand):
M-x, shell-command, {invocation-directory}{invocation-name}

in the second Emacs instance try to load the which-key package:
M-x, load-library, which-key

load-library: Cannot open load file: No such file or directory, which-key

Expected behavior

The invocation-directory should point to the $my-emacs/bin/ instead of $emacs-27.2/bin in both Emacs instances.

Additional context

In nixpkgs I see wrapper.sh which does an exec without -a which seems to be where the original invocation name gets lost. This is the same on master. This will end up being the bin/emacs binary in the built derivation for my-emacs. bin/emacs in emacs-27.2 will be another wrapper doing another exec -a (I could not figure out where this was being generated, though).

I tried changing wrapper.sh using this patch but this was not successful, yet.

From 8e7802bc9ffec82080a4916169a30305ec44e891 Mon Sep 17 00:00:00 2001
From: Jan Rehders <[email protected]>
Date: Wed, 10 Nov 2021 04:12:15 +0100
Subject: [PATCH] Pass command invocation to wrapper script

Probably the $0 is wrong
---
 pkgs/build-support/emacs/wrapper.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 mode change 100644 => 100755 pkgs/build-support/emacs/wrapper.sh

diff --git a/pkgs/build-support/emacs/wrapper.sh b/pkgs/build-support/emacs/wrapper.sh
old mode 100644
new mode 100755
index e8eecb8c869..56079801c07
--- a/pkgs/build-support/emacs/wrapper.sh
+++ b/pkgs/build-support/emacs/wrapper.sh
@@ -44,4 +44,4 @@ export emacsWithPackages_siteLisp=@wrapperSiteLisp@
 export EMACSNATIVELOADPATH="${newNativeLoadPath[*]}"
 export emacsWithPackages_siteLispNative=@wrapperSiteLispNative@
 
-exec @prog@ "$@"
+exec -a "$0" @prog@ "$@"
-- 
2.31.1

I think the second wrapper script in emacs-27.2 is overriding the invocation name, again.

Notify maintainers

@Stunkymonkey @alyssais @tadfisher (based on blame, not sure where the meta info is or if this is part of some specific package?)

Metadata

Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

[user@system:~]$ nix-shell -p nix-info --run "nix-info -m"
 - system: `"x86_64-linux"`
 - host os: `Linux 5.14.15, NixOS, 21.05.3980.f0869b1a2c0 (Okapi)`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.3.16`
 - channels(root): `"nixos-21.05.3980.f0869b1a2c0"`
 - channels(sheijk): `""`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixos`

Maintainer information:

# a list of nixpkgs attributes affected by the problem attribute:
- nixpkgs.emacs.emacsPackagesFor
# a list of nixos modules affected by the problem module:
- nixos.emacs
@sheijk sheijk added the 0.kind: bug Something is broken label Nov 10, 2021
@alyssais
Copy link
Member

alyssais commented Nov 10, 2021 via email

@veprbl veprbl added the 6.topic: emacs Text editor label Nov 11, 2021
@jcf
Copy link
Contributor

jcf commented Feb 9, 2024

I think I've tracked down the source of the problem, which affects versions of Emacs built with Home Manager.

The trivial runCommand builder has its name overridden to append with-packages, but the same modification is not made to emacs, which substitute depends on via prog. With such a change, the call to substitute should behave as expected.

substitute ${./wrapper.sh} $out/Applications/Emacs.app/Contents/MacOS/Emacs \
  --subst-var-by bash ${emacs.stdenv.shell} \
  --subst-var-by wrapperSiteLisp "$deps/share/emacs/site-lisp" \
  --subst-var-by wrapperSiteLispNative "$deps/share/emacs/native-lisp" \
  --subst-var-by prog "$emacs/Applications/Emacs.app/Contents/MacOS/Emacs"
chmod +x $out/Applications/Emacs.app/Contents/MacOS/Emacs
wrapProgramBinary $out/Applications/Emacs.app/Contents/MacOS/Emacs

If you have Home Manager setup, you can reproduce the issue with the following configuration:

{
  programs.emacs = {
    enable = true;

    package = pkgs.emacs-unstable-pgtk;

    extraPackages = epkgs: [epkgs.vterm];

    overrides = final: prev: {
      # `emacs-28` patches are compatible with `emacs-29`.
      #
      # Where a compatible path exists, there is a symlink upstream to keep
      # things clean, but GitHub doesn't follow symlinks to generate the
      # responses we need (instead GitHub returns the target of the symlink).
      patches =
        (prev.patches or [])
        ++ [
          # Fix OS window role (needed for window managers like yabai)
          (pkgs.fetchpatch {
            url = "https://raw.githubusercontent.com/d12frosted/homebrew-emacs-plus/master/patches/emacs-28/fix-window-role.patch";
            sha256 = "0c41rgpi19vr9ai740g09lka3nkjk48ppqyqdnncjrkfgvm2710z";
          })
          # Use poll instead of select to get file descriptors
          (pkgs.fetchpatch {
            url = "https://raw.githubusercontent.com/d12frosted/homebrew-emacs-plus/master/patches/emacs-29/poll.patch";
            sha256 = "0j26n6yma4n5wh4klikza6bjnzrmz6zihgcsdx36pn3vbfnaqbh5";
          })
          # Enable rounded window with no decoration
          (pkgs.fetchpatch {
            url = "https://raw.githubusercontent.com/d12frosted/homebrew-emacs-plus/master/patches/emacs-29/round-undecorated-frame.patch";
            sha256 = "0x187xvjakm2730d1wcqbz2sny07238mabh5d97fah4qal7zhlbl";
          })
          # Make Emacs aware of OS-level light/dark mode
          (pkgs.fetchpatch {
            url = "https://raw.githubusercontent.com/d12frosted/homebrew-emacs-plus/master/patches/emacs-28/system-appearance.patch";
            sha256 = "14ndp2fqqc95s70fwhpxq58y8qqj4gzvvffp77snm2xk76c1bvnn";
          })
        ];
    };
  };
}

@jcf
Copy link
Contributor

jcf commented Feb 10, 2024

Mentioning Emacs maintainers for input before I open a pull request to fix this: @AndersonTorres @adisbladis @Atemu @jwiegley @lovek323 @matthewbauer

I'm working on a reproducible example of this issue to ease testing, and I think I know what's causing the problem — while the runCommand call has -with-packages added, the prog substitution doesn't.

Would a pull request that applies the with-packages rename in both places solve this problem? If so, I'll work on something along those lines this weekend.


Update: I've tested my hypothesis locally, and it doesn't fix the problem. Executing the original Emacs derivation (sans with-packages suffix) is the desired behaviour. If, instead, you execute the with-packages version, Emacs never finishes launching.

Back to the drawing board!

@binarin
Copy link
Contributor

binarin commented Nov 28, 2024

The other thing appears to be affected by this - native compilation, which also uses invocation-name and invocation-directory - https://github.com/emacs-mirror/emacs/blob/8903106bb783c2825233c149b6799960aacdea57/lisp/emacs-lisp/comp-run.el#L302
And when emacs e.g. tries to automatically native-compile user's init.el, it can cause really funny side-effects during compilation, like (use-package :ensure t) installing package to user's .emacs.d directory (despite them already being installed emacsWithPackages)

The change introduced in #106486 (commit 23d4bfb) says that load-path sanitization is only for convenience when hacking on emacs itself or it's nixpkgs packaging.

Now that it's affecting 2 core features of emacs itself - emacs-restart and native compilation, maybe the convenience of emacs hacker/packager should be given a lower priority, with load-path sanitization removed or hidden behind an option. CC @alyssais

The last test case of the following shell script shows that without sanitization (as it's inhibited by -Q) nested emacs processes would be working in the way emacs itself expects them to work.

#!/usr/bin/env bash
nix build --impure --expr '
{ pkgs ? import <nixpkgs> {} }:
let
  myEmacs = pkgs.emacs-nox;
  emacsWithPackages = (pkgs.emacsPackagesFor myEmacs).emacsWithPackages;
in
  emacsWithPackages (epkgs:
    (with epkgs.elpaPackages; [ undo-tree ]))
'

E=./result/bin/emacs

cat <<'EOF' > test.el
(message "load-path has %s items" (length load-path))
(message "UNDO_TREE is installed?: %s" (package-installed-p (quote undo-tree)))
EOF

cat <<'EOF' > nested.el
(with-temp-buffer
  (call-process
   (expand-file-name invocation-name invocation-directory)
   nil
   (current-buffer)
   nil
   "-Q" ;; doesn't matter whether it's "-Q" or "-q"
   "--batch"
   "--load"
   "test.el")
  (message "NESTED:\n%s" (buffer-string)))
EOF

echo "======== Running directly: -q"
$E -q --batch --load test.el

echo -e "\n======== Running directly: -Q"
$E -Q --batch --load test.el

echo -e "\n======== Running as native compilation does it"
$E -q --batch --load nested.el

echo -e "\n======== Running as native compilation does it, but outer emacs with -Q - doesn't sanitize load-path"
$E -Q --batch --load nested.el

binarin added a commit to binarin/nixpkgs that referenced this issue Dec 2, 2024
fixes NixOS#145302 NixOS#237855

emacsWithPackages wrapper script/`site-start.el` sanitize
EMACSLOADPATH, to make nested emacs invocations independent of the
package set specified in emacsWithPackages.

But there are valid use cases when one needs to call nested emacs with
the same package set. This includes built-in emacs functionality such
as `restart-emacs` and async native compilations, and also external
packages like `emacs-async` and `esup`. In all of these cases
`invocation-directory`/`invocation-name` variables are being used to
launch nested emacs.

With this patch these variables will be populated to point to the
emacsWithPackages wrapper executable, so that executing
`(file-name-concat invocation-directory invocation-name)` will give a
fully functional emacs again.

`EMACSLOADPATH` sanitization was introduced by NixOS#106486, this behaviour
stays unchanged. The reasoning was to be able to run different emacs
executables without polluting their EMACSLOADPATH (as described here
NixOS@23d4bfb).
The only change is that invoking itself is again feasible (and that's
what emacs actually expects).
jian-lin added a commit to binarin/nixpkgs that referenced this issue Dec 6, 2024
fixes NixOS#145302 NixOS#237855

emacsWithPackages wrapper script/`site-start.el` sanitize
EMACSLOADPATH, to make nested emacs invocations independent of the
package set specified in emacsWithPackages.

But there are valid use cases when one needs to call nested emacs with
the same package set. This includes built-in emacs functionality such
as async native compilations, and also external packages like
`emacs-async` and `esup`. In all of these cases
`invocation-directory`/`invocation-name` variables are being used to
launch nested emacs.

With this patch these variables will be populated to point to the
emacsWithPackages wrapper executable, so that executing
`(file-name-concat invocation-directory invocation-name)` will give a
fully functional emacs again.

`EMACSLOADPATH` sanitization was introduced by NixOS#106486, this behaviour
stays unchanged. The reasoning was to be able to run different emacs
executables without polluting their EMACSLOADPATH (as described here
NixOS@23d4bfb).
The only change is that invoking itself is again feasible (and that's
what emacs actually expects).

Co-authored-by: Lin Jian <[email protected]>
@jian-lin
Copy link
Contributor

jian-lin commented Dec 6, 2024

fixed by #361145

@jian-lin jian-lin closed this as completed Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug Something is broken 6.topic: emacs Text editor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants