emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Tree-sitter introduction documentation


From: Eli Zaretskii
Subject: Re: Tree-sitter introduction documentation
Date: Fri, 30 Dec 2022 17:31:53 +0200

> From: Yuan Fu <casouri@gmail.com>
> Date: Fri, 30 Dec 2022 03:06:37 -0800
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>  Eli Zaretskii <eliz@gnu.org>,
>  Dmitry Gutov <dgutov@yandex.ru>,
>  theophilusx@gmail.com,
>  emacs-devel@gnu.org
> 
> > I have asked the question before, but freedom or not, the above is a
> > nuisance to run for every language.  If the process is as automatic as
> > the above example demonstrates, shouldn't Emacs have a command to take a
> > grammar and compile+install it?  I guess this could be more complicated
> > if the grammar is generated using a custom tool-chain for each language
> > (or is it always Javascript?), but nothing impossible.
> 
> Though the magic of programming, such command now exists: 
> treesit-install-language-grammar. It needs recipes to work, though. The 
> recipe would involve https://github.com, which I guess is probably too 
> heretical to include in Emacs source, so I left the recipes empty. I tested 
> the install command with these recipes:
> 
> (setq treesit-language-source-alist
>       '((python "https://github.com/tree-sitter/tree-sitter-python.git";)
>         (typescript 
> "https://github.com/tree-sitter/tree-sitter-typescript.git";
>                     "typescript/src" "typescript")))

Thanks.  I did some minor fixes to the doc strings, but this command
still "needs work"(TM).  See my comments below:

  This command requires Git, a C compiler and (sometimes) a C++ compiler,
  and the linker to be installed and on PATH.  It also requires that the
  recipe for LANG exists in `treesit-language-source-alist'.

I don't think treesit-language-source-alist is a good idea, especially
if we don't intend populating it, at least not as a user-facing
feature.  Instead, the command should ask the user for the relevant
values, and offer recording the values on some file that would be read
next time the user wants to install an updated library.

  OUT-DIR is the directory to put the compiled library file, it
  defaults to ~/.emacs.d/tree-sitter.

I don't understand what "defaults" means here, since OUT-DIR is not an
optional argument of treesit--install-language-grammar-1.

  (let* ((lang (symbol-name lang))
         (default-directory "/tmp")

A literal "/tmp" is not portable and un-Emacsy; please use
temporary-file-directory instead.

         (soext (pcase system-type
                  ('darwin "dylib")
                  ((or 'ms-dos 'cywin 'windows-nt) "dll")

MS-DOS doesn't use DLL files.  Please use dynamic-library-suffixes
instead, it's already set up correctly.  And the code should be ready
for that variable having a nil value.

          (message "Cloning repository")
          ;; git clone xxx --depth 1 --quiet workdir
          (treesit--call-process-signal
           "git" nil t nil "clone" url "--depth" "1" "--quiet"
           workdir)

Why "--depth 1"?  This should be a defcustom, and the default should
be to clone the full repository, IMO.  Also, what about updating the
library when it is already installed, and the Git repository already
exists for it?  Or are we going to clone anew each time and them
remove the repository? that could make its cloning be slow in some
cases.

          ;; cp "${grammardir}"/grammar.js "${sourcedir}"
          (copy-file (concat grammar-dir "/grammar.js")
                     (concat source-dir "/grammar.js"))

Why is this part needed?  In any case, please don't use concat to
produce file names, use expand-file-name instead.  Also, we should
call copy-file with 4th argument non-nil, I think.

          (treesit--call-process-signal
           cc nil t nil "-fPIC" "-c" "-I." "parser.c")

I wonder why we don't use 'compile' here.  That would show the
compiler output without any extra efforts.

          ;; Copy out.
          (copy-file lib-name (concat out-dir "/") t)

See above: don't use concat here.

This command should also be mentioned in NEWS, where we describe how
to install the grammar libraries.

Bottom line: I think we need first to discuss how we want such a
facility to work, and only then implement it.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]