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

[RFC] Full compliance with msgpack-RPC #1121

Merged
merged 3 commits into from Aug 30, 2014

Conversation

Projects
None yet
8 participants
@tarruda
Copy link
Member

tarruda commented Aug 28, 2014

Should close #1118

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 28, 2014

Maybe @philix would like to review this :)

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 29, 2014

As I said in #1118, ideally we would have a static hash table generated at build time, which would have these advantages:

  • Avoid the need for initialization code.
  • Use a custom hash function that does not depend on 0-terminated strings while also avoiding the need to copy the msgpack buffer to a temporary 0-terminated buffer

Once the static hash table generation is in place we should probably remove method ids and use only strings to have a single way of calling RPC functions(I did not remove method ids yet to avoid breaking compatibility with current client implementations)

In any case this should be enough to enable client library development against the msgpack-rpc specification

@equalsraf

This comment has been minimized.

Copy link
Contributor

equalsraf commented Aug 29, 2014

I've started having a look at this for neovim-qt, everything is working so far, I've replaced ids for function names and my example programs are working as intended :D

Quick Question: I've noticed there is no method name for function id [0] - is it going to be removed? I like having the ability to check compatibility at runtime.

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 29, 2014

Quick Question: I've noticed there is no method name for function id [0] - is it going to be removed? I like having the ability to check compatibility at runtime.

The metadata call will not be removed because it can have extra information that is useful for API-wrapper generators(classes/properties hints for example), though it will probably change into a real method name, eg: vim_get_api_metadata

Once this PR is merged and we support msgpack 2.0, I think we should add semantic versioning information to the metadata where:

  • Patch will be increased whenever API bugs are fixed without changing the function signatures
  • Minor will be increased whenever functions are added
  • Major will be increased whenever function signatures are modified

It will be an useful mechanism that client libraries can use to check for compatibility(especially for static languages that need to generate at build time but may be used against newer versions of nvim)

@splinterofchaos

This comment has been minimized.

Copy link
Member

splinterofchaos commented Aug 29, 2014

[metadata] will be an useful mechanism that client libraries can use to check for compatibility(especially for static languages that need to generate at build time but may be used against newer versions of nvim)

Very much, so. I actually wrote a shell that prints this information and lets you type msgpack commands to send to vim. Very useful for both debugging and understanding how it works. It would be sorely missed if removed.

https://github.com/splinterofchaos/neovim-cpp-client-experiment/blob/master/src/vim-shell.cpp

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 29, 2014

@splinterofchaos That's great, I've been wanting something like that. We should link to that from the API page.

@philix

This comment has been minimized.

Copy link
Member

philix commented Aug 29, 2014

Maybe @philix would like to review this :)

@justinmk Sure. :) String method ids are the best thing since bread and
butter.
Unfortunately I'm on a bus right now with poor mobile network signal.

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 30, 2014

Just tested with ruby msgpack-rpc library and it seems to be working, going to merge now

@tarruda tarruda force-pushed the tarruda:msgpack-rpc-compliance branch from 15a3106 to 2a7fa0f Aug 30, 2014

tarruda added some commits Aug 28, 2014

msgpack-rpc: Always use arrays when sending events or calls
This is required by the msgpack-RPC specification. Also, the
send_call/send_event functions were refactored to accept a variable number of
arguments
Fix environment variable for triggering embedded tests
NVIM should be used for all technical identifiers and this was changed in
the python-client

@tarruda tarruda force-pushed the tarruda:msgpack-rpc-compliance branch from 2a7fa0f to 887446a Aug 30, 2014

@philix

This comment has been minimized.

Copy link
Member

philix commented Aug 30, 2014

Just tested with ruby msgpack-rpc library and it seems to be working, going to merge now

Go ahead.

@tarruda tarruda merged commit 887446a into neovim:master Aug 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

tarruda added a commit that referenced this pull request Aug 30, 2014

@myitcv

This comment has been minimized.

Copy link
Member

myitcv commented Aug 30, 2014

Apologies, I'll get to looking at the Go library on Monday. Sounds like
everything should be fine however
On 30 Aug 2014 02:28, "Thiago de Arruda" notifications@github.com wrote:

Merged #1121 #1121.


Reply to this email directly or view it on GitHub
#1121 (comment).

Object result = provider_call(method, vim_to_object(argvars));
Array args = ARRAY_DICT_INIT;
ADD(args, vim_to_object(argvars));
Object result = provider_call(method, args);

if (result.type == kObjectTypeNil) {
return;

This comment has been minimized.

@oni-link

oni-link Aug 30, 2014

Contributor

Does rettv need to be set for an "early" error?

@myitcv

This comment has been minimized.

Copy link
Member

myitcv commented Sep 2, 2014

The Go neovim package has been updated (I used this document as a reference) in line with these changes.

Indeed I've also updated it to use --embedded-mode for tests per your suggestion @tarruda - all working well, thanks for the very clear instructions. The git repo is now also linked to Travis, pulling down master of Neovim and rebuilding each time (uses .ci/common.sh from Neovim)

Once this PR is merged and we support msgpack 2.0, I think we should add semantic versioning information to the metadata where:

  • Patch will be increased whenever API bugs are fixed without changing the function signatures
  • Minor will be increased whenever functions are added
  • Major will be increased whenever function signatures are modified

It will be an useful mechanism that client libraries can use to check for compatibility(especially for static languages that need to generate at build time but may be used against newer versions of nvim)

👍

In the meantime, I am soon going to commit a failsafe that breaks the Go neovim package build when it detects the API has changed.

@tarruda - one thing I noticed whilst migrating from method IDs to method names: as of 3051015 at least, method IDs can be different between builds on different machines. This was causing my tests to fail because they were in effect calling the wrong method: my test thought it was calling 40 = vim_get_buffers whereas the Neovim on the Travis box was exposing 40 = some_other_method. Not a big issue because, as I said, the Go neovim package is now using string IDs.

@tarruda tarruda referenced this pull request Sep 7, 2014

Merged

September Newsletter #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment