[RDY] Building with VS2017 #8084

b-r-o-c-k · 2018-03-01T02:13:36Z

This PR makes it possible to build Neovim with MSVC and Visual Studio 2017. After making changes to the CMakeLists and fixing some errors caused by missing headers, I was able to get it to compile successfully. MSYS2 isn't required anymore however, it could be used to install gperf which is the only dependency not downloaded by CMake. There are still runtime errors to fix, but I'd some feedback on my progress.

Todo:

~~Update LuaRocks after this wget issue is resolved~~ A patch was added for LuaRocks, to make it use the curl.exe bundled inside the wintools.zip
Fix runtime errors
Check for regressions with other platforms/compilers
Add Appveyor CI for MSVC?
Update build documentation

Preprocessor directives on the first line of the file were not being parsed.

MSVC has the __restrict keyword and a marco is defined for it in `win_defs.h`.

With MSVC, STDOUT_FILENO and STDERR_FILENO are defined as function calls instead of constants, meaning they can't be assigned to enum values. The enum was only used in one file, so it has been removed. A definition for STDIN_FILENO has been added that is consistent with the other two definitions.

jamessan

Thanks for the PR! Improving our Windows support is much appreciated.

My only comments so far are to avoid forcing us to carry patches for dependencies if at all possible. Please work with the respective upstreams to try and fix the issues there.

If we need to carry patches, please do it using the existing mechanisms provided by ExternalProject_Add instead of having us pull from your forks.

jamessan · 2018-03-01T02:53:08Z

third-party/CMakeLists.txt

