diff mbox series

[bug#49946,v5,27/27] gnu: Add emacs-tree-sitter-langs.

Message ID 87bkvz54cg.fsf@gmx.com
State New
Headers show
Series None | expand

Commit Message

Pierre Langlois May 15, 2022, 12:20 p.m. UTC 
Hi Maxime, sorry for the late reply!

Maxime Devos <maximedevos@telenet.be> writes:

> [[PGP Signed Part:Undecided]]
> Pierre Langlois schreef op di 29-03-2022 om 20:43 [+0100]:
>> +                  ;; The BUNDLE-VERSION file prevents emacs-tree-sitter-langs
>> +                  ;; from downloading libraries at load time.
>
> WDYT of patching emacs-tree-sitter-langs to not download, such that it
> doesn't download and run non-Guix libraries behind the user's back?

The way the current patchset works, by providing a compatible bundle, we
already prevent emacs-tree-sitter-langs from downloading binaries by
default.  I agree we could go further though, and entirely remove the
code that downloads binaries.

However I'm not sure about it.  Essentially, there is a
tree-sitter-langs-build.el file [0] that can either download binaries or
fetch sources and compile locally.  So a user could decide to opt-out of
using Guix binaries and instead use an alternative method.

I 100% agree that by default we should make the package use Guix
libraries, this way it also /just works/.  However, if somebody wants to
do things differently, I'm not sure we want to get in the way.  In the
end, this isn't so different from letting people use an alternative
package manager if they like.  We'd rather people used Guix of course :-).

I don't have a really strong opinion about this though, so if you still
prefer to delete the downloading code, I've attached an example patch
that entirely replace it with a bare-bones implementation as an example.

Let me know what you think!

[0]: https://github.com/emacs-tree-sitter/tree-sitter-langs/blob/master/tree-sitter-langs-build.el

> Also, why do we need a bundle at all, would simply installing emacs-
> tree-sitter, and, e.g., tree-sitter-java, just work?

Yeah, having a bundle is fundamentally how this package works AFAICT.

I think the main reason is that in order to do highlighting effectively
in emacs, it's not enough to install the tree-sitter runtime and a
tree-sitter-<lang> grammar.  You also want to "configure" how the
highlighting is done by providing custom "queries" expressions.

And this package provides queries for every language that it supports:
https://github.com/emacs-tree-sitter/tree-sitter-langs/tree/master/queries

All that being said, I believe that long-term the idea is that upstream
language-specific packages would eventually gain support for tree-sitter
and then this bundle "glue" package will no longer be necessary.
Especially if one day emacs proper gains native support for tree-sitter
(I think I saw some discussions about that on emacs-devel last year).

But given this package is quite useful though, I'd be surprised if it
goes away soon.

Hope this makes sense!

Thanks,
Pierre

>
> Greetings,
> Maxime.
>
> [[End of PGP Signed Part]]

Comments

M May 15, 2022, 2:33 p.m. UTC | #1 
Pierre Langlois schreef op zo 15-05-2022 om 13:20 [+0100]:
> I 100% agree that by default we should make the package use Guix
> libraries, this way it also /just works/.  However, if somebody wants
> to
> do things differently, I'm not sure we want to get in the way.  In
> the
> end, this isn't so different from letting people use an alternative
> package manager if they like.  We'd rather people used Guix of course
> :-).

OK, my proposal to prevent any accidents (what if we misinterpreted
BUNDLE-VERSION / the semantics change in a future version, and now
binaries are downloaded by default), not to stop people from choosing
to do.  Seems a bit risky though (see the bit about future changes in
semantics).

Greetings,
Maxime.
Pierre Langlois May 15, 2022, 2:55 p.m. UTC | #2 
Maxime Devos <maximedevos@telenet.be> writes:

> [[PGP Signed Part:Undecided]]
> Pierre Langlois schreef op zo 15-05-2022 om 13:20 [+0100]:
>> I 100% agree that by default we should make the package use Guix
>> libraries, this way it also /just works/.  However, if somebody wants
>> to
>> do things differently, I'm not sure we want to get in the way.  In
>> the
>> end, this isn't so different from letting people use an alternative
>> package manager if they like.  We'd rather people used Guix of course
>> :-).
>
> OK, my proposal to prevent any accidents (what if we misinterpreted
> BUNDLE-VERSION / the semantics change in a future version, and now
> binaries are downloaded by default), not to stop people from choosing
> to do.  Seems a bit risky though (see the bit about future changes in
> semantics).

Ah I see, that's a good point.  I /think/ today we should be able to
catch such things thanks to the tests, if the package decides to
download binaries during testing it'll fail.  But maybe it could, for
some reason, use Guix binaries during testing and then decide to
download its own when running outside of the build environment.

Now I'm conlicted :-), maybe rewriting the build.el file is the safest
option, and maybe if we get bug reports from people who would like to
use their own grammars we could revisit?
M May 15, 2022, 4:05 p.m. UTC | #3 
Pierre Langlois schreef op zo 15-05-2022 om 15:55 [+0100]:
> [...]
> 
> Ah I see, that's a good point.  I /think/ today we should be able to
> catch such things thanks to the tests, if the package decides to
> download binaries during testing it'll fail.  But maybe it could, for
> some reason, use Guix binaries during testing and then decide to
> download its own when running outside of the build environment.
> 
> Now I'm conlicted :-), maybe rewriting the build.el file is the safest
> option, and maybe if we get bug reports from people who would like to
> use their own grammars we could revisit?

