Subject: Re: [bug#49946] [PATCH v7 01/32] gnu: tree-sitter: Move to its own module.

From: Pierre Langlois <[email-masked]>
Date: Fri, 10 Feb 2023 17:02:31 +0000

Pierre Langlois <[email-masked]> writes:

> [[PGP Signed Part:Undecided]]
> Hi Andrew, thanks for pushing this along! It's great to see things
> getting merged.
>
> Andrew Tropin <[email-masked]> writes:
>
>> [[PGP Signed Part:Undecided]]
>> On 2023-02-09 18:04, Andrew Tropin wrote:
>>
>>> On 2023-02-09 13:39, zimoun wrote:
>>>
>>>> Hi,
>>>>
>>>> On Thu, 09 Feb 2023 at 14:11, Andrew Tropin <[email-masked]> wrote:
>>>>
>>>>> I applied tree-sitter and tree-sitter-cli patches,
>>>>
>>>> Just to be sure to understand, you have only applied 02/32 and 05/32,
>>>> right?
>>>>
>>>>
>>>> [bug#49946] [PATCH v7 02/32] gnu: tree-sitter: Update to 0.20.7.
>>>> id:[email-masked]
>>>> http://issues.guix.gnu.org/msgid/[email-masked]
>>>>
>>>> [bug#49946] [PATCH v7 05/32] gnu: Add tree-sitter-cli.
>>>> id:[email-masked]
>>>> http://issues.guix.gnu.org/msgid/[email-masked]
>>>>
>>>> Leaving out all the others, right?
>>>
>>> Merged first 5 patches from 01 to 05, also added one more commit, which
>>> addresses some things from reviews and one commit, which adds html
>>> grammar.
>>>
>>> The html grammar is added for the testing purposes.  It relies on
>>> generated parser.c and scanner.c and we will need to repackage it using
>>> grammar.js instead.  I'm not sure if a separate build system is needed
>>> for this, I guess we can just rewrite tree-sitter-grammar function,
>>> which generates packages as in example with tree-sitter-grammar-html:
>>> https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/tree-sitter.scm?h=53b00b91b73bd60412d5bd057e22e6d63194a7f7#n158
>>>
>>> Anyway, I only skimmed tree-sitter-build-system source code, and plan to
>>> read it carefully, evaluate and either introduce new build system or
>>> just move all needed parts to tree-sitter-grammar function.  WDYT?
>>> After we done with it we can package all other grammars.
>>
>> Ok, I realized that the proper build process for tree-sitter grammars is
>> a little harder than I expected, tree-sitter-build system make sense.  I
>> reviewed it, made a small change:
>
> Ah great, I was going to comment to try and push for us to keep the
> build system. I originally went with a template package and inheritance,
> but Maxime suggested moving to a build-system which ended up making the
> package definitions a *lot* nicer IMO (see previous discussion here
> https://issues.guix.gnu.org/49946#144). It also allows us to deal with
> grammars that depend on each other more nicely I think.
>
>>
>> @@ -29,7 +29,7 @@ (define-module (guix build tree-sitter-build-system)
>>  ;; Commentary:
>>  ;;
>>  ;; Build procedures for tree-sitter grammar packages.  This is the
>> -;; builder-side code, which builds on top fo the node build-system.
>> +;; builder-side code, which builds on top of the node build-system.
>>  ;;
>>  ;; Tree-sitter grammars are written in JavaScript and compiled to a native
>>  ;; shared object.  The `tree-sitter generate' command invokes `node' in order
>> @@ -114,7 +114,7 @@ (define (compile-language dir)
>>                     "-fno-exceptions"
>>                     "-O2"
>>                     "-g"
>> -                   "-o" ,(string-append lib "/" lang ".so")
>> +                   "-o" ,(string-append lib "/libtree-sitter-" lang ".so")
>>                     ;; An additional `scanner.{c,cc}' file is sometimes
>>                     ;; provided.
>>                     ,@(cond
>>
>>
>> rewrote html grammar to use this build system and made it work with
>> built-in treesit package.  Also, tried examples of c and cpp grammars
>> from patches in this thread.
>>
>> If you ok with it, I'll push the build system to master and update the
>> html grammar accordingly.

Oh, I forgot to say, this change to the build system LGTM! I'm really
happy to see it merged soon :-). The path change will probably break the
emacs-28-based tree-sitter support, but that's OK, it's better for the
build-system to be made to target emacs 29's builtin support. I'm sure I
can work around for emacs 28.

Thanks,
Pierre