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.

Sign up for GitHub

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

Jump to bottom

Cmake build on Windows #1849

Merged
merged 7 commits into from
Apr 25, 2019
Merged

Cmake build on Windows #1849

merged 7 commits into from
Apr 25, 2019

Conversation

peterychang
Copy link
Collaborator

@peterychang peterychang commented Apr 17, 2019 •
edited
Loading

Dependencies managed through vcpkg
Required vcpkgs

cpprestsdk:x64-windows
zlib:x64-windows
boost-align:x64-windows
boost-system:x64-windows
boost-program-options:x64-windows
boost-test:x64-windows
boost-thread:x64-windows
boost-uuid:x64-windows

Unsupported CMake options

PROFILE
VALGRIND_PROFILE
GCOV
STATIC_LINK_VW
BUILD_JAVA
LTO

edit:
Python build now supported!
A manual hack for python 2.7 is required on vcpkg because boost-python builds on python3 by default with no programatic way to change the version

Verified on Windows against python 2.7, 3.7

peterychang and others added 2 commits April 17, 2019 11:17
fixing cmake file, adding cmake settings json
9d6fc4e 
add build helper script

extend windows build script to use vcpkg

Adding check for invalid windows options
@peterychang
Update README.Windows.md
dad3f8d
@JohnLangford
Copy link
Member

This pull request introduces 1 alert when merging 28a9e65 into bed5b80 - view on LGTM.com

new alerts:

  • 1 for Unused import

Comment posted by LGTM.com

@peterychang
Copy link
Collaborator Author

investigating the failed CI, looks like the python 2.7 windows build failed for some reason

Python module building on Windows using setup.py via cmake/vcpkg
ee4c06d 
updating docs, making vcpkg-root option optional and Windows-only

clean up unused import

Current python build on Windows links against boost statically
@peterychang
Copy link
Collaborator Author

There is one difference between the outputs of the cmake and sln builds for Windows python.
The sln currently links some (all?) libraries statically while the cmake file links everything dynamically.

The cmake/setup.py installation ensures all required dlls are copied to the appropriate location

lokitoth
lokitoth reviewed Apr 24, 2019
View reviewed changes
Copy link
Member

@lokitoth lokitoth left a comment

Choose a reason for hiding this comment

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

Looks good overall, up to some questions and minor things.

CMakeLists.txt
# VW targets Windows 8.1 SDK
if(WIN32)
set(CMAKE_SYSTEM_VERSION 8.1 CACHE TYPE INTERNAL FORCE)
endif()

project(vowpal_wabbit C CXX)
Copy link
Member

Choose a reason for hiding this comment

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

[Minor] Is there a reason this line needs to be below the Win8.1 SDK targetting setting? If so, consider adding a comment about this.

CMakeLists.txt Outdated
@@ -40,6 +40,10 @@ option(LTO "Enable Link Time optimization (Requires Release build, only works wi

string(TOUPPER "${CMAKE_BUILD_TYPE}" CONFIG)

if(WIN32 AND (PROFILE OR VALGRIND_PROFILE OR GCOV OR STATIC_LINK_VW OR BUILD_JAVA OR LTO))
message(FATAL_ERROR "Unsupported option enabled on Win32 build")
Copy link
Member

Choose a reason for hiding this comment

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

[Minor] Are we actually building in Win32 mode? (X86) If not, consider changing to "on Windows build"

Suggested change
message(FATAL_ERROR "Unsupported option enabled on Win32 build")
message(FATAL_ERROR "Unsupported option enabled on Windows build")

python/pylibvw.cc Outdated
@@ -10,7 +10,7 @@
#include "../vowpalwabbit/options_serializer_boost_po.h"

// see http://www.boost.org/doc/libs/1_56_0/doc/html/bbv2/installation.html
#define BOOST_PYTHON_STATIC_LIB
//#define BOOST_PYTHON_STATIC_LIB
Copy link
Member

Choose a reason for hiding this comment

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

[Minor] Can be removed?

Suggested change
//#define BOOST_PYTHON_STATIC_LIB

build-windows.bat Outdated

cmake %~dp0 -G "Visual Studio 14 2015 Win64" -DCMAKE_TOOLCHAIN_FILE=%VCPKG_PATH%\scripts\buildsystems\vcpkg.cmake
msbuild vowpal_wabbit.sln /p:Configuration=Release
GOTO FINISH
Copy link
Member

Choose a reason for hiding this comment

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

[Minor] Consider using the :EOF automatic label.

Suggested change
GOTO FINISH
GOTO:EOF

CMakeLists.txt
@@ -79,10 +83,6 @@ if(GCOV)
set(linux_flags ${linux_flags} -g -O0 -fprofile-arcs -ftest-coverage -fno-strict-aliasing -pg)
endif()

set(windows_release_config /GL /O2 /Gy /Oi /Ob2 /Ot /Oy /GT /MD)
Copy link
Member

Choose a reason for hiding this comment

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

Do we not need these compiler flags? (I am not sure what they do offhand)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These flags are set in the CMakeSettings.json file. Setting this stuff here was causing problems between the various configs

Copy link
Member

Choose a reason for hiding this comment

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

Does build-windows.bat need to reference this somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems to be automagic CMake functionality

Copy link
Member

Choose a reason for hiding this comment

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

Where is CMakeSettings.json? It isn't in the repo? These flags are required in some form

build-windows.bat Outdated
@@ -0,0 +1,17 @@
@echo off
Copy link
Member

Choose a reason for hiding this comment

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

Should this file be under python/? Or alternatively, call it build-windows-experimental.bat?

(This does not seem like it will build the .NET bindings, and that is one of the major Windows targets)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

build-windows-experimental.bat doesn't actually build the python bindings, just the main library (with associated dll/exes)

@peterychang peterychang merged commit 822158d into VowpalWabbit:master Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackgerrits jackgerrits jackgerrits left review comments

@lokitoth lokitoth lokitoth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@peterychang @JohnLangford @lokitoth @jackgerrits