search

marienz / nix-doom-emacs-unstraightened

Builds Doom Emacs using Nix
Apache License 2.0
27 stars 1 forks source link

tree-sitter issue due to read-only filesystem #7

Open schwanberger opened 1 month ago

schwanberger commented 1 month ago

Hi!

First off, I would also like to say thank you very much - and wow - well done. I love emacs and nix and I really appreciate this project as I can deepen my understanding of both (especially where they intersect).

Using my config that includes the tree-sitter module I encounter this error when first opening a .go file. I suspect the issue appear in general for all languages. It tries to download the grammar.

Error running timer: (doom-hook-error go-mode-local-vars-hook tree-sitter! (error "Eager macro-expansion failure: (file-error \"Opening output file\" \"Read-only file system\" \"/nix/store/7qda5ymlh0sgqrb5p5bjc6lvgl6p4qcy-emacs-tree-sitter-grammars/langs/bin/tree-sitter-grammars.x86_64-unknown-linux-gnu.v0.12.167.tar.gz\")"))

I believe (am unsure) that the download does not happen in my non-unstraightened setup since I'm including the emacs pkg treesit-grammars.with-all-grammars in a emacsWithPackagesFromUsePackage function. I expect this includes all the grammers available so no download is required at runtime.

Would it make sense to include an "extraEmacsPackages" configuration option for unstraightened? It could be a merge (with "extraEmacsPackages" being right side) into the set containing the packages deduced/included from doom config.

If you agree I would like to take a stab at it and submit a pull request (Will remember to include the CLA).

Any other suggestion is also welcome of course.

marienz commented 1 month ago

Glad you like it! It's been an interesting puzzle making Doom and Nix work together. There are definitely some rough edges but I think it speaks to the strengths of both projects that this entire contraption works at all (without requiring invasive patching of Doom or a ton of custom derivations, there are a few of both but so far they seem manageable).

I believe (am unsure) that the download does not happen in my non-unstraightened setup since I'm including the emacs pkg treesit-grammars.with-all-grammars in a emacsWithPackagesFromUsePackage function. I expect this includes all the grammers available so no download is required at runtime.

Interesting! Possibly related:

> nix log /nix/store/vjfgzmrrmd775ni0nhxa0nhnrwvj0ii0-emacs-tree-sitter-langs-9999snapshot
...
tsc-dyn-get: Using source :github (:loaded nil :recorded 0.18.0 :requested 0.18.0)
tsc-dyn-get: Recorded version already satifies requested -> loading
tree-sitter-langs: Installing grammar bundle v0.12.167 (was v0.12.196)
Contacting host: github.com:443

In toplevel form:
tree-sitter-langs.el:63:2: Error: github.com/443 Temporary failure in name resolution
...
tsc-dyn-get: Using source :github (:loaded nil :recorded 0.18.0 :requested 0.18.0)
tsc-dyn-get: Recorded version already satifies requested -> loading
tree-sitter-langs: Installing grammar bundle v0.12.167 (was v0.12.196)
Contacting host: github.com:443

