Open
Conversation
nekowinston
changed the title
github-actions
bot
added
10.rebuild-darwin: 501+
nix-owners
bot
requested review from
Profpatsch,
ereslibre,
matthewbauer and
uncenter
Member
Author
|
Member
|
I could be wrong but I think a PR like this for Tree-sitter should target staging because of the rebuilds? |
Member
Author
I'm not sure; I guess another way of handling it would be turning off the wasm feature by default for now? Since wasmtime + tree-sitter + neovim takes quite a while to build, I'd appreciate it if binary caches defaulted to support for neovim at least, should support for Wasm artifacts find wider adoption. |
wegank
added
the
2.status: merge conflict
nekowinston
force-pushed
the
tree-sitter-add-wasm-feature
branch
from
5e73d51 to
84beb7c
Compare
ofborg
bot
removed
the
2.status: merge conflict
nekowinston
force-pushed
the
tree-sitter-add-wasm-feature
branch
from
84beb7c to
594842e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
wegank
added
the
2.status: merge conflict
nekowinston
force-pushed
the
tree-sitter-add-wasm-feature
branch
from
594842e to
06e4c95
Compare
ofborg
bot
removed
the
2.status: merge conflict
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Member
|
nekowinston
force-pushed
the
tree-sitter-add-wasm-feature
branch
from
06e4c95 to
17e0a93
Compare
nixpkgs-ci
bot
added
the
2.status: merge conflict
nekowinston
force-pushed
the
tree-sitter-add-wasm-feature
branch
from
a11de3c to
4202301
Compare
nixpkgs-ci
bot
removed
the
2.status: merge conflict
13 tasks
nekowinston
force-pushed
the
tree-sitter-add-wasm-feature
branch
from
4202301 to
043a919
Compare
Member
|
Please say when this PR is ready for another round of nixpkgs-review |
Member
Author
|
I'm now sadly expecting stuff to break as soon as wasmtime 36.0.0 is merged1, so figuring out if we should keep an additional pinned wasmtime@29.0.1 that follows neovim/tree-sitter CMakeLists comes first, I guess. When I discussed this on Matrix before, it was suggested to lock wasmtime to whatever neovim uses, but I feel that would hold users of the CLI back quite a bit. Maybe I should flag the PR as WIP since there isn't really a reason to build neovim with wasmtime support as of now. Footnotes |
13 tasks
nekowinston
force-pushed
the
tree-sitter-add-wasm-feature
branch
from
043a919 to
a0f5672
Compare
Member
Author
|
After working on Recent updates to wasmtime broke the C API, so I had to add a pinned version tracking v29 for tree-sitter/neovim. Please let me know if I've done this the wrong way. |
nekowinston
force-pushed
the
tree-sitter-add-wasm-feature
branch
from
a0f5672 to
69a3c70
Compare
nekowinston
force-pushed
the
tree-sitter-add-wasm-feature
branch
from
69a3c70 to
722853f
Compare
nixpkgs-ci
bot
added
the
2.status: merge conflict
nekowinston
force-pushed
the
tree-sitter-add-wasm-feature
branch
from
722853f to
3b441b8
Compare
nixpkgs-ci
bot
removed
the
2.status: merge conflict
Member
|
the treesitter feature could be submitted separately to treesitter maintainers. Unless upstream enables wasm by default, there is no reason for us to do so. I wouldn't mind enabling wasm by default on https://github.com/nix-community/neovim-nightly-overlay/ though (if other maintainers agree). |
nixpkgs-ci
bot
added
the
2.status: merge conflict
nekowinston
force-pushed
the
tree-sitter-add-wasm-feature
branch
from
3b441b8 to
19dd6d9
Compare
nixpkgs-ci
bot
removed
the
2.status: merge conflict
Member
Author
From my PR description:
Speaking purely in terms of closure sizes, that'd increase the |
nixpkgs-ci
bot
added
the
2.status: merge conflict
wasmtime introduced breaking changes to the C API sometime between v33 and v36. Since both neovim and tree-sitter track the same version of wasmtime (v29.0.1) this adds a package tracking that major version. Loosening the `EXACT` specifier in neovim's CMakeLists worked until v33.
nekowinston
force-pushed
the
tree-sitter-add-wasm-feature
branch
from
19dd6d9 to
118804a
Compare
nixpkgs-ci
bot
removed
the
2.status: merge conflict
nixpkgs-ci
bot
added
the
2.status: merge conflict
teto
added
the
6.topic: tree-sitter
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enables the
wasmfeature.Motivation for this is nvim-treesitter/nvim-treesitter#4767.
Upstream requires an EXACT version of
v29.0.1of wasmtime, but patching CMakeLists allows using versions up to v33. Since the latest version in nixpkgs at time of writing is v36, and that version introduced breaking changes in the C API, I've addedwasmtime_29to track both tree-sitter's and neovim's pinned major version.I've also enabled the CMake option
ENABLE_WASMTIMEforneovimin this PR, mainly for easy testing of the functionality. I'd like for this option to be a default, as we've been able to reduce the added/closure size forwasmtime.deva lot in recent commits (~20MB added size).Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.