Seems like an option to me (I don't think we actually know yet if there
will be interest in downloading the grammars from upstream or not among
potential Guix users).  Though FWIW, they can already use their own
grammars with "--with-source" transformations (to change an existing
grammar) or by packaging them (for new grammars) and the like (albeit
with Guix instead of Emacs).

Greetings,
Maxime.
diff mbox series

Patch

From 7ad62ccef2446011dfbdfb2dbe8cc58f46fb05d8 Mon Sep 17 00:00:00 2001
From: Pierre Langlois <pierre.langlois@gmx.com>
Date: Sat, 2 Apr 2022 19:22:52 +0100
Subject: [PATCH] wip

---
 gnu/packages/tree-sitter.scm | 49 ++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/gnu/packages/tree-sitter.scm b/gnu/packages/tree-sitter.scm
index 7e14ebd1e3..d6892db6e3 100644
--- a/gnu/packages/tree-sitter.scm
+++ b/gnu/packages/tree-sitter.scm
@@ -795,11 +795,11 @@  (define-public emacs-tree-sitter
 @end enumerate")
     (license license:expat)))

-(define (make-emacs-tree-sitter-langs-grammar-bundle version)
+(define emacs-tree-sitter-langs-grammar-bundle
   (package
     (name "emacs-tree-sitter-langs-grammar-bundle")
     (source #f)
-    (version version)
+    (version (package-version tree-sitter))
     (build-system trivial-build-system)
     (inputs
      ;; FIXME: Support for some languages is still left to package.
@@ -836,11 +836,7 @@  (define (make-emacs-tree-sitter-langs-grammar-bundle version)
                           (map (match-lambda
                                  ((name directory)
                                   (string-append directory "/lib/tree-sitter")))
-                               '#$(package-inputs this-package))))
-                  ;; The BUNDLE-VERSION file prevents emacs-tree-sitter-langs
-                  ;; from downloading libraries at load time.
-                  (call-with-output-file (string-append #$output "/BUNDLE-VERSION")
-                    (lambda (port) (display #$version port)))))))
+                               '#$(package-inputs this-package))))))))
     (synopsis #f)
     (description #f)
     (home-page #f)
@@ -861,7 +857,7 @@  (define-public emacs-tree-sitter-langs
                 "1p2zbb6ac7wi6x6zpbczcmpkb2p45md2csd2bj43d8s56ckzw5mp"))))
     (build-system emacs-build-system)
     (inputs
-     (list (make-emacs-tree-sitter-langs-grammar-bundle version)))
+     (list emacs-tree-sitter-langs-grammar-bundle))
     (propagated-inputs
      (list emacs-tree-sitter))
     (arguments
@@ -870,15 +866,36 @@  (define-public emacs-tree-sitter-langs
       #:test-command ''("script/test")
       #:phases
       #~(modify-phases %standard-phases
+          (add-after 'unpack 'disable-downloader
+            (lambda _
+              (call-with-output-file "tree-sitter-langs-build.el"
+                (lambda (port)
+                  (let ((on-load-message
+                         (string-append
+                          "tree-sitter-langs: Grammar bundle already installed "
+                          "via Guix.  Installing external grammars via this "
+                          "function isn't supported, if a language you need is "
+                          "missing please report a bug at bug-guix@gnu.org.")))
+                    (format
+                     port
+                     ";;;###autoload
+                      (defun tree-sitter-langs-install-grammars
+                             (&optional skip-if-installed version os
+                                        keep-bundle)
+                        (interactive)
+                        (message \"~a\"))
+                      (defconst tree-sitter-langs--queries-dir
+                        (file-name-as-directory
+                          (concat (file-name-directory (locate-library \"tree-sitter-langs.el\"))
+                                  \"queries\")))
+                      (defun tree-sitter-langs--bin-dir () \"~a\")
+                      (provide 'tree-sitter-langs-build)"
+                     on-load-message
+                     #$emacs-tree-sitter-langs-grammar-bundle))))))
           (add-after 'unpack 'remove-cask
             (lambda _
               (substitute* "script/test"
                 (("cask") ""))))
-          (add-before 'check 'bundle-for-testing
-            (lambda* (#:key inputs #:allow-other-keys)
-              (delete-file-recursively "bin")
-              (symlink #$(make-emacs-tree-sitter-langs-grammar-bundle version)
-                       "bin")))
           (add-before 'check 'patch-tree-sitter-require-test
             (lambda _
               (use-modules (ice-9 regex))
@@ -906,12 +923,6 @@  (define-public emacs-tree-sitter-langs
             (lambda _
               (delete-file-recursively "queries/hcl")
               (delete-file-recursively "queries/pgn")))
-          (add-before 'install 'install-bundle
-            (lambda _
-              (let ((elpa (elpa-directory #$output)))
-                (mkdir-p elpa)
-                (symlink #$(make-emacs-tree-sitter-langs-grammar-bundle version)
-                         (string-append elpa "/bin")))))
           (add-after 'install 'install-queries
             (lambda* (#:key outputs #:allow-other-keys)
               (let ((elpa (elpa-directory (assoc-ref outputs "out"))))
--
2.36.0