Error: error ("/nix/store/vjfgzmrrmd775ni0nhxa0nhnrwvj0ii0-emacs-tree-sitter-langs-9999snapshot/share/emacs/site-lisp/elpa/tree-sitter-langs-9999snapshot/tree->
  mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode -0xe7f8fdfc510e41>))
  debug-early-backtrace()
  debug-early(error (error "/nix/store/vjfgzmrrmd775ni0nhxa0nhnrwvj0ii0-emacs-tree-sitter-langs-9999snapshot/share/emacs/site-lisp/elpa/tree-sitter-langs-9999s>
  signal(error ("/nix/store/vjfgzmrrmd775ni0nhxa0nhnrwvj0ii0-emacs-tree-sitter-langs-9999snapshot/share/emacs/site-lisp/elpa/tree-sitter-langs-9999snapshot/tre>
  comp--native-compile("/nix/store/vjfgzmrrmd775ni0nhxa0nhnrwvj0ii0-emacs-tree-sitter-langs-9999snapshot/share/emacs/site-lisp/elpa/tree-sitter-langs-9999snaps>
  batch-native-compile()
  command-line-1(("--eval" "(setq large-file-warning-threshold nil)" "-f" "batch-native-compile" "/nix/store/vjfgzmrrmd775ni0nhxa0nhnrwvj0ii0-emacs-tree-sitter>
  command-line()
  normal-top-level()

Building tree-sitter-langs attempts to download a grammar bundle from Github, which fails in Nix's build sandbox. I'd seen that one scroll by but not gotten around to figuring out if it broke anything at runtime...

I think you've found the answer: opening a Go file with tree-sitter and (go +tree-sitter) modules and Doom debug mode enabled gets me: 

  error("Eager macro-expansion failure: %S" (file-error "Opening output file" "Read-only file system" "/nix/store/7qda5ymlh0sgqrb5p5bjc6lvgl6p4qcy-emacs-tree-sitter-grammars/langs/bin/tree-sitter-grammars.x86_64-unknown-linux-gnu.v0.12.167.tar.gz"))
  internal-macroexpand-for-load((eval-and-compile (unless (bound-and-true-p tree-sitter-langs--testing) (tree-sitter-langs-install-grammars :skip-if-installed))) nil)
  load-with-code-conversion("/nix/store/lim2yfilmxcxcvpss1fnfqlih0lfxy9x-emacs-..." "/nix/store/lim2yfilmxcxcvpss1fnfqlih0lfxy9x-emacs-..." nil t)
  require(tree-sitter-langs)

That looks like the same error you're getting. And it looks like the same function that was failing at build time, which this time can contact github but can't write its output.

tree-sitter-langs is a manual package in nixpkgs (see here), and the comment "Fake same version number as upstream language bundle to prevent triggering runtime downloads" in there seems pretty relevant. I seem to be breaking that (but I haven't entirely worked out how yet).

Would it make sense to include an "extraEmacsPackages" configuration option for unstraightened? It could be a merge (with "extraEmacsPackages" being right side) into the set containing the packages deduced/included from doom config.

Hm!

I'd previously considered something like the hook you're proposing, but hoped I could get away without it because adding (package! treesit-grammars) is equivalent to using that hook to add treesit-grammars. But that's not going to work for treesit-grammars.with-all-grammars, let alone for treesit-grammars.with-grammars (p: ...)...

Assuming you mean a hook similar to the extraEmacsPackages provided by emacsWithPackagesFromUsePackage (taking a function from Emacs package set to list of packages) then that seems easy enough to add (just call it from "step 3" in default.nix, adding to our own list).

If you're ok dealing with the CLA (sorry about that...) then I'd be happy to accept a PR.

But if I do:

diff --git a/default.nix b/default.nix
index 77777a0..09667a7 100644
--- a/default.nix
+++ b/default.nix
@@ -273,7 +273,7 @@ let
   # Step 3: Build an emacsWithPackages, pulling all packages from step 1 from
   # the set from step 2.
   emacsWithPackages = doomEmacsPackages.emacsWithPackages
-    (epkgs: (map (p: epkgs.${p}) (builtins.attrNames doomPackageSet)));
+    (epkgs: [(map (p: epkgs.${p}) (builtins.attrNames doomPackageSet))] ++ [epkgs.treesit-grammars.with-all-grammars]);

   # Step 4: build a DOOMDIR, Doom profile and profile loader using Emacs from
   # step 3 and packages.el from step 1.

to force treesit-grammars.with-all-grammars I still get the same error, so I don't think that by itself this is going to fix your problem (still happy to accept that PR if this grammars package does something useful once that error's fixed, though!)

marienz commented 1 month ago

https://github.com/marienz/nix-doom-emacs-unstraightened/tree/tree-sitter has a partial fix, but hits a second problem:

File local-variables error: (doom-hook-error go-mode-local-vars-hook tree-sitter! (tsc-lang-abi-too-new 14 (13 . 13) /nix/store/nac5r22p9fa1awvy8d08rvppyvsg5wnn-emacs-tree-sitter-grammars/langs/bin/go.so))

I suspect but haven't confirmed yet that the (13 . 13) there is the supported ABI range from tsc-dyn (/nix/store/bx2sx7s4jpnrnbq8qdbgy7dyqg9xrzk5-tsc-dyn-0.18.0/share/emacs/site-lisp/elpa/tsc-0.18.0/tsc-dyn.so, https://github.com/emacs-tree-sitter/elisp-tree-sitter/blob/02fe7b86d92b1aab954045146469b7893f0ab371/core/src/lang.rs#L172), in which case that error translates to "tsc-dyn is too old for the grammar". Not sure how to fix that one yet (it might not be "my" bug, it might be version skew in nixpkgs this time...)

schwanberger commented 1 month ago

Yes, I ran into that as well. Seeing as Doom Emacs will be moving to the Emacs built-in treesitter in the future, I then decided to use the package treesit-auto and treesit-grammars.with-all-grammars and unpin eglot after which there is no conflict.

I suspect that this is not really satisfactory for you though.

I've also put a lot of effort into trying to make the override I mentioned work but I'm not having any luck. The furthest I've gotten is to use mkConfig to create a list of strings and simply doing the same map shenanigans as you're doing in the diff above. That works for simple package names like "vterm" but not for "treesit-grammars.with-all-grammars"

I've since ventured deep into pkgs.override and pkgs.emacs.pkgs.emacsWithPackages { override = .... } territory but no luck. Trying to override the drvAttrs.deps (or something similar) of emacsWithPackages = doomEmacsPackages.emacsWithPackages ... seems way to hacky and low-level.

It all boils down to me wanting to have an extraPackages config option defined as a function/lambda like so:

extraPackages = epkgs: [ epkgs.vterm epkgs.treesit-grammars.with-all-grammars ]

But I've been unable to use that directly in the emacsWithPackages = doomEmacsPackages.emacsWithPackages line.

Do you have a good idea/proposal for how to extend an already defined set of "emacs-packages"?

schwanberger commented 1 month ago

I've finally arrived at a solution that works and I will submit a pull request (with CLA) sometime next week.

In summary:

Add an extraPackages home-manager configuration option that declares function to be consumed by doomEmacsPackages.emacsWithPackages, e.g.

  programs.doom-emacs = {
    enable = true;
    doomDir = inputs.doom-config;
    emacs = my-emacs-unstable;
    extraPackages = epkgs: [ epkgs.vterm epkgs.treesit-grammars.with-all-grammars ];
    provideEmacs = false;
  };

The way the function is used is like so in default.nix:

    emacsWithPackages = doomEmacsPackages.emacsWithPackages
      (epkgs: (map (p: epkgs.${p}) (builtins.attrNames doomPackageSet)) ++ (extraPackages doomEmacsPackages));

What do you think?

marienz commented 1 month ago

Looks good to me!

(I do want to dig into what's going wrong with tree-sitter, but I'm a little sidetracked with CI and other improvements...)

marienz commented 1 month ago

Looking more closely...

So it does look like our go.so (from tree-sitter-grammars / tree-sitter-go-grammar) is too new. But I don't really want to pull in the pre-compiled bundle that makes this work with vanilla Doom...

https://github.com/emacs-tree-sitter/elisp-tree-sitter/issues/247 looks related, and says to use Emacs 29's built-in tree-sitter support instead.

That should load libraries from treesit-extra-load-path, which tree-sitter-grammars is not on, but it still won't work if it is because it looks for a differently-named file:

ELISP> (treesit-language-available-p 'go t)
(nil not-found
     ("libtree-sitter-go" "libtree-sitter-go.0" "libtree-sitter-go.0.0" "libtree-sitter-go.so" "libtree-sitter-go.so.0" "libtree-sitter-go.so.0.0")
     "No such file or directory")

But (treesit-library-abi-version) confirms Emacs supports ABI version 14, so its built-in support should work if fed a suitably-structured link farm for grammars.

I suspect Doom won't then automatically use the built-in support, though... That looks like it'd be https://github.com/doomemacs/doomemacs/issues/7623.

The other option would be "build grammars for the old ABI". But assuming https://github.com/tree-sitter/tree-sitter-go/blob/master/src/parser.c is typical, those are generated c source with the language version hardcoded into them: that approach doesn't look practical.

So I don't think I can really reasonably fix this until Doom switches to built-in treesitter...

schwanberger commented 1 month ago

I believe you are right.

Perhaps we should close this issue? If you'd like, I can detail the workaround I'm using in the readme as part of the PR together with a link to this issue.

The nifty thing about the workaround is that it is both nix and non-nix compatible due to treesit-auto implements automatic download at runtime. For nix I prefer to provide it with the deps up front.

marienz commented 1 month ago

There are other options I intend to look into:

If I understand the problem correctly, it boils down to unfortunate timing: tree-sitter going through an ABI change around the same time Emacs transitions to built-in tree-sitter support. So this will eventually get fixed, but depending on what Doom decides to do about Emacs 28 support it may take a while (and if it does I'd really prefer not to declare all of tree-sitter unsupported until then...)

marienz commented 4 weeks ago

Keeping this open as I think it's doable to build tree-sitter-langs with old ABI (which I'll try to land in nixpkgs upstream but may carry locally).

For anyone running into this until then: please try the workaround schwanberger@ just added (thanks!)

schwanberger commented 1 week ago

Keeping this open as I think it's doable to build tree-sitter-langs with old ABI (which I'll try to land in nixpkgs upstream but may carry locally).

For anyone running into this until then: please try the workaround schwanberger@ just added (thanks!)

Hi. Have you seen the recent commits to doom emacs? This just might have been solved upstream. (Haven't tested)

https://github.com/doomemacs/doomemacs/commit/ee10764c220c790625381c245416cb0864a4168d