-set(UNIBILIUM_URL https://github.com/mauke/unibilium/archive/v2.0.0.tar.gz)
-set(UNIBILIUM_SHA256 78997d38d4c8177c60d3d0c1aa8c53fd0806eb21825b7b335b1768d7116bc1c1)
+if(WIN32)
+  set(UNIBILIUM_URL https://github.com/b-r-o-c-k/unibilium/archive/v2.0.0.tar.gz)


Can you please try to get this merged upstream before asking us to pull from somewhere else? I would suspect the header change to be fairly uncontroversial. Not sure about the cmake side of things.

jamessan · 2018-03-01T03:02:05Z

third-party/CMakeLists.txt

@@ -119,8 +119,13 @@ set(LIBTERMKEY_URL http://www.leonerd.org.uk/code/libtermkey/libtermkey-0.20.tar
 set(LIBTERMKEY_SHA256 6c0d87c94ab9915e76ecd313baec08dedf3bd56de83743d9aa923a081935d2f5)
 endif()

+if(MSVC)
+set(LIBVTERM_URL https://github.com/b-r-o-c-k/libvterm/archive/eb386b1d82f7d07363c9133b7aa06902ccd555fe.tar.gz)


Please try to work with upstream on MSVC support. I thought VS 2015 had C99 support, so why don't VLA work?

Although they have added support for some C99 features, they still don't support VLAs. The C90 standard is the only one they aim to be fully compliant with. You can read their response to the VLA feature request here.

jamessan · 2018-03-01T03:06:37Z

@janlazo would probably be a good person to help review this.

A header was added for compatibility with MSVC and CMakeLists.txt was added for building with CMake.

The patch removes VLAs because MSVC does not support them.

LuaRocks bundles an outdated wget.exe for downloading packages on Windows. It is too old to support GitHub's TLS, so this patch will replace it with curl.

b-r-o-c-k · 2018-03-02T05:06:05Z

I contacted the libvterm developers and submitted a PR to Unibilium with the necessary patches. In case they don't get merged quickly, I also added the patches to this project.

zhaozg · 2018-03-02T11:31:11Z

@b-r-o-c-k It's better if appveyor build script.

justinmk · 2018-03-04T19:27:44Z

src/nvim/os/os_defs.h

-  OS_STDIN_FILENO = STDIN_FILENO,
-  OS_STDOUT_FILENO = STDOUT_FILENO,
-  OS_STDERR_FILENO = STDERR_FILENO,
-};


@ZyX-I any objection to removing this?

This is not used in unit tests currently, so no objections. If it was then enum would be required.

justinmk · 2018-03-04T19:30:52Z

src/nvim/os/unix_defs.h

 #include <unistd.h>
+#include <sys/param.h>


The comment is confusing, let's delete it, or replace it with simply:

// unix-only headers (not available on Windows)

It is clear what's going on by analogy with win_defs.h.

justinmk · 2018-03-04T19:41:27Z

@b-r-o-c-k This is extremely appreciated and nicely done. Other than my comments LGTM and would like to merge it. As @zhaozg mentioned we would want to quickly followup with a AppVeyor MSVC build, but in the meantime this is worth including.

The ruby-related hang on AppVeyor can be investigated later (could be due to a upsteram change in the ruby client).

MSVC predefines `_WIN32`, but not `WIN32`. Also, some unnecessary includes have been removed.

MSVC doesn't have unistd.h or usleep() so it was replaced with the Sleep() WinAPI function.

Shougo · 2018-03-07T02:39:50Z

The MSVC build seems to have only 4 failed tests (nice!). The python3_spec failures are probably an accident of neovim/pynvim#306, not related to this PR.

The change is reverted in Windows now. So I think no problem.

justinmk · 2018-03-07T07:39:49Z

Only thing left in your todo list is "Update build documentation", what did you have in mind? What steps are needed for a new contributor to get going with MSVC ?

Shougo · 2018-03-07T07:52:28Z

lint is failed. But it seems non related to the changes.

rc/nvim/eval.c:5943:  Extra space before ( in function call  [whitespace/parens] [4]
src/nvim/eval.c:5944:  Extra space before ( in function call  [whitespace/parens] [4]
src/nvim/option.c:2444:  /*-style comment found, it should be replaced with //-style.  /*-style comments are only allowed inside macros.  Note that you should not use /*-style comments to document macros itself, use doxygen-style comments for this.  [readability/old_style_comment] [5]
src/nvim/option.c:3312:  Small and focused functions are preferred: () has 696 non-comment lines (error triggered by exceeding 500 lines).  [readability/fn_size] [1]
Total errors found: 4

justinmk · 2018-03-07T09:46:46Z

third-party/CMakeLists.txt

@@ -50,6 +50,13 @@ endif()

 option(USE_EXISTING_SRC_DIR "Skip download of deps sources in case of existing source directory." OFF)

+if(WIN32)
+  find_package(Git)


@b-r-o-c-k should find_program be used instead? https://cmake.org/cmake/help/v3.0/command/find_program.html

I don't know why we're using find_package in GetGitRevisionDescription.cmake, that was copied from some other project.

find_package will use FindGit.cmake which is presumably better because it can find Git installations with non-standard names and paths.

justinmk · 2018-03-07T09:53:59Z

ci/build.bat

 echo on
 if "%CONFIGURATION%" == "MINGW_32" (
  set ARCH=i686
  set BITS=32
-) else (
+) else if "%CONFIGURATION%" == "MINGW_64" (


This should stay as else. We need ARCH and BITS to be set for MINGW_64-gcov.

justinmk · 2018-03-07T09:54:20Z

ci/build.bat

  set ARCH=x86_64
  set BITS=64
-)
-if "%CONFIGURATION%" == "MINGW_64-gcov" (
+) else if "%CONFIGURATION%" == "MINGW_64-gcov" (


Just if, not else if.

justinmk · 2018-03-07T09:56:46Z

ci/build.bat

@@ -42,19 +48,19 @@ where.exe neovim-node-host.cmd || goto :error

 mkdir .deps
 cd .deps
-cmake -G "MinGW Makefiles" -DCMAKE_BUILD_TYPE=RelWithDebInfo ..\third-party\ || goto :error
-mingw32-make VERBOSE=1 || goto :error
+cmake -G %CMAKE_GENERATOR% -DCMAKE_BUILD_TYPE=RelWithDebInfo ..\third-party\ || goto :error


~~should %CMAKE_GENERATOR% be in quotes?~~

No, build log shows that it's quoted implicitly:

[00:01:01] C:\projects\neovim\.deps>cmake -G "Visual Studio 15 2017 Win64" -DCMAKE_BUILD_TYPE=RelWithDebInfo ..\third-party\ || goto :error

Right, but the quotes aren't implicit. Quotes are not needed on that line because they are part of the value assigned here:

set CMAKE_GENERATOR="Visual Studio 15 2017"

Batch is a very strange scripting language and it has mostly been made obsolete by PowerShell.

set "CMAKE_GENERATOR=Visual Studio 15 2017"
If any argument has a space, double-quote the entire thing.

b-r-o-c-k · 2018-03-08T04:01:33Z

Build Instructions for Visual Studio 2017

Install all of the following:
- Visual Studio 2017 with the Desktop development with C++ workload
- CMake
- Python 2 (required for libuv)
- gperf

Build the dependencies (this will work in Command Prompt or PowerShell):

mkdir .deps
cd .deps
cmake -G "Visual Studio 15 2017" -DCMAKE_BUILD_TYPE=RelWithDebInfo -DPYTHON_EXECUTABLE="C:\path\to\python.exe" ..\third-party\
cmake --build .

Open the neovim folder in Visual Studio. This can be done from the file explorer using the "Open in Visual Studio" right-click menu option or from the menu in Visual Studio by selecting File → Open → Folder.
In the Solution Explorer, right-click the CMakeSettings.txt and select "Change CMake Settings". A new file named CMakeSettings.json will be created. Add this line
"variables": [ {"name": "GPERF_PRG", "value": "C:\\path\\to\\gperf.exe"} ],
below this line
"name": "x86-Release",
(This step can be skipped if gperf is in the PATH)
Select "x86-Release" configuration from the project settings dropdown and wait for CMake configuration to complete.
Click CMake → Build All.
Note: It is also possible to build with the "x64-Release" configuration, as long as cmake -G "Visual Studio 15 2017 Win64" is used to build the dependencies. However, the Debug configurations will not work because certain dependencies need to be linked with release version of the C runtime.

In the future, we could make this easier by:

Having the third-party/CMakeLists.txt download gperf in the same way it downloads other Windows executables.
Removing the requirement for Python by including a CMakeLists.txt for libuv, such as the one used by vcpkg.
Making step 2 happen automatically when the CMake config detects it is running within Visual Studio. I think this would also remove the requirement for CMake because Visual Studio bundles it's own version of it.

justinmk · 2018-03-08T19:16:11Z

ci/build.bat

+  :: These are native MinGW builds, but they use the toolchain inside
+  :: MSYS2, this allows using all the dependencies and tools available
+  :: in MSYS2, but we cannot build inside the MSYS2 shell.
+  set CMAKE_GENERATOR="MinGW Makefiles"


should be:

set "CMAKE_GENERATOR=MinGW Makefiles"

Fixed in #8117

justinmk · 2018-03-08T19:26:40Z

Let's continue further work in other PR(s), this one is too big. Thanks @b-r-o-c-k !

justinmk · 2018-03-08T19:37:30Z

todo / future reference

MSVC builds are failing with errors like this (probably libintl-related as mentioned):

Vim(language):E319: The command is not available in this version: lang C
...
Vim(language):E319: The command is not available in this version: lang mess C

Also noticed gzip: Xtestfile.gz: Device or resource busy.
HN thread with info on generating PDB from mingw-w64 clang-generated executables.
Also use PATCH_COMMAND for libtermkey win: tui: libtermkey fork #6659
revisit d7ee323
revert b3d0241 after Upgrade wget.exe application luarocks/luarocks#713

CuteLasty · 2018-05-07T22:19:25Z

I meet a build error when build .deps, please add --ignore-space-change to git appy patch in neovim\.deps\libuv.vcxproj

FEATURES: 3cc7ebf #7234 built-in VimL expression parser 6a7c904 #4419 implement <Cmd> key to invoke command in any mode b836328 #7679 'startup: treat stdin as text instead of commands' 58b210e :digraphs : highlight with hl-SpecialKey #2690 7a13611 #8276 'startup: Let `-s -` read from stdin' 1e71978 events: VimSuspend, VimResume #8280 1e7d5e8 #6272 'stdpath()' f96d99a #8247 server: introduce --listen e8c39f7 #8226 insert-mode: interpret unmapped META as ESC 98e7112 msg: do not scroll entire screen (#8088) f72630b #8055 let negative 'writedelay' show all redraws 5d2dd2e win: has("wsl") on Windows Subsystem for Linux #7330 a4f6cec cmdline: CmdlineEnter and CmdlineLeave autocommands (#7422) 207b7ca #6844 channels: support buffered output and bytes sockets/stdio API: f85cbea #7917 API: buffer updates 418abfc #6743 API: list information about all channels/jobs. 36b2e3f #8375 API: nvim_get_commands 273d2cd #8329 API: Make nvim_set_option() update `:verbose set …` 8d40b36 #8371 API: more reliable/descriptive VimL errors ebb1acb #8353 API: nvim_call_dict_function 9f994bb #8004 API: nvim_list_uis 3405704 #7520 API/UI: forward option updates to UIs 911b1e4 #7821 API: improve nvim_command_output WINDOWS OS: 9cefd83 #8084, #8516 build/win: support MSVC ee4e1fd win: Fix reading content from stdin (#8267) TUI: ffb8904 #8309 TUI: add support for mouse release events in urxvt 8d5a46e #8081 TUI: implement "standout" attribute 6071637 TUI: support TERM=konsole-256color 67848c0 #7653 TUI: report TUI info with -V3 ('verbose' >= 3) 3d0ee17 TUI/rxvt: enable focus-reporting d109f56 #7640 TUI: 'term' option: reflect effective terminal behavior FIXES: ed6a113 #8273 'job-control: avoid kill-timer race' 4e02f1a #8107 'jobs: separate process-group' 451c48a terminal: flush vterm output buffer on pty output #8486 5d6732f :checkhealth fixes #8335 53f11dc #8218 'Fix errors reported by PVS' d05712f inccommand: pause :terminal redraws (#8307) 51af911 inccommand: do not execute trailing commands #8256 84359a4 terminal: resize to the max dimensions (#8249) d49c1dd #8228 Make vim_fgets() return the same values as in Vim 60e96a4 screen: winhl=Normal:Background should not override syntax (#8093) 0c59ac1 #5908 'shada: Also save numbered marks' ba87a2c cscope: ignore EINTR while reading the prompt (#8079) b1412dc #7971 ':terminal Enter/Leave should not increment jumplist' 3a5721e TUI: libtermkey: force CSI driver for mouse input #7948 6ff13d7 #7720 TUI: faster startup 1c6e956 #7862 TUI: fix resize-related segfaults a58c909 #7676 TUI: always hide cursor when flushing, never flush buffers during unibilium output 303e1df #7624 TUI: disable BCE almost always 249bdb0 #7761 mark: Make sure that jumplist item will not have zero lnum 6f41ce0 #7704 macOS: Set $LANG based on the system locale a043899 #7633 'Retry fgets on EINTR' CHANGES: ad60927 #8304 default to 'nofsync' f3f1970 #8035 defaults: 'fillchars' a6052c7 #7984 defaults: sidescroll=1 b69fa86 #7888 defaults: enable cscopeverbose 7c4bb23 defaults: do :filetype stuff unless explicitly "off" 2aa308c #5658 'Apply :lmap in macros' 8ce6393 terminal: Leave 'relativenumber' alone (#8360) e46534b #4486 refactor: Remove maxmem, maxmemtot options 131aad9 win: defaults: 'shellcmdflag', 'shellxquote' #7343 c57d315 #8031 jobwait(): return -2 on interrupt also with timeout 6452831 clipboard: macOS: fallback to tmux if pbcopy is broken #7940 300d365 #7919 Make 'langnoremap' apply directly after a map ada1956 #7880 'lua/executor: Remove lightuserdata' INTERNAL: de0a954 #7806 internal statistics for list impl dee78a4 #7708 rewrite internal list impl

oni-link · 2018-06-13T14:11:09Z

src/nvim/option.c

@@ -2436,6 +2436,11 @@ static bool valid_filetype(char_u *val)
  return true;
 }

+#ifdef _MSC_VER
+// MSVC optimizations are disabled for this function because it
+// incorrectly generates an empty string for SHM_ALL.


The optimizer probably uses an empty string because the old pointer became invalid.

Problem seems to be that the use of SHM_ALL creates a local array that is assigned to pointer p (line 3201), but after leaving the block scope, p is used to access the (destroyed) array (line 3213).

This was added in fa6f892 (via neovim#8084). This is meant to check if it is still required, and should the be moved upstream if so.

b-r-o-c-k added 12 commits February 28, 2018 19:37

build/msvc: Fix preprocessor parsing

69da692

Preprocessor directives on the first line of the file were not being parsed.

build/msvc: Add mode_t typedef to win_defs.h

d520e25

build/msvc: Fix name of import library

d99f8fe

build/msvc: Move include into unix_defs.h

f04b53a

build/msvc: Fix missing restrict keyword

2c414fb

MSVC has the __restrict keyword and a marco is defined for it in `win_defs.h`.

build/msvc: Remove unnecessary header include

8f39e96

build/msvc: Allow building Unibilium on Windows

9aec64c

build/msvc: Fix linking for luajit and luv

0169872

build/msvc: Remove unnecessary copy command for msgpack DLL

3a6ee88

build/msvc: Add MSVC compiler flag when building LuaRocks

cb245a7

build/msvc: Use patched libvterm for MSVC

f2b6145

marvim added the RFC label Mar 1, 2018

jamessan reviewed Mar 1, 2018

View reviewed changes

b-r-o-c-k added 5 commits March 1, 2018 22:48

build/msvc: Add CMakeLists.txt and header for Unibilium

fa6f892

A header was added for compatibility with MSVC and CMakeLists.txt was added for building with CMake.

build/msvc: Add patch for libvterm

305673e

The patch removes VLAs because MSVC does not support them.

build/msvc: Add patch for LuaRocks

b3d0241

LuaRocks bundles an outdated wget.exe for downloading packages on Windows. It is too old to support GitHub's TLS, so this patch will replace it with curl.

build/msvc: Make libtermkey depend on Unibilium

adfad73

Merge branch 'master' into msvc-compat

de3a833

justinmk added the platform:windows label Mar 4, 2018

justinmk reviewed Mar 4, 2018

View reviewed changes

b-r-o-c-k added 4 commits March 4, 2018 17:44

build/msvc: Remove pkgconfig dependency on Windows

8e30598

build/msvc: Add missing WIN32 macro

01fc02b

MSVC predefines `_WIN32`, but not `WIN32`. Also, some unnecessary includes have been removed.

build/msvc: Make shell-test work

773f23e

MSVC doesn't have unistd.h or usleep() so it was replaced with the Sleep() WinAPI function.

build/msvc: Fix errors caused by compiler intrinsics

d7ee323

build/msvc: Add Git as a requirement for Windows patches

8ba8d72

b-r-o-c-k force-pushed the msvc-compat branch from 0555338 to 8ba8d72 Compare March 7, 2018 02:53

justinmk reviewed Mar 7, 2018

View reviewed changes

justinmk mentioned this pull request Mar 7, 2018

Error when building latest pull of neovim, using MinGW #6617

Closed

equalsraf mentioned this pull request Mar 7, 2018

Windows: improvements #5229

Closed

14 tasks

justinmk mentioned this pull request Mar 7, 2018

win/mingw64 build: mingw64.exe instead of cmd.exe ? #7059

Closed

build/msvc: Fix AppVeyor build script for 'MINGW_64-gcov' configuration

0adf950

jamessan mentioned this pull request Mar 7, 2018

[RFC] open subprocess pipes in overlapped mode on windows #8113

Merged

justinmk reviewed Mar 8, 2018

View reviewed changes

justinmk merged commit 9cefd83 into neovim:master Mar 8, 2018

justinmk removed the RDY label Mar 8, 2018

justinmk mentioned this pull request Mar 8, 2018

build: use Ninja on travis CI; avoid parallel Luarocks bootstrap #8117

Merged

b-r-o-c-k deleted the msvc-compat branch March 11, 2018 02:38

justinmk mentioned this pull request Mar 16, 2018

Neovim(Windows) unsuccessfully suspends when <Ctrl-Z> is pressed #8139

Closed

justinmk added the gsoc community: Google Summer of Code project label Apr 24, 2018

oni-link reviewed Jun 13, 2018

View reviewed changes

blueyed added a commit to blueyed/neovim that referenced this pull request Jul 10, 2019

third-party: remove msvc-compat/unistd.h [skip travis]

e47a8c2

This was added in fa6f892 (via neovim#8084). This is meant to check if it is still required, and should the be moved upstream if so.

blueyed mentioned this pull request Jul 10, 2019

third-party: remove msvc-compat/unistd.h [skip travis] #10465

Merged

blueyed added a commit to blueyed/neovim that referenced this pull request Aug 26, 2019

third-party: remove msvc-compat/unistd.h [skip travis]

c8d1f28

This was added in fa6f892 (via neovim#8084). This is meant to check if it is still required, and should the be moved upstream if so.

Uh oh!

[RDY] Building with VS2017 #8084

[RDY] Building with VS2017 #8084

Uh oh!

Conversation

b-r-o-c-k commented Mar 1, 2018 • edited by justinmk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamessan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamessan commented Mar 1, 2018

Uh oh!

b-r-o-c-k commented Mar 2, 2018

Uh oh!

zhaozg commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk Mar 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented Mar 4, 2018

Uh oh!

Shougo commented Mar 7, 2018

Uh oh!

justinmk commented Mar 7, 2018

Uh oh!

Shougo commented Mar 7, 2018

Uh oh!

justinmk Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janlazo Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b-r-o-c-k commented Mar 8, 2018

Build Instructions for Visual Studio 2017

Uh oh!

justinmk Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented Mar 8, 2018

Uh oh!

justinmk commented Mar 8, 2018

todo / future reference

Uh oh!

CuteLasty commented May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

b-r-o-c-k commented Mar 1, 2018 •

edited by justinmk

Loading

justinmk Mar 4, 2018 •

edited

Loading

justinmk Mar 7, 2018 •

edited

Loading

justinmk Mar 7, 2018 •

edited

Loading

janlazo Mar 7, 2018 •

edited

Loading

justinmk Mar 8, 2018 •

edited

Loading

CuteLasty commented May 7, 2018 •

edited

Loading