Skip to content
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

libs: vendor libmpack and libmpack-lua #15566

Merged
merged 6 commits into from
Sep 9, 2021
Merged

libs: vendor libmpack and libmpack-lua #15566

merged 6 commits into from
Sep 9, 2021

Conversation

bfredl
Copy link
Member

@bfredl bfredl commented Sep 4, 2021

  • we want to use libmpack to implement msgpack decoding with much less memory overhead (decode directly to API or internal types, not to msgpack-c allocated tree of heap objects)
  • expose vim.mpack for plugins Lua: expose mpack #15452

Alternative: bundle these as separate cmake dependencies. This could be done but then someone must write the necessary cmake logic, and my suspicion is that these libs aren't used much outside neovim anyway (msgpack-c is the "official" c bindings).

@github-actions github-actions bot added build building and installing Neovim using the provided scripts lua stdlib labels Sep 4, 2021
src/nvim/CMakeLists.txt Outdated Show resolved Hide resolved
src/nvim/mpack/LICENSE-MIT Outdated Show resolved Hide resolved
@bfredl bfredl force-pushed the mpack branch 3 times, most recently from 5cc8d38 to 2d73054 Compare September 9, 2021 09:04
@bfredl bfredl marked this pull request as ready for review September 9, 2021 12:46
@bfredl
Copy link
Member Author

bfredl commented Sep 9, 2021

This should be pretty much done as a MVP (except an OS X build failure I'm currently investigating fixed). For now only one-shot pack/unpack methods are documented and tested, but support of Sessions will be a natural followup when implementing RPC for lua worker threads and/or remote processes (which are gonna want a wrapper around mpack.Session + vim.loop) .

@bfredl bfredl force-pushed the mpack branch 2 times, most recently from 6881c0a to ad8eda3 Compare September 9, 2021 14:08
The *vim.mpack* module provides packing and unpacking of lua objects to
msgpack encoded strings. |vim.NIL| and |vim.empty_dict()| are supported.

vim.mpack.pack({obj}) *vim.mpack.pack*
Copy link
Member

@justinmk justinmk Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the "pack"/"unpack" a distinct concept from "encode"/"decode"? We have already msgpackparse/msgpackdump (Python convention) and json_ecode/json_decode.

If the distinction is not important it might make sense to call this vim.mpack.encode/decode. Or perhaps from_lua/to_lua ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's not, I suspect msgpack libs always use "pack" and "unpack" as it rhymes with "msgpack". I don't think from_lua and to_lua makes sense as (1) lua is implied by the context of lua code (2) later we need the nouns such as the Packer and the Unpacker, where the Encoder/Decoder works if there's a strong preference to change it to that, though my honest opinion would be to err at the side of lazyness and keep the msgpack conventions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where the Encoder/Decoder works if there's a strong preference to change it to that, though my honest opinion would be to err at the side of lazyness and keep the msgpack conventions.

if you just want to avoid the chore, I am willing to do it :) Consistency (in the scope of our system) helps users a lot.

@bfredl
Copy link
Member Author

bfredl commented Sep 9, 2021

is it needed to do this now? marcos.h seems to encode nvim-specific desicions what c compilers we support, and it would make more sense to figure this out when extracting/upstreaming code later (if ever).

@bfredl bfredl merged commit d80aac3 into neovim:master Sep 9, 2021
casr added a commit to casr/macports-ports that referenced this pull request Oct 3, 2024
`neovim`:

- removed `lua51-mpack` as a dependency since it's [no longer
  required][0]
- removed `libtermkey` as a dependency since it's [no longer
  required][1]

[0]: neovim/neovim#15566
[1]: neovim/neovim#25870

`neovim-devel`:

- removed `msgpack` as a dependency since it's [no longer required][2]
- removed `libvterm` as a dependency since it's [no longer required][3]

[2]: neovim/neovim#29540
[3]: neovim/neovim#30011

Also note that `neovim-devel` is not progressing passed 2024-08-31 until
`utf8proc` sees its next release. Versions after this date will fail
with a compile error due to a type mismatch.

Fixes: https://trac.macports.org/ticket/70028
casr added a commit to casr/macports-ports that referenced this pull request Oct 3, 2024
`neovim`:

- removed `lua51-mpack` as a dependency since it's [no longer
  required][0]
- removed `libtermkey` as a dependency since it's [no longer
  required][1]

[0]: neovim/neovim#15566
[1]: neovim/neovim#25870

`neovim-devel`:

- removed `msgpack` as a dependency since it's [no longer required][2]
- removed `libvterm` as a dependency since it's [no longer required][3]

[2]: neovim/neovim#29540
[3]: neovim/neovim#30011

Also note that `neovim-devel` is not progressing past 2024-08-31 until
`utf8proc` sees its next release. Versions after this date will fail
with a compile error due to a type mismatch.

Fixes: https://trac.macports.org/ticket/70028
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build building and installing Neovim using the provided scripts lua stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants