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

fix(build): prefix load paths instead of set #185

Merged
merged 1 commit into from
Dec 22, 2024

Conversation

jordanisaacs
Copy link
Contributor

@jordanisaacs jordanisaacs commented Dec 20, 2024

Allows callers to add their EMACSNATIVELOADPATH. This is a workaround for a bug where --init-directory is set after the user eln cache is set up. By allowing the caller to add the eln cache directory in the env variable, emacs will be able to discover the byte compiled {early-}init.el files.

@jordanisaacs jordanisaacs requested a review from akirak as a code owner December 20, 2024 22:06
pkgs/emacs/wrapper.nix Outdated Show resolved Hide resolved
@akirak
Copy link
Member

akirak commented Dec 21, 2024

Thanks for suggesting this PR. Before checking the content, I would like to identify the problem.

one can now prefix EMACSNATIVELOADPATH for a byte-compiled early-init.el/init.el

Since #178 has been merged, you can use lib.buildElispPackage to byte-compile init.el, as terlar does. Will this PR solve a new problem, or solve the existing problem in a better way? It looks like you two have similar but slightly different Nix configurations. I don't precisely understand what experience you are trying to achieve, so I want to know.

@jordanisaacs
Copy link
Contributor Author

jordanisaacs commented Dec 21, 2024

My config was originally based on terlar's. I also wanted a custom early-init.el/init.el. I approached it a different route using --init-directory emacs flag so it would work just by installing the package (no home-manager needed).

When cleaning up my config I realized it wasn't actually working. early-init.el wasn't using the byte compiled version since it couldn't find the eln file. init.el was using a byte-compiled version but not the nix one, emacs was auto-compiling to a mutable cache directory because it couldn't find it. I started getting byte compilation errors when making all cache directories part of the nix store.

I can't fix this in early-init.el because at that point early-init.el is already loaded without its eln file. The only solution is to use EMACSNATIVELOADPATH. Unless I am missing something, always open to a better approach.

@jordanisaacs
Copy link
Contributor Author

The build elisp function is tangential, I should probably migrate my build process to it :) But my understanding is that doesn't solve the runtime lookup of the built eln files.

@jordanisaacs
Copy link
Contributor Author

See: https://github.com/emacs-mirror/emacs/blob/e59e7278924cd0dca49d4333dba188530721f5a3/lisp/startup.el#L610 where it gets the load paths. We need to make sure the location of early-init.eln is there.

@jordanisaacs
Copy link
Contributor Author

jordanisaacs commented Dec 21, 2024

The reason it works when not using --init-directory is it pushes user-emacs-directory "eln-cache" in addition to the env variable. Setting --init-directory does not update user-emacs-directory so it still can't be found.

edit: It is actually more subtle then that. --init-directory does set user-emacs-directory but after the variable is used to push the extra user directory eln-cache. This should probably be fixed upstream (so user eln-cache is pushed after processing command line) but this is a workaround for the time being.

There is a second behavior here to keep in mind even if if this is fixed upstream. Emacs startup assumes the top of the native-comp-eln-load-path is the user cache directory. And it pops and then re-adds an updated cache directory of you later update emacs-user-directory. So if the early-init.el redirects the user-emacs-directory but still is byte compiled it needs the following to make sure the custom eln-cache doesn't get removed from the load list:

(push (expand-file-name "eln-cache/" user-emacs-directory) (cdr native-comp-eln-load-path))
(setq user-emacs-directory (expand-file-name "emacs/" (getenv "XDG_STATE_HOME")))

@akirak
Copy link
Member

akirak commented Dec 22, 2024

Thank you for looking into the problem space and elaborating. Sounds more difficult than it should be. Why do you want to load the native-compiled early-init.el? Is it intended to improve the startup time?

@jordanisaacs
Copy link
Contributor Author

Startup time and if you don't byte compile it, emacs byte compiles early-init.el it in the background. I'll update the diff to remove prefixing the EMACSLOADPATH and just allow it for EMACSNATIVELOADPATH

@akirak
Copy link
Member

akirak commented Dec 22, 2024

emacs byte compiles early-init.el it in the background

This isn't the case with my config (installed via home-manager), but if it is, the fix should be the default, and not a customization feature.

@akirak
Copy link
Member

akirak commented Dec 22, 2024

Twist doesn't integrate with package.el. I think you could move part of your early-init.el to init.el, because your Emacs instance shouldn't enable package.el. Is it still important to compile early-init.el for improved startup time?

Allows callers to add their own native load paths. See PR for a long winded
explanation of why it is useful for users of `--init-directory`.
@jordanisaacs
Copy link
Contributor Author

This entire thing is a workaround for users of --init-directory, we can't fix it by default in twist.nix because twist.nix doesn't know where the user needs their eln-cache to be. You don't hit this bug because your eln-cache is discoverable by using home-manager, it does the xdg lookup setting user-emacs-directory before setting user eln-cache, see below excerpt. No matter what if I want an early-init.el emacs is going to try to byte-compile it when it doesn't find it (unless I explictly disable it which is even worse imo).

    (setq startup--xdg-config-home-emacs
	  (let ((xdg-config-home (getenv-internal "XDG_CONFIG_HOME")))
	    (if xdg-config-home
		(concat xdg-config-home "/emacs/")
	      startup--xdg-config-default)))
    (setq user-emacs-directory
	  (startup--xdg-or-homedot startup--xdg-config-home-emacs nil))

    (when (featurep 'native-compile)
      (unless (native-comp-available-p)
        ;; Disable deferred async compilation and trampoline synthesis
        ;; in this session.  This is necessary if libgccjit is not
        ;; available on MS-Windows, but Emacs was built with
        ;; native-compilation support.
        (setq native-comp-jit-compilation nil
              native-comp-enable-subr-trampolines nil))

      ;; Form `native-comp-eln-load-path'.
      (let ((path-env (getenv "EMACSNATIVELOADPATH")))
        (when path-env
          (dolist (path (split-string path-env path-separator))
            (unless (string= "" path)
              (push path native-comp-eln-load-path)))))
      (push (expand-file-name "eln-cache/" user-emacs-directory)
            native-comp-eln-load-path))

obviously the correct fix is upstream, but for now its a one liner workaround.

@akirak
Copy link
Member

akirak commented Dec 22, 2024

Okay, this change will not cause any regression to users of home-manager, so I'll merge it. I may override the change in the future depending on priorities.

@akirak akirak merged commit 1338c59 into emacs-twist:master Dec 22, 2024
1 check passed
@akirak
Copy link
Member

akirak commented Dec 22, 2024

@jordanisaacs Thank you for your patience and investigating the issue.

@jordanisaacs
Copy link
Contributor Author

I reported an upstream bug https://debbugs.gnu.org/cgi/bugreport.cgi?bug=75022, will revert this change if/once a fix lands https://debbugs.gnu.org/cgi/bugreport.cgi?bug=75022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants