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

Compiling under Windows (VS2015) #1749

Closed
justinmk opened this Issue Dec 28, 2014 · 104 comments

Comments

Projects
None yet
@justinmk
Member

justinmk commented Dec 28, 2014

Neovim on AppVeyor

Comprehensive notes on building for Windows

  • Attempt to build neovim using cmake and VS2013
  • Build & install libuv somewhere that cmake can find it
  • start thinking about how to split platform code (os_unix.c os_msvc.c etc)
  • Start splitting compiler options in CMake (#810)
  • fix listening server, Windows pipe names must always start be in the form "\\\\.\\pipe\\pipename" (d52fb89 )
  • gendeclarations.lua patch. #877
  • Port if_cscope to use libuv (#810 (comment)) #3927
  • we need a os_file_is_readable() in os/fs.c to fix eval.c:f_file_readable() #3166
  • check if Neovim can work on windows without libintl (find HAVE_WORKING_LIBINTL, implement ex_cmds2.c:get_locale_val()?). It builds but the old code was removed, and needs testing.
  • Add include guards: sys/time.h unistd.h
  • Reorder includes here and there, i.e. ensure platform includes and/or vim.h are included before anything else
  • bring back utf16_to_enc()/enc_to_utf16() - needed to convert string encoding when calling windows functions #4837
  • declare VS 2015 as minimum MSVC (comment)
  • busted tests: lubyk/xml#4 should work for MSVC but not for Mingw
  • lua nvim-client does not build on Windows.
  • Refactor vim_FulllName. NOTE: #2471 (comment)
  • Get ffi working with MSVC - specially how to handle __pragma (see d530fb6 for more details)
  • os_setperm() needs to be re-thought; it is nonsense on Windows because of uv_fs_chmod.

From #1749 (comment) :

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Dec 28, 2014

Contributor

@justinmk we can scratch a couple

Add bracket to if statements (maybe?)

Not needed anymore, this was a compiler bug.

Start splitting compiler options in CMake

I meant it more in terms of guarding gcc/clang specific flags with if (COMPILER_ID ...). One way or another #810 should have most of what we need. It might be worth it (or not) considering moving compiler specific option out of the main CMakeLists.txt.

Contributor

equalsraf commented Dec 28, 2014

@justinmk we can scratch a couple

Add bracket to if statements (maybe?)

Not needed anymore, this was a compiler bug.

Start splitting compiler options in CMake

I meant it more in terms of guarding gcc/clang specific flags with if (COMPILER_ID ...). One way or another #810 should have most of what we need. It might be worth it (or not) considering moving compiler specific option out of the main CMakeLists.txt.

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Jan 25, 2015

Contributor

Commit 31c8440 uses SSIZE_T which is POSIX, but does not exist in Windows, looking for suitable replacements (using INT_MAX for now).

Contributor

equalsraf commented Jan 25, 2015

Commit 31c8440 uses SSIZE_T which is POSIX, but does not exist in Windows, looking for suitable replacements (using INT_MAX for now).

@elmart

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Jan 25, 2015

Member

intmax_t is ok, yes. ptrdiff_t could be another alternative.
But, to me, the ideal alternative is neither of those.
I don't like C's convention of mixing two concerns (proper result and error indication) in the same variable.
I'd prefer using two variables (one size_t variable for the proper result, and one bool variable for error indication.
It seems ssize_t is only used by find_term_bykeys, which is used only once. So, refactoring that function to get rid of ssize_t altogether seems to me like the best alternative. You'd have several alternatives for the new function signature:

  • size_t find_term_bykeys(char_u *src, bool *error);
  • bool find_term_bykeys(char_u *src, size_t *index);
  • compound_result_T find_term_bykeys(char_u *src);, where compound_result_T (named some other way) would be a typedef for struct {size_t index; bool error;}.
    This is something I'd do for the entire codebase, little by little. The only drawback is having to define a per-function return type. But if people agrees on doing this, that can be much alleviated by defining macros to generate those types on the fly.
Member

elmart commented Jan 25, 2015

intmax_t is ok, yes. ptrdiff_t could be another alternative.
But, to me, the ideal alternative is neither of those.
I don't like C's convention of mixing two concerns (proper result and error indication) in the same variable.
I'd prefer using two variables (one size_t variable for the proper result, and one bool variable for error indication.
It seems ssize_t is only used by find_term_bykeys, which is used only once. So, refactoring that function to get rid of ssize_t altogether seems to me like the best alternative. You'd have several alternatives for the new function signature:

  • size_t find_term_bykeys(char_u *src, bool *error);
  • bool find_term_bykeys(char_u *src, size_t *index);
  • compound_result_T find_term_bykeys(char_u *src);, where compound_result_T (named some other way) would be a typedef for struct {size_t index; bool error;}.
    This is something I'd do for the entire codebase, little by little. The only drawback is having to define a per-function return type. But if people agrees on doing this, that can be much alleviated by defining macros to generate those types on the fly.

equalsraf added a commit to equalsraf/neovim that referenced this issue Jan 25, 2015

find_term_bykeys to return bool instead of ssize_t
- Return value is true if found, false otherwise
- index value (size_t) is set using ptr argument
- from neovim#1749 - to avoid the use of ssize_t

equalsraf added a commit to equalsraf/neovim that referenced this issue Jan 29, 2015

find_term_bykeys to return bool instead of ssize_t
- Return value is true if found, false otherwise
- index value (size_t) is set using ptr argument
- from neovim#1749 - to avoid the use of ssize_t
@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Jan 31, 2015

Contributor

FYI, (somehow 😮) my branch is now building luarocks from third-party - meaning it builds Neovim with just:

  • git, python2 (required by libuv)
  • CMake
  • Visual studio 2013 update 4

There is an automated build on Appveyor

Contributor

equalsraf commented Jan 31, 2015

FYI, (somehow 😮) my branch is now building luarocks from third-party - meaning it builds Neovim with just:

  • git, python2 (required by libuv)
  • CMake
  • Visual studio 2013 update 4

There is an automated build on Appveyor

@sethjackson

This comment has been minimized.

Show comment
Hide comment
@sethjackson

sethjackson Jan 31, 2015

Contributor

@equalsraf Windows defines SSIZE_T in basetsd.h.
See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751%28v=vs.85%29.aspx

Whenever I've needed ssize_t I've just typedef'd it as SSIZE_T.

Contributor

sethjackson commented Jan 31, 2015

@equalsraf Windows defines SSIZE_T in basetsd.h.
See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751%28v=vs.85%29.aspx

Whenever I've needed ssize_t I've just typedef'd it as SSIZE_T.

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Jan 31, 2015

Contributor

@sethjackson thanks. I was able to find ssize_t, what I couldnt find was SSIZE_MAX and ended up using INT_MAX instead. Maybe LONG_MAX might be the correct one depending on the architecture.

Contributor

equalsraf commented Jan 31, 2015

@sethjackson thanks. I was able to find ssize_t, what I couldnt find was SSIZE_MAX and ended up using INT_MAX instead. Maybe LONG_MAX might be the correct one depending on the architecture.

@fwalch

This comment has been minimized.

Show comment
Hide comment
@fwalch

fwalch Jan 31, 2015

Member

There is an automated build on Appveyor

Awesome! Maybe AppVeyor can already be enabled for neovim/neovim (cc @justinmk), and you can add the appveyor.yml to your Windows PRs. That way the Appveyor build status will be visible in your PRs along with the Travis status.

Member

fwalch commented Jan 31, 2015

There is an automated build on Appveyor

Awesome! Maybe AppVeyor can already be enabled for neovim/neovim (cc @justinmk), and you can add the appveyor.yml to your Windows PRs. That way the Appveyor build status will be visible in your PRs along with the Travis status.

@elmart

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Jan 31, 2015

Member

couldnt find SSIZE_MAX and ended up using INT_MAX instead

I think the way to go would be using preprocessor to define SSIZE_MAX where not present. It shouldn't be difficult, at first glance.

Member

elmart commented Jan 31, 2015

couldnt find SSIZE_MAX and ended up using INT_MAX instead

I think the way to go would be using preprocessor to define SSIZE_MAX where not present. It shouldn't be difficult, at first glance.

@sethjackson

This comment has been minimized.

Show comment
Hide comment
@sethjackson

sethjackson Jan 31, 2015

Contributor

I agree with @elmart. I think defining SSIZE_MAX would probably be the right option.

SSIZE_T is a typedef for LONG_PTR which is a long on 32-bit archs and __int64 on 64-bit archs.

This should work:

#if defined(_WIN64)
    #define SSIZE_MAX _I64_MAX
#else
    #define SSIZE_MAX LONG_MAX
#endif

wrapped in a Windows-specific header of course.

Contributor

sethjackson commented Jan 31, 2015

I agree with @elmart. I think defining SSIZE_MAX would probably be the right option.

SSIZE_T is a typedef for LONG_PTR which is a long on 32-bit archs and __int64 on 64-bit archs.

This should work:

#if defined(_WIN64)
    #define SSIZE_MAX _I64_MAX
#else
    #define SSIZE_MAX LONG_MAX
#endif

wrapped in a Windows-specific header of course.

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Jan 31, 2015

Contributor

Here is a badge badge

Configured AppVeyor to package the contents of the build folder here. 40mb is a bit, but it should include most things needed to debug. Linkage is static so it shouldn't need anything to run. If anyone gets a chance to run the binary in a windows machine let me know.

Contributor

equalsraf commented Jan 31, 2015

Here is a badge badge

Configured AppVeyor to package the contents of the build folder here. 40mb is a bit, but it should include most things needed to debug. Linkage is static so it shouldn't need anything to run. If anyone gets a chance to run the binary in a windows machine let me know.

@ebraminio

This comment has been minimized.

Show comment
Hide comment
@ebraminio

ebraminio Jan 31, 2015

Yes, works.

ebraminio commented Jan 31, 2015

Yes, works.

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Jan 31, 2015

Contributor

@ebraminio 👍

Contributor

equalsraf commented Jan 31, 2015

@ebraminio 👍

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Feb 2, 2015

Contributor

@justinmk cannot find any references to this anymore, I think it was fixed (#846? and maybe others)

  • we need a os_file_is_readable() in os/fs.c to fix eval.c:f_file_readable()
Contributor

equalsraf commented Feb 2, 2015

@justinmk cannot find any references to this anymore, I think it was fixed (#846? and maybe others)

  • we need a os_file_is_readable() in os/fs.c to fix eval.c:f_file_readable()
@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Feb 2, 2015

Contributor

And now that the visual studio generator is working we can build using clang-cl (not for third-party deps though) just `cmake -G "Visual Studio 12" -T "LLVM-vs2013".

Contributor

equalsraf commented Feb 2, 2015

And now that the visual studio generator is working we can build using clang-cl (not for third-party deps though) just `cmake -G "Visual Studio 12" -T "LLVM-vs2013".

@aktau

This comment has been minimized.

Show comment
Hide comment
@aktau

aktau Feb 2, 2015

Member

Yes, works.

It runs on Windows?

Anyhow, the progress on this is unbelievably awesome (despite not being a Windows user).

Even if it just compiles, I'd like to advocate getting AppVeyor integrated like travis if at all possible, so we at least don't regress on compilation.

Member

aktau commented Feb 2, 2015

Yes, works.

It runs on Windows?

Anyhow, the progress on this is unbelievably awesome (despite not being a Windows user).

Even if it just compiles, I'd like to advocate getting AppVeyor integrated like travis if at all possible, so we at least don't regress on compilation.

@ebraminio

This comment has been minimized.

Show comment
Hide comment
@ebraminio

ebraminio Feb 2, 2015

Yes.

a

Of course the progress is awesome. Thank you guys!

ebraminio commented Feb 2, 2015

Yes.

a

Of course the progress is awesome. Thank you guys!

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Feb 2, 2015

Contributor

@aktau sadly the mingw version in Travis seems too old, otherwise we could enable Mingw cross compile.

BTW the binaries run in wine, so if anyone wants to fire up the unit tests over TCP that would be cool (I have not tried the tests - but once I accidentally started nvim.exe against the python interpreter from Linux).

Contributor

equalsraf commented Feb 2, 2015

@aktau sadly the mingw version in Travis seems too old, otherwise we could enable Mingw cross compile.

BTW the binaries run in wine, so if anyone wants to fire up the unit tests over TCP that would be cool (I have not tried the tests - but once I accidentally started nvim.exe against the python interpreter from Linux).

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Feb 3, 2015

Contributor

@justinmk a missing item that got lost along the way

  • Implement unix_expandpath for Windows (check original Vim source for expandpath)
Contributor

equalsraf commented Feb 3, 2015

@justinmk a missing item that got lost along the way

  • Implement unix_expandpath for Windows (check original Vim source for expandpath)
@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Feb 5, 2015

Contributor

Crash dump for MSVC builds - to reproduce connect over msgpack-rpc and then disconnect - I cannot debug this at the moment, since I have no workable debugger here. If anyone groks stack/register dumps, some insight would be appreciated.

Looking at the trace it seems there might be something wrong with the kv in call_set_error. Oddly I cannot reproduce this with Mingw builds.

Contributor

equalsraf commented Feb 5, 2015

Crash dump for MSVC builds - to reproduce connect over msgpack-rpc and then disconnect - I cannot debug this at the moment, since I have no workable debugger here. If anyone groks stack/register dumps, some insight would be appreciated.

Looking at the trace it seems there might be something wrong with the kv in call_set_error. Oddly I cannot reproduce this with Mingw builds.

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Feb 5, 2015

Contributor

Nevermind, #1846 fixes it.

Contributor

equalsraf commented Feb 5, 2015

Nevermind, #1846 fixes it.

@aktau

This comment has been minimized.

Show comment
Hide comment
@aktau

aktau Feb 5, 2015

Member

Seems to be GNU GAS (AT&T) syntax: http://en.wikibooks.org/wiki/X86_Assembly/GAS_Syntax

0x0061bd5b call_set_error+0x5b [c:\projects\neovim\src\nvim\msgpack_rpc\channel.c:728] in nvim: movl    0x0(%edx,%eax,4),%ecx

Is: 

Load 0x0(%edx,%eax,4) into register %ecx

Aka:

%ecx = *(%edx + (%eax * 4))

With

%eax = 00000000
%edx = 00000000

Gives

%ecx = *0x0 // NULL pointer dereference

Helpfully, the line in question was indicated:

#define kv_A(v, i) ((v).a[(i)])
#define kv_size(v) ((v).n)

static void call_set_error(Channel *channel, char *msg)
{
  ELOG("Msgpack-RPC error: %s", msg);
  for (size_t i = 0; i < kv_size(channel->call_stack); i++) {
    ChannelCallFrame *frame = kv_A(channel->call_stack, i); // CRASH HERE
    frame->returned = true;
    frame->errored = true;
    frame->result = STRING_OBJ(cstr_to_string(msg));
  }

  close_channel(channel);
}

From this I surmise that %eax is the looping variable, and that the loop counter was 0 at the moment of crashing. We see that kv_size() returned at least 1, else the loop wouldn't have been entered. The register containing it seems to have been overwritten as I can't see any other low-values in the register set.

It all seems a bit fishy, channel->call_stack.n > 0 yet channel->call_stack.a == NULL (the array). That shouldn't happen. I can only think of two reasons why that would happen:

  1. Badly initialized kvec containing garbage values, of which the second word is accidentally NULL. Yet this seems to be properly done by kv_init() in register_channel(). I believe the channel comes from channel_from_stdio() or channel_from_stream() because I don't see job_out() in the stacktrace.
  2. Some part of the code actually manually setting n instead of going through the kv_ functions
  3. Wat...?

I would need to see the disassembly of the call_set_error() function that MSVC compiled to know a bit more. I'm also a bit less familiar with Microsofts 64-bit calling convention (or did you build a 32-bit binary? It kinda looks like it from your dump).

Notes to self:

static void complete_call(msgpack_object *obj, Channel *channel)
{
  ChannelCallFrame *frame = kv_A(channel->call_stack,
                             kv_size(channel->call_stack) - 1);
  frame->returned = true;
  frame->errored = obj->via.array.ptr[2].type != MSGPACK_OBJECT_NIL;

  if (frame->errored) {
    msgpack_rpc_to_object(&obj->via.array.ptr[2], &frame->result);
  } else {
    msgpack_rpc_to_object(&obj->via.array.ptr[3], &frame->result);
  }
}

Are we sure the first line never underflows? I assume so, since the name of the function implies that there is at least one frame, but was just noting down.

Slightly related sidenote: why are we doing this with a kvec instead of a plain array? I suppose we need to set some upper limit to the stack frames and we could just make that part of the function: ChannelCallFrame call_stack[MAX_CALL_FRAMES] instead of kvec_t(ChannelCallFrame *) call_stack. It will likely get rid of some extra allocations and we'd have to worry less about these things.

@tarruda one callchain caught my eye. Consider a channel created with channel_from_stream(). It has streams.uv != NULL. parse_msgpack() gets called with eof == 1. So the following if-statement gets entered:

  if (eof) {
    close_channel(channel);
    call_set_error(channel, "Channel was closed by the client");
    return;
  }

Let's look a bit closer at close_channel():

/// Close the channel streams/job and free the channel resources.
static void close_channel(Channel *channel)
{
  if (channel->closed) {
    return;
  }

  channel->closed = true;
  if (channel->is_job) {
    if (channel->data.job) {
      job_stop(channel->data.job);
    }
  } else {
    rstream_free(channel->data.streams.read);
    wstream_free(channel->data.streams.write);
    uv_handle_t *handle = (uv_handle_t *)channel->data.streams.uv;
    if (handle) {
      uv_close(handle, close_cb);
      free_channel(channel);
    } else {
      event_push((Event) { .handler = on_stdio_close }, false);
    }
  }
}

We see that near the end, if streams.uv != NULL, this will get run:

    if (handle) {
      uv_close(handle, close_cb);
      free_channel(channel);
    } 

Incidentally, free_channel() frees the kvec (and everything else). But this will get used in call_set_error().

Does this chain of events sound plausible?

In any case, I think we shouldn't conflate close_channel() and free_channel() in this way. Especially if it only happens very conditionally. Async programming is difficult enough without having to keep these things in one's head.

Member

aktau commented Feb 5, 2015

Seems to be GNU GAS (AT&T) syntax: http://en.wikibooks.org/wiki/X86_Assembly/GAS_Syntax

0x0061bd5b call_set_error+0x5b [c:\projects\neovim\src\nvim\msgpack_rpc\channel.c:728] in nvim: movl    0x0(%edx,%eax,4),%ecx

Is: 

Load 0x0(%edx,%eax,4) into register %ecx

Aka:

%ecx = *(%edx + (%eax * 4))

With

%eax = 00000000
%edx = 00000000

Gives

%ecx = *0x0 // NULL pointer dereference

Helpfully, the line in question was indicated:

#define kv_A(v, i) ((v).a[(i)])
#define kv_size(v) ((v).n)

static void call_set_error(Channel *channel, char *msg)
{
  ELOG("Msgpack-RPC error: %s", msg);
  for (size_t i = 0; i < kv_size(channel->call_stack); i++) {
    ChannelCallFrame *frame = kv_A(channel->call_stack, i); // CRASH HERE
    frame->returned = true;
    frame->errored = true;
    frame->result = STRING_OBJ(cstr_to_string(msg));
  }

  close_channel(channel);
}

From this I surmise that %eax is the looping variable, and that the loop counter was 0 at the moment of crashing. We see that kv_size() returned at least 1, else the loop wouldn't have been entered. The register containing it seems to have been overwritten as I can't see any other low-values in the register set.

It all seems a bit fishy, channel->call_stack.n > 0 yet channel->call_stack.a == NULL (the array). That shouldn't happen. I can only think of two reasons why that would happen:

  1. Badly initialized kvec containing garbage values, of which the second word is accidentally NULL. Yet this seems to be properly done by kv_init() in register_channel(). I believe the channel comes from channel_from_stdio() or channel_from_stream() because I don't see job_out() in the stacktrace.
  2. Some part of the code actually manually setting n instead of going through the kv_ functions
  3. Wat...?

I would need to see the disassembly of the call_set_error() function that MSVC compiled to know a bit more. I'm also a bit less familiar with Microsofts 64-bit calling convention (or did you build a 32-bit binary? It kinda looks like it from your dump).

Notes to self:

static void complete_call(msgpack_object *obj, Channel *channel)
{
  ChannelCallFrame *frame = kv_A(channel->call_stack,
                             kv_size(channel->call_stack) - 1);
  frame->returned = true;
  frame->errored = obj->via.array.ptr[2].type != MSGPACK_OBJECT_NIL;

  if (frame->errored) {
    msgpack_rpc_to_object(&obj->via.array.ptr[2], &frame->result);
  } else {
    msgpack_rpc_to_object(&obj->via.array.ptr[3], &frame->result);
  }
}

Are we sure the first line never underflows? I assume so, since the name of the function implies that there is at least one frame, but was just noting down.

Slightly related sidenote: why are we doing this with a kvec instead of a plain array? I suppose we need to set some upper limit to the stack frames and we could just make that part of the function: ChannelCallFrame call_stack[MAX_CALL_FRAMES] instead of kvec_t(ChannelCallFrame *) call_stack. It will likely get rid of some extra allocations and we'd have to worry less about these things.

@tarruda one callchain caught my eye. Consider a channel created with channel_from_stream(). It has streams.uv != NULL. parse_msgpack() gets called with eof == 1. So the following if-statement gets entered:

  if (eof) {
    close_channel(channel);
    call_set_error(channel, "Channel was closed by the client");
    return;
  }

Let's look a bit closer at close_channel():

/// Close the channel streams/job and free the channel resources.
static void close_channel(Channel *channel)
{
  if (channel->closed) {
    return;
  }

  channel->closed = true;
  if (channel->is_job) {
    if (channel->data.job) {
      job_stop(channel->data.job);
    }
  } else {
    rstream_free(channel->data.streams.read);
    wstream_free(channel->data.streams.write);
    uv_handle_t *handle = (uv_handle_t *)channel->data.streams.uv;
    if (handle) {
      uv_close(handle, close_cb);
      free_channel(channel);
    } else {
      event_push((Event) { .handler = on_stdio_close }, false);
    }
  }
}

We see that near the end, if streams.uv != NULL, this will get run:

    if (handle) {
      uv_close(handle, close_cb);
      free_channel(channel);
    } 

Incidentally, free_channel() frees the kvec (and everything else). But this will get used in call_set_error().

Does this chain of events sound plausible?

In any case, I think we shouldn't conflate close_channel() and free_channel() in this way. Especially if it only happens very conditionally. Async programming is difficult enough without having to keep these things in one's head.

@aktau

This comment has been minimized.

Show comment
Hide comment
@aktau

aktau Feb 5, 2015

Member

Alright... I hadn't noticed that #1846 fixes it. Though I'm still confused as to why this wasn't noticed before. If the chain of events I described above is what's happening, then we've gotten really lucky. But also, Valgrand or ASan would've/should've totally caught this. Why haven't they? Do we not test libuv stream based channels?

Member

aktau commented Feb 5, 2015

Alright... I hadn't noticed that #1846 fixes it. Though I'm still confused as to why this wasn't noticed before. If the chain of events I described above is what's happening, then we've gotten really lucky. But also, Valgrand or ASan would've/should've totally caught this. Why haven't they? Do we not test libuv stream based channels?

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Feb 5, 2015

Member

Note about the terminal UI on windows: We may be able to get it working by simply using libuv tty handle(with #1820), turns out libuv implements an ansi escape code parser that translates into native windows console API calls! ref

Member

tarruda commented Feb 5, 2015

Note about the terminal UI on windows: We may be able to get it working by simply using libuv tty handle(with #1820), turns out libuv implements an ansi escape code parser that translates into native windows console API calls! ref

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 20, 2015

Member

@bfredl I misunderstood then. 👍 to restoring that logic.

Member

justinmk commented Aug 20, 2015

@bfredl I misunderstood then. 👍 to restoring that logic.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 23, 2015

Regarding:

Add include guards: sys/time.h unistd.h

With MSVC, we can probably use <io.h> for <unistd.h> and <pdh.h> for <sys/time.h> (I haven't used <pdh.h> myself, just found folks use it to get low-level processor time info etc. on windows, related: libuv/libuv#342).

ghost commented Aug 23, 2015

Regarding:

Add include guards: sys/time.h unistd.h

With MSVC, we can probably use <io.h> for <unistd.h> and <pdh.h> for <sys/time.h> (I haven't used <pdh.h> myself, just found folks use it to get low-level processor time info etc. on windows, related: libuv/libuv#342).

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 24, 2015

Member

@jasonwilliams200OK Thanks! I see #810 is using <io.h>, and the only place left that we are using sys/time.h is in log.c and os_defs.h for strftime(). It seems that MSVC's time.h provides strftime(), which #810 also has included. So I'm marking that item completed. @equalsraf can you confirm?

Member

justinmk commented Aug 24, 2015

@jasonwilliams200OK Thanks! I see #810 is using <io.h>, and the only place left that we are using sys/time.h is in log.c and os_defs.h for strftime(). It seems that MSVC's time.h provides strftime(), which #810 also has included. So I'm marking that item completed. @equalsraf can you confirm?

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 24, 2015

Member

I just removed both remaining instances of sys/time.h, and it compiled fine.

Member

justinmk commented Aug 24, 2015

I just removed both remaining instances of sys/time.h, and it compiled fine.

@christiand

This comment has been minimized.

Show comment
Hide comment
@christiand

christiand Oct 5, 2015

Not sure if this is the right place to post, but I'm curious as to the team's thoughts regarding a Windows GUI. No so much "when", but in what form? i.e. if i'm looking to use it professionally and contribute, where to best spend the energy? The assorted "nvim-inside-3rdparty-app" like atom/qt/etc. are interesting.. but ultimately solo interest projects at this point. I'd like to believe that there would be a native supported experience a la gvim.
I'm not to familiar with where you guys drew the line when refactoring vim (and I don't know vim source to begin with..) but is gvim's gui layer a candidate at all?

christiand commented Oct 5, 2015

Not sure if this is the right place to post, but I'm curious as to the team's thoughts regarding a Windows GUI. No so much "when", but in what form? i.e. if i'm looking to use it professionally and contribute, where to best spend the energy? The assorted "nvim-inside-3rdparty-app" like atom/qt/etc. are interesting.. but ultimately solo interest projects at this point. I'd like to believe that there would be a native supported experience a la gvim.
I'm not to familiar with where you guys drew the line when refactoring vim (and I don't know vim source to begin with..) but is gvim's gui layer a candidate at all?

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Oct 14, 2015

Contributor

@christiand there is a an issue for discussing a reference GUI in Neovim (#1639) you might want to have a look at.

For windows specifically there are a number of third party clients that work right now (qt, atom, C#), there also the one implemented as part of neovim/python-client. Technically Neovim treats all UIs as clients, even the built in UI - so all of them get the same API, at the very least this should make it easier for the best solution to race to the top.

Contributor

equalsraf commented Oct 14, 2015

@christiand there is a an issue for discussing a reference GUI in Neovim (#1639) you might want to have a look at.

For windows specifically there are a number of third party clients that work right now (qt, atom, C#), there also the one implemented as part of neovim/python-client. Technically Neovim treats all UIs as clients, even the built in UI - so all of them get the same API, at the very least this should make it easier for the best solution to race to the top.

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Oct 15, 2015

Contributor

Question: How do we feel about static vs dynamic linking and distributing DLLs?

For Unix Neovim links statically as much as possible but for Windows this makes things harder:

  • trying to find pre-compiled third party dependencies (e.g. libintl or iconv - seems nobody distributes /MT libraries for MSVC).
  • Some of our pending issues with lua relate to using static linking (luarocks dislikes this in rocks recipes)
  • Our third-party recipes get a bit more complex in order to enforce /MT

Using dynamic linking also has its own problems:

  • Finding and bundling DLL dependencies with binaries (cmake has BundleUtilities, but I never tested) - there are scripts for this lying around the internet
  • Need to bundle the MSVC runtime DLLs too
Contributor

equalsraf commented Oct 15, 2015

Question: How do we feel about static vs dynamic linking and distributing DLLs?

For Unix Neovim links statically as much as possible but for Windows this makes things harder:

  • trying to find pre-compiled third party dependencies (e.g. libintl or iconv - seems nobody distributes /MT libraries for MSVC).
  • Some of our pending issues with lua relate to using static linking (luarocks dislikes this in rocks recipes)
  • Our third-party recipes get a bit more complex in order to enforce /MT

Using dynamic linking also has its own problems:

  • Finding and bundling DLL dependencies with binaries (cmake has BundleUtilities, but I never tested) - there are scripts for this lying around the internet
  • Need to bundle the MSVC runtime DLLs too
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 15, 2015

One swell approach could be to first clone them conditionally in CMake (quasi-submodule):

if(WIN32)
  # first delete the existing
  execute_process( COMMAND powershell "rm -r -fo ${CMAKE_CURRENT_LIST_DIR}/libint 2>&1> $null" ) 

  # then clone
  execute_process( COMMAND git clone https://github.com/evaleev/libint --recursive -b v1 ${CMAKE_CURRENT_LIST_DIR}/libint ) 
end

Then build them. However, there are at least two issues with this approach:

  • If the downstream doesn't have CMake, then we will need to configure that in our script too (perhaps add a separate cmake module .cmake for each lib).
  • If the dependency lib doesn't exist on github/git (using SVN/CVS etc.), then we will need to mirror it.

Besides technical issues, I am not sure how does the license [in]compatibilities work in [up/down]stream / submodule scenarios, but it would be good to iron that out with some OSS solicitor too. :)

This is extra work at build time, but I think this way we will have control and freedom to however we want to ship it (we may end up building .dll and bundle it in installer, instead of statically linking .lib for some good reasons or vice versa).

ghost commented Oct 15, 2015

One swell approach could be to first clone them conditionally in CMake (quasi-submodule):

if(WIN32)
  # first delete the existing
  execute_process( COMMAND powershell "rm -r -fo ${CMAKE_CURRENT_LIST_DIR}/libint 2>&1> $null" ) 

  # then clone
  execute_process( COMMAND git clone https://github.com/evaleev/libint --recursive -b v1 ${CMAKE_CURRENT_LIST_DIR}/libint ) 
end

Then build them. However, there are at least two issues with this approach:

  • If the downstream doesn't have CMake, then we will need to configure that in our script too (perhaps add a separate cmake module .cmake for each lib).
  • If the dependency lib doesn't exist on github/git (using SVN/CVS etc.), then we will need to mirror it.

Besides technical issues, I am not sure how does the license [in]compatibilities work in [up/down]stream / submodule scenarios, but it would be good to iron that out with some OSS solicitor too. :)

This is extra work at build time, but I think this way we will have control and freedom to however we want to ship it (we may end up building .dll and bundle it in installer, instead of statically linking .lib for some good reasons or vice versa).

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Oct 15, 2015

Contributor

One swell approach could be to first clone them conditionally in CMake (quasi-submodule):

This we already have, but we grab tarballs of specific versions instead of cloning repositories.

  • If the downstream doesn't have CMake, then we will need to configure that in our script too

This is what the modules in third-party/cmake/Build*.cmake are doing right now.

But some cases are trickier - e.g. iconv only builds with autotools and these are only available in Unix or MSYS and some compile steps are non trivial (merge config.h.in with config options), nothing short of writing our own CMake recipe would work. The same probably applies for libintl and other GNU libraries.

f the dependency lib doesn't exist on github/git (using SVN/CVS etc.), then we will need to mirror it.

Yup, this is what is done for libtermkey and unibilium.

Besides technical issues, I am not sure how does the license [in]compatibilities work in [up/down]stream / submodule scenarios, but it would be good to iron that out with some OSS solicitor too. :)

I certainly hope not, since our dependencies are the same for Windows and Unix (so far) - but it would not hurt to check.

My main doubts in regards to MSVC is whether or not to build static libraries (/MT) of our dependencies, or to just build DLLs (and tweak the build process to bundle them with the Neovim binary).

As for grabbing third-party builds from someplace else - we will have to do it, at least for some runtime tools (#1811) but maybe not for build dependencies.

Contributor

equalsraf commented Oct 15, 2015

One swell approach could be to first clone them conditionally in CMake (quasi-submodule):

This we already have, but we grab tarballs of specific versions instead of cloning repositories.

  • If the downstream doesn't have CMake, then we will need to configure that in our script too

This is what the modules in third-party/cmake/Build*.cmake are doing right now.

But some cases are trickier - e.g. iconv only builds with autotools and these are only available in Unix or MSYS and some compile steps are non trivial (merge config.h.in with config options), nothing short of writing our own CMake recipe would work. The same probably applies for libintl and other GNU libraries.

f the dependency lib doesn't exist on github/git (using SVN/CVS etc.), then we will need to mirror it.

Yup, this is what is done for libtermkey and unibilium.

Besides technical issues, I am not sure how does the license [in]compatibilities work in [up/down]stream / submodule scenarios, but it would be good to iron that out with some OSS solicitor too. :)

I certainly hope not, since our dependencies are the same for Windows and Unix (so far) - but it would not hurt to check.

My main doubts in regards to MSVC is whether or not to build static libraries (/MT) of our dependencies, or to just build DLLs (and tweak the build process to bundle them with the Neovim binary).

As for grabbing third-party builds from someplace else - we will have to do it, at least for some runtime tools (#1811) but maybe not for build dependencies.

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Dec 14, 2015

Contributor
  • Get ffi working with MSVC - specially how to handle __pragma (see d530fb6 for more details)
Contributor

equalsraf commented Dec 14, 2015

  • Get ffi working with MSVC - specially how to handle __pragma (see d530fb6 for more details)
@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 14, 2015

Member

@equalsraf nice work. But the functional tests don't use FFI, do they run?

Member

justinmk commented Dec 14, 2015

@equalsraf nice work. But the functional tests don't use FFI, do they run?

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Dec 14, 2015

Contributor

They don't run, the lua client was not working.in Windows last I checked. I have some fixes lying around, but there were some issues with luarocks failing to build.

Contributor

equalsraf commented Dec 14, 2015

They don't run, the lua client was not working.in Windows last I checked. I have some fixes lying around, but there were some issues with luarocks failing to build.

@dt1973

This comment has been minimized.

Show comment
Hide comment
@dt1973

dt1973 Jan 10, 2016

Would it be possible to come up with some sorta Windows installer?

Thanks!

dt1973 commented Jan 10, 2016

Would it be possible to come up with some sorta Windows installer?

Thanks!

@fwalch

This comment has been minimized.

Show comment
Hide comment
@fwalch

fwalch Jan 10, 2016

Member

@dt1973 I think it is already being addressed in #810 (equalsraf@025760f), which is merged into this repo step-by-step.

Member

fwalch commented Jan 10, 2016

@dt1973 I think it is already being addressed in #810 (equalsraf@025760f), which is merged into this repo step-by-step.

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Jan 10, 2016

Contributor

@fwalch, yes I added an installer its available as an artifact in appveyor. For some reason the $PATH is not being set, seems to be a problem with CPack/NSIS.

Contributor

equalsraf commented Jan 10, 2016

@fwalch, yes I added an installer its available as an artifact in appveyor. For some reason the $PATH is not being set, seems to be a problem with CPack/NSIS.

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Jan 24, 2016

Contributor

@leonerd we have a working libvterm for Windows with equalsraf/libvterm-win@59ef3ef builds with Mingw/MSVC.

Thanks @brcolow

Contributor

equalsraf commented Jan 24, 2016

@leonerd we have a working libvterm for Windows with equalsraf/libvterm-win@59ef3ef builds with Mingw/MSVC.

Thanks @brcolow

@brcolow

This comment has been minimized.

Show comment
Hide comment
@brcolow

brcolow Jan 24, 2016

Contributor

Also builds with cygwin (which is what I used) (GNU Make 4.1 Built for x86_64-unknown-cygwin)

Contributor

brcolow commented Jan 24, 2016

Also builds with cygwin (which is what I used) (GNU Make 4.1 Built for x86_64-unknown-cygwin)

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Apr 19, 2016

Contributor

The changes to STATIC_ASSERT in 6a35f2a don't build on MSVC

eval.c(12962): warning C4003: not enough actual parameters for macro 'STATIC_ASSERT_STATEMENT'
eval.c(12962): error C2059: syntax error: ')'

I was expecting VA_ARGS to actually work here but it seems to expand with a trailing coma. this

  STATIC_ASSERT(sizeof(u.prof) == sizeof(u) && sizeof(u.split) == sizeof(u),
      "type punning will produce incorrect results on this platform");

turns into

  do {  static_assert(sizeof(u.prof) == sizeof(u) && sizeof(u.split) == sizeof(u),
    "type punning will produce incorrect results on this platform", );  } while (0);

@ZyX-I any particular reason for STATIC_ASSERT to have variable arguments? all cases seem to take two arguments (cond, msg)

Contributor

equalsraf commented Apr 19, 2016

The changes to STATIC_ASSERT in 6a35f2a don't build on MSVC

eval.c(12962): warning C4003: not enough actual parameters for macro 'STATIC_ASSERT_STATEMENT'
eval.c(12962): error C2059: syntax error: ')'

I was expecting VA_ARGS to actually work here but it seems to expand with a trailing coma. this

  STATIC_ASSERT(sizeof(u.prof) == sizeof(u) && sizeof(u.split) == sizeof(u),
      "type punning will produce incorrect results on this platform");

turns into

  do {  static_assert(sizeof(u.prof) == sizeof(u) && sizeof(u.split) == sizeof(u),
    "type punning will produce incorrect results on this platform", );  } while (0);

@ZyX-I any particular reason for STATIC_ASSERT to have variable arguments? all cases seem to take two arguments (cond, msg)

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 20, 2016

@equalsraf, could you provide steps to repro? I tested the following code with VS2015 update 2:

struct test
{
    int prof;
    int split;
};

#define STATIC_ASSERT_PRAGMA_START
#define STATIC_ASSERT_PRAGMA_END
#define STATIC_ASSERT(cond, msg) \
    do \
    { \
      STATIC_ASSERT_PRAGMA_START \
      STATIC_ASSERT_STATEMENT(cond, msg); \
      STATIC_ASSERT_PRAGMA_END \
    } while (0)

# define STATIC_ASSERT_STATEMENT(cond, msg) static_assert(cond, msg)

int main(void)
{
    struct test u;
    STATIC_ASSERT(sizeof(u.prof) == sizeof(u) && sizeof(u.split) == sizeof(u),
      "type punning will produce incorrect results on this platform");
}

via http://webcompiler.cloudapp.net/ using /TC (both capital letters) compiler flag, and it gives type punning... (expected) compiler error.

ghost commented Apr 20, 2016

@equalsraf, could you provide steps to repro? I tested the following code with VS2015 update 2:

struct test
{
    int prof;
    int split;
};

#define STATIC_ASSERT_PRAGMA_START
#define STATIC_ASSERT_PRAGMA_END
#define STATIC_ASSERT(cond, msg) \
    do \
    { \
      STATIC_ASSERT_PRAGMA_START \
      STATIC_ASSERT_STATEMENT(cond, msg); \
      STATIC_ASSERT_PRAGMA_END \
    } while (0)

# define STATIC_ASSERT_STATEMENT(cond, msg) static_assert(cond, msg)

int main(void)
{
    struct test u;
    STATIC_ASSERT(sizeof(u.prof) == sizeof(u) && sizeof(u.split) == sizeof(u),
      "type punning will produce incorrect results on this platform");
}

via http://webcompiler.cloudapp.net/ using /TC (both capital letters) compiler flag, and it gives type punning... (expected) compiler error.

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Apr 20, 2016

Contributor

@jasonwilliams200OK sadly no, your example works fine here.

Contributor

equalsraf commented Apr 20, 2016

@jasonwilliams200OK sadly no, your example works fine here.

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Apr 20, 2016

Contributor

This seems like a match http://stackoverflow.com/questions/10002296/macroized-parameters and the proposed fix seems to work, or at least builds, but I don't have a fix that works in all compilers.

Contributor

equalsraf commented Apr 20, 2016

This seems like a match http://stackoverflow.com/questions/10002296/macroized-parameters and the proposed fix seems to work, or at least builds, but I don't have a fix that works in all compilers.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 21, 2016

@equalsraf, my example fails if we use #define STATIC_ASSERT(...) and then STATIC_ASSERT_STATEMENT(__VA_ARGS__).

Found related active bugs:
https://connect.microsoft.com/VisualStudio/Feedback/Details/1232378 and
https://connect.microsoft.com/VisualStudio/Feedback/Details/1099052.

ghost commented Apr 21, 2016

@equalsraf, my example fails if we use #define STATIC_ASSERT(...) and then STATIC_ASSERT_STATEMENT(__VA_ARGS__).

Found related active bugs:
https://connect.microsoft.com/VisualStudio/Feedback/Details/1232378 and
https://connect.microsoft.com/VisualStudio/Feedback/Details/1099052.

@justinmk justinmk removed the compiler:msvc label Jun 16, 2016

@equalsraf equalsraf referenced this issue Aug 16, 2016

Closed

Windows: improvements #5229

10 of 14 tasks complete

@justinmk justinmk closed this Aug 16, 2016

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 16, 2016

Member

Replaced by #5229

Member

justinmk commented Aug 16, 2016

Replaced by #5229

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