diff mbox series

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

Message ID 87fsbdo34v.fsf@gmx.com
State New
Headers show
Series [bug#49946,v7,01/32] gnu: tree-sitter: Move to its own module. | expand

Commit Message

Pierre Langlois Feb. 10, 2023, 5:04 p.m. UTC 
Hi,

Andrew Tropin <andrew@trop.in> writes:

> [[PGP Signed Part:Undecided]]
> On 2023-02-09 13:39, zimoun wrote:
>
>> Hi,
>>
>> On Thu, 09 Feb 2023 at 14:11, Andrew Tropin <andrew@trop.in> 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:20221125012142.22579-3-pierre.langlois@gmx.com
>> http://issues.guix.gnu.org/msgid/20221125012142.22579-3-pierre.langlois@gmx.com
>>
>> [bug#49946] [PATCH v7 05/32] gnu: Add tree-sitter-cli.
>> id:20221125012142.22579-6-pierre.langlois@gmx.com
>> http://issues.guix.gnu.org/msgid/20221125012142.22579-6-pierre.langlois@gmx.com
>>
>> 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.

I had also started rebasing addressing comments on the tree-sitter-cli
package, and had a few extra changes left that haven't been pushed.

I realised we could remove a now redundant comment, and while enabling
*some* tests, I realized we should directly depend on node and dot, so
that people may use the `tree-sitter' command, without needing them in
the same profile.

What do you think of the following two patches?
Thanks,
Pierre

Comments

Andrew Tropin Feb. 12, 2023, 6:28 a.m. UTC | #1 
On 2023-02-10 17:04, Pierre Langlois wrote:

> Hi,
>
> Andrew Tropin <andrew@trop.in> writes:
>
>> [[PGP Signed Part:Undecided]]
>> On 2023-02-09 13:39, zimoun wrote:
>>
>>> Hi,
>>>
>>> On Thu, 09 Feb 2023 at 14:11, Andrew Tropin <andrew@trop.in> 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:20221125012142.22579-3-pierre.langlois@gmx.com
>>> http://issues.guix.gnu.org/msgid/20221125012142.22579-3-pierre.langlois@gmx.com
>>>
>>> [bug#49946] [PATCH v7 05/32] gnu: Add tree-sitter-cli.
>>> id:20221125012142.22579-6-pierre.langlois@gmx.com
>>> http://issues.guix.gnu.org/msgid/20221125012142.22579-6-pierre.langlois@gmx.com
>>>
>>> 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.
>
> I had also started rebasing addressing comments on the tree-sitter-cli
> package, and had a few extra changes left that haven't been pushed.
>
> I realised we could remove a now redundant comment, and while enabling
> *some* tests, I realized we should directly depend on node and dot, so
> that people may use the `tree-sitter' command, without needing them in
> the same profile.

Sounds very valid! :)

>
> What do you think of the following two patches?
>

I applied them locally and tested, LGTM.  Will push them a little later
with the rest of tree-sitter related changes.

> From b24b04015261cfea2143be63671877c9c2c8d458 Mon Sep 17 00:00:00 2001
> From: Pierre Langlois <pierre.langlois@gmx.com> Date: Fri, 10 Feb 2023
> 16:10:25 +0000 Subject: [PATCH 1/2] gnu: Remove tree-sitter comment
> from emacs.scm imports.
>
> * gnu/packages/emacs.scm: Remove tree-sitter comment.
> ---
>  gnu/packages/emacs.scm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
> index 4ce41deb88..4791570d12 100644
> --- a/gnu/packages/emacs.scm
> +++ b/gnu/packages/emacs.scm
> @@ -76,7 +76,7 @@ (define-module (gnu packages emacs)
>    #:use-module (gnu packages sqlite)
>    #:use-module (gnu packages texinfo)
>    #:use-module (gnu packages tls)
> -  #:use-module (gnu packages tree-sitter) ; for tree-sitter
> +  #:use-module (gnu packages tree-sitter)
>    #:use-module (gnu packages web)       ; for jansson
>    #:use-module (gnu packages webkit)
>    #:use-module (gnu packages xml)
> -- 
> 2.39.1
>
> From 8225d51edeb528c51d30ad0f225eb96be5586a37 Mon Sep 17 00:00:00 2001
> From: Pierre Langlois <pierre.langlois@gmx.com>
> Date: Tue, 29 Mar 2022 20:13:11 +0100
> Subject: [PATCH 2/2] gnu: tree-sitter-cli: Enable some tests and add node and
>  dot.
>
> * gnu/packages/tree-sitter.scm (tree-sitter-cli)[inputs]: Add graphviz and
> node-lts.
> [arguments]<#:cargo-test-flags>: Skip tests that require downloading grammars.
> <#:phases>: Add 'patch-node and 'patch-dot phases.  Tweak install phase.
> ---
>  gnu/packages/tree-sitter.scm | 49 ++++++++++++++++++++++++++++--------
>  1 file changed, 39 insertions(+), 10 deletions(-)
>
> diff --git a/gnu/packages/tree-sitter.scm b/gnu/packages/tree-sitter.scm
> index 2e44339ca2..4331ffdd97 100644
> --- a/gnu/packages/tree-sitter.scm
> +++ b/gnu/packages/tree-sitter.scm
> @@ -24,7 +24,9 @@ (define-module (gnu packages tree-sitter)
>    #:use-module ((guix licenses) #:prefix license:)
>    #:use-module (gnu packages crates-graphics)
>    #:use-module (gnu packages crates-io)
> +  #:use-module (gnu packages graphviz)
>    #:use-module (gnu packages icu4c)
> +  #:use-module (gnu packages node)
>    #:use-module (guix build-system cargo)
>    #:use-module (guix build-system gnu)
>    #:use-module (guix gexp)
> @@ -96,13 +98,30 @@ (define-public tree-sitter-cli
>                                println!(\"cargo:rustc-link-lib=tree-sitter\");~@
>                                }~%")))))))
>      (build-system cargo-build-system)
> -    (inputs (list tree-sitter))
> +    (inputs
> +     (list tree-sitter graphviz node-lts))
>      (arguments
>       (list
> -      ;; Running test requires downloading fixtures, see the
> -      ;; script/fetch-fixtures script, which fetches grammars.  Maybe it make
> -      ;; sence to run tests in the grammar's packages?
> -      #:tests? #f
> +      #:cargo-test-flags
> +      ''("--release" "--"
> +         ;; Skip tests which rely on downloading grammar fixtures.  It is
> +         ;; difficult to support such tests given upstream does not encode
> +         ;; which version of the grammars are expected.
> +         ;; Instead, we do run some tests for each grammar in the tree-sitter
> +         ;; build-system, by running `tree-sitter test'.  This isn't as
> +         ;; complete as running all tests from tree-sitter-cli, but it's a
> +         ;; good compromise compared to maintaining two different sets of
> +         ;; grammars (Guix packages vs test fixtures).
> +         "--skip=tests::corpus_test"
> +         "--skip=tests::highlight_test"
> +         "--skip=tests::node_test"
> +         "--skip=tests::parser_test"
> +         "--skip=tests::pathological_test"
> +         "--skip=tests::query_test"
> +         "--skip=tests::tags_test"
> +         "--skip=tests::test_highlight_test"
> +         "--skip=tests::test_tags_test"
> +         "--skip=tests::tree_test")
>        ;; We're only packaging the CLI program so we do not need to install
>        ;; sources.
>        #:install-source? #f
> @@ -132,11 +151,21 @@ (define-public tree-sitter-cli
>            (add-after 'unpack 'delete-cargo-lock
>              (lambda _
>                (delete-file "Cargo.lock")))
> -          (replace 'install
> -            (lambda* (#:key outputs #:allow-other-keys)
> -              (let ((bin (string-append #$output "/bin")))
> -                (mkdir-p bin)
> -                (install-file "target/release/tree-sitter" bin)))))))
> +          (add-after 'unpack 'patch-node
> +            (lambda _
> +              (substitute* "cli/src/generate/mod.rs"
> +                (("Command::new\\(\"node\"\\)")
> +                 (string-append "Command::new(\"" #$node-lts "/bin/node\")")))))
> +          (add-after 'unpack 'patch-dot
> +            (lambda _
> +              (substitute* "cli/src/util.rs"
> +                (("Command::new\\(\"dot\"\\)")
> +                 (string-append "Command::new(\"" #$graphviz "/bin/dot\")")))))
> +           (replace 'install
> +             (lambda _
> +               (let ((bin (string-append #$output "/bin")))
> +                 (mkdir-p bin)
> +                 (install-file "target/release/tree-sitter" bin)))))))
>      (description "Tree-sitter is a parser generator tool and an incremental
>  parsing library.  It can build a concrete syntax tree for a source file and
>  efficiently update the syntax tree as the source file is edited.
Pierre Langlois Feb. 12, 2023, 12:29 p.m. UTC | #2 
Andrew Tropin <andrew@trop.in> writes:

> [[PGP Signed Part:Undecided]]
> On 2023-02-10 17:04, Pierre Langlois wrote:
>
>> Hi,
>>
>> Andrew Tropin <andrew@trop.in> writes:
>>
>>> [[PGP Signed Part:Undecided]]
>>> On 2023-02-09 13:39, zimoun wrote:
>>>
>>>> Hi,
>>>>
>>>> On Thu, 09 Feb 2023 at 14:11, Andrew Tropin <andrew@trop.in> 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:20221125012142.22579-3-pierre.langlois@gmx.com
>>>> http://issues.guix.gnu.org/msgid/20221125012142.22579-3-pierre.langlois@gmx.com
>>>>
>>>> [bug#49946] [PATCH v7 05/32] gnu: Add tree-sitter-cli.
>>>> id:20221125012142.22579-6-pierre.langlois@gmx.com
>>>> http://issues.guix.gnu.org/msgid/20221125012142.22579-6-pierre.langlois@gmx.com
>>>>
>>>> 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.
>>
>> I had also started rebasing addressing comments on the tree-sitter-cli
>> package, and had a few extra changes left that haven't been pushed.
>>
>> I realised we could remove a now redundant comment, and while enabling
>> *some* tests, I realized we should directly depend on node and dot, so
>> that people may use the `tree-sitter' command, without needing them in
>> the same profile.
>
> Sounds very valid! :)
>
>>
>> What do you think of the following two patches?
>>
>
> I applied them locally and tested, LGTM.  Will push them a little later
> with the rest of tree-sitter related changes.

Cool, thanks!
diff mbox series

Patch

From 8225d51edeb528c51d30ad0f225eb96be5586a37 Mon Sep 17 00:00:00 2001
From: Pierre Langlois <pierre.langlois@gmx.com>
Date: Tue, 29 Mar 2022 20:13:11 +0100
Subject: [PATCH 2/2] gnu: tree-sitter-cli: Enable some tests and add node and
 dot.

* gnu/packages/tree-sitter.scm (tree-sitter-cli)[inputs]: Add graphviz and
node-lts.
[arguments]<#:cargo-test-flags>: Skip tests that require downloading grammars.
<#:phases>: Add 'patch-node and 'patch-dot phases.  Tweak install phase.
---
 gnu/packages/tree-sitter.scm | 49 ++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/gnu/packages/tree-sitter.scm b/gnu/packages/tree-sitter.scm
index 2e44339ca2..4331ffdd97 100644
--- a/gnu/packages/tree-sitter.scm
+++ b/gnu/packages/tree-sitter.scm
@@ -24,7 +24,9 @@  (define-module (gnu packages tree-sitter)
   #:use-module ((guix licenses) #:prefix license:)
   #:use-module (gnu packages crates-graphics)
   #:use-module (gnu packages crates-io)
+  #:use-module (gnu packages graphviz)
   #:use-module (gnu packages icu4c)
+  #:use-module (gnu packages node)
   #:use-module (guix build-system cargo)
   #:use-module (guix build-system gnu)
   #:use-module (guix gexp)
@@ -96,13 +98,30 @@  (define-public tree-sitter-cli
                               println!(\"cargo:rustc-link-lib=tree-sitter\");~@
                               }~%")))))))
     (build-system cargo-build-system)
-    (inputs (list tree-sitter))
+    (inputs
+     (list tree-sitter graphviz node-lts))
     (arguments
      (list
-      ;; Running test requires downloading fixtures, see the
-      ;; script/fetch-fixtures script, which fetches grammars.  Maybe it make
-      ;; sence to run tests in the grammar's packages?
-      #:tests? #f
+      #:cargo-test-flags
+      ''("--release" "--"
+         ;; Skip tests which rely on downloading grammar fixtures.  It is
+         ;; difficult to support such tests given upstream does not encode
+         ;; which version of the grammars are expected.
+         ;; Instead, we do run some tests for each grammar in the tree-sitter
+         ;; build-system, by running `tree-sitter test'.  This isn't as
+         ;; complete as running all tests from tree-sitter-cli, but it's a
+         ;; good compromise compared to maintaining two different sets of
+         ;; grammars (Guix packages vs test fixtures).
+         "--skip=tests::corpus_test"
+         "--skip=tests::highlight_test"
+         "--skip=tests::node_test"
+         "--skip=tests::parser_test"
+         "--skip=tests::pathological_test"
+         "--skip=tests::query_test"
+         "--skip=tests::tags_test"
+         "--skip=tests::test_highlight_test"
+         "--skip=tests::test_tags_test"
+         "--skip=tests::tree_test")
       ;; We're only packaging the CLI program so we do not need to install
       ;; sources.
       #:install-source? #f
@@ -132,11 +151,21 @@  (define-public tree-sitter-cli
           (add-after 'unpack 'delete-cargo-lock
             (lambda _
               (delete-file "Cargo.lock")))
-          (replace 'install
-            (lambda* (#:key outputs #:allow-other-keys)
-              (let ((bin (string-append #$output "/bin")))
-                (mkdir-p bin)
-                (install-file "target/release/tree-sitter" bin)))))))
+          (add-after 'unpack 'patch-node
+            (lambda _
+              (substitute* "cli/src/generate/mod.rs"
+                (("Command::new\\(\"node\"\\)")
+                 (string-append "Command::new(\"" #$node-lts "/bin/node\")")))))
+          (add-after 'unpack 'patch-dot
+            (lambda _
+              (substitute* "cli/src/util.rs"
+                (("Command::new\\(\"dot\"\\)")
+                 (string-append "Command::new(\"" #$graphviz "/bin/dot\")")))))
+           (replace 'install
+             (lambda _
+               (let ((bin (string-append #$output "/bin")))
+                 (mkdir-p bin)
+                 (install-file "target/release/tree-sitter" bin)))))))
     (description "Tree-sitter is a parser generator tool and an incremental
 parsing library.  It can build a concrete syntax tree for a source file and
 efficiently update the syntax tree as the source file is edited.
-- 
2.39.1