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

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

Open
schwanberger opened this issue May 23, 2024 · 10 comments
Open

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

schwanberger opened this issue May 23, 2024 · 10 comments

Comments

@schwanberger
Copy link
Contributor

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
Copy link
Owner

marienz commented May 23, 2024

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
Copy link
Owner

marienz commented May 25, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

marienz commented May 27, 2024

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
Copy link
Owner

marienz commented May 28, 2024

Looking more closely...

  • Does work in vanilla Doom
  • Vanilla Doom loads straight/build-29.3/tsc/tsc-dyn.so and straight/build-29.3/tree-sitter-langs/bin/go.so
  • Both have (tsc-dyn-get--loaded-version) report 0.18, and looking at https://github.com/emacs-tree-sitter/elisp-tree-sitter/tree/master/core that hasn't changed non-trivially after that version.
  • Conveniently, go.so is not stripped, so I can call tree_sitter_go() in it from a debugger and confirm it returns a TSLanguage struct with version 13.

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

emacs-tree-sitter/elisp-tree-sitter#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 doomemacs/doomemacs#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
Copy link
Contributor Author

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
Copy link
Owner

marienz commented May 29, 2024

There are other options I intend to look into:

  • Figure out how the grammar bundle tree-sitter-langs downloads is built. It may be easier to just rebuild this than I expect it to be.
  • Take an old nixpkgs as additional input, use its tree-sitter-grammars. Depending on how old a nixpkgs I need and how large the closure is that may be reasonable.

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
Copy link
Owner

marienz commented Jun 6, 2024

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
Copy link
Contributor Author

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)

doomemacs/doomemacs@ee10764

marienz referenced this issue Aug 28, 2024
tree-sitter-langs has special handling in nixpkgs, and our pinning
breaks assumptions it makes about version numbers. This causes
tree-sitter-langs to attempt to download and write a grammar bundle both
at build time (where the network request fails) and runtime (where it
causes an error on visiting a file supposed to be handled by
tree-sitter).

Fix up our derivation so it loads the Nix-provided grammar bundle.

This only partially fixes the problem: at least for Go, the
tree-sitter-langs package now loads, but visiting a Go file causes
`tsc-lang-abi-too-new 14 (13 . 13)`.

The included test fails without the fix in this commit, but does not
trigger this new failure.

Commit this partial fix because after
NixOS/nixpkgs@2421239 the build-time
failure became an error. This should fix CI and leave us no worse off
than before.
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

No branches or pull requests

2 participants