-
Notifications
You must be signed in to change notification settings - Fork 20
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
Update tree-sitter-cli to 0.20.6 #26
Conversation
The details are escaping me at the moment, but I think there was something on the tree-sitter end I'd been waiting for before going through with an upgrade to the 0.20.x series. I'll see if I can dig up what that was. |
I've not turned up what was holding things up yet. Regarding the PR though, there are a few things I would do differently.
I appreciate the effort, but If an update to |
Sure, do it however you want. Moving to v2 of package-lock.json seems like a good thing tho, seeing as npm's v6 isn't the default in current or maintained node releases and it's backwards compatible. What's the goal of keeping it at v1? |
Re: version 1 - I think when I started working on tree-sitter-clojure and other things, the tooling that got set up was oriented around version 1 lock files -- node 12.x + npm 6.y. That's still what I'm currently using (and I have other projects that use the same tooling). Perhaps I should switch to 14.x, but I haven't investigated potential consequences yet. IIUC, according to: https://nodejs.org/en/about/releases/ Node.js v14 is in maintenance (and will be until next year?). When I install that with nvm, it looks like the corresponding npm is v6. I don't know if the following is correct, but looking at: https://www.abrahamberg.com/blog/npm-package-json-lock-version-1-or-2/ node v6 uses a version 1 lock file. May be some of the above is wrong? |
Oops, maybe I'm mistaken then. My apologies. I thought node 14 came with npm v6, but it looks like you are right. Haha I gotta double check things before I say shit I don't actually know. |
Thanks for bringing the point up in any case -- I wasn't aware of the finer details. Just to be clear, I think node 12 comes with npm 6.x and node 14 comes with npm 6.y -- at least when I install with nvm that's what I see mentioned. It looks like this file: https://nodejs.org/dist/index.json has these sorts of details. According to it:
and:
which in short is:
|
Maybe it was to do with this?: emacs-tree-sitter/tree-sitter-langs@3d30229 |
Thanks for the links and refernce, @SignSpice! Good to see a potential reason. Bummer that the emacs folks are stuck with an old version (for the time being). From a brief overview of some available tree-sitter implementations (here's a pretty big list), it seems that there's no consensus on specific versions, that each implementation uses whichever version they feel is best. |
@SignSpice Ah, I remember what you pointed out but I don't remember if that was a factor in holding back :) Thanks for the links in any case. |
I haven't seen any recent activity regarding the issue that is holding up elisp-tree-sitter from moving to the 0.20.x series. IIUC, Emacs 29 may be built with tree-sitter support, so possibly before long, use of elisp-tree-sitter will be less prevalent. I've used some commits from the emacs-29 branch as well as the master branch recently and they've been working more or less ok for me. I think @dannyfreeman mentioned the idea of having a branch of tree-sitter-clojure that still uses 0.19.x (for those who want to use elisp-tree-sitter) and switching the master branch over to the 0.20.y series. |
I think maintaining two branches for the different versions wouldn't be all that bad (I would be happy to do that work). It might be worth upgrading after #31 is merged. I'll also be that this issue #32 get fixed in tree-sitter itself and will require the use of a newer version of tree-sitter. If we maintain two branches, I think we would need to adopt a new practice regarding commits in this repository. Since the generated files will generate a lot of merge conflicts that will be difficult to resolve between the two branches, going forward it would probably be best to commit changes to the grammar.js file in it's own commit, and changes to the generated files in a separate commit. This would allow cherry picking grammar changes from master to the 19.x branch, then generating in a separate commit on the 19.x branch without worrying about big conflicts. |
Thanks for your thoughts (and offer!)
Yes, ATM it does seem like a good idea to wait at least for #31 to be merged. It would be nice if #32 got fixed, but hard to say if / when it will happen. I mentioned this in #32, but it seems possible for Emacs' use case that it could be worked around for the time being as Emacs' If we do go with the 2-branch approach, the idea of separating commits for On the note of generated files, there is the following bit at the main tree-sitter repository:
That's from the Tree-sitter 1.0 Checklist issue though so perhaps it won't be any time soon... |
When I first started working on this I wondered why we did this and thought it would be better to remove the generated files from source control. Now after seeing this very large thread in emacs mailing list: https://lists.gnu.org/archive/html/emacs-devel/2022-12/msg00692.html |
Thanks for your comments and that link.
I can see that. There is also this though:
via: https://lists.gnu.org/archive/html/emacs-devel/2022-12/msg01180.html I would add that there's also the issue of an unintentional mismatch. FWIW, I think there has been some discusson at the tree-sitter repository regarding the use of node.js:
On a side note, there was this issue where there was some discussion of using something other than |
Related to:
I came across the following 2 posts at the emacs-devel mailing list archives:
The first has a nice overview diagram of a pipeline for generating the C / C++ starting with
The second has a JavaScript program that can be used with quickjs to produce
To produce
Then invoke The result can be compared with what is typically generated via the "ordinary" method:
Not all that different. |
Looks like quickjs might be usable at least for Linux, macos, and Windows. Binary releases appear to be available for Linux and Windows from the author of quickjs: https://bellard.org/quickjs/binary_releases/ (On a side note, I didn't have luck building on Windows yet.) There appears to be something for brew for macos: https://formulae.brew.sh/formula/quickjs Not using Node.js would be nice [1], but I'm not sure yet whether it's worth transitioning to some other approach yet. [1] Getting away from the seemingly needless churn and vulnerabilities would be welcome (the docs might get simpler too...). |
It looks like this line is currently where I think dannyfreeman pointed out and maxbrunsfeld had earlier confirmed that while nodejs is (currently) necessary for producing On a related note, maxbrunsfeld also mentioned in another discussion that the version of nodejs does not matter. I don't know if that will continue to remain true as I'm not sure what nodejs' future will be like. maxbrunsfeld also mentioned a few specific constructs that are in nodejs that are relied on -- maxbrunsfeld also appeared quite open to the idea of having changes in tree-sitter to allow alternative javascript runtimes. AFAIK, it hasn't been done yet though. There is something related(?) mentioned in the Tree-sitter 1.0 Checklist:
Though I'm not quite sure what that means. One of the other tree-sitter maintainers, ahlinc, also mentioned:
|
I tried replicating what the tree-sitter-cli does to generate Assuming
The following is roughly what these lines in tree-sitter-cli's source do:
Looks like an extra newline is thrown in at the end too. |
In 3daa97f (on the dev branch), As mentioned in #45, I intend to make it clear what version of the |
I've labeled this PR with the Once we transfer the appropriate changes from the dev branch having to do with expressing our use of tree-sitter 0.20.7 (or possibly a specific later commit) for generating the content within Note though that addressing #17 (which I believe was the point of the original PR) may be an ongoing issue as one of the main causes is the evolution of Emscripten -- which is still continuing. For getting the I've written up more on this topic here. |
ATM, there is an invocation in another repository in these lines. |
@NoahTheDuke Thanks for opening this PR and the subsequent discussion. I think it was a significant factor in us arriving at an overall improvement 👍 Now that v0.0.12 (which includes some changes [1] that used tree-sitter v0.20.7 for generating parser source) has been released, may be it's ok to close this PR? [1] Among others, this one: 12fcfb9 |
Ok, I'm going to take the reactions as an ok to close :) Thanks again! |
Closes #17.