Improve cmake build for MSVC #1191

sphaerophoria · 2018-09-20T08:07:08Z

Current cmake files don't handle MSVC builds very well. They don't support toxav and only support libs being in a specific location. I've adjusted them to build on MSVC for me a lot better.

Setup a root prefix semi like linux would (include/ lib/ bin/). Copy pthread libs to lib and headers to include.
Download latest libsodium and extract it
build libvpx and libopus, installing them to the same directory we put pthreads in
`cmake .. -DCMAKE_INSTALL_PREFIX="<dir with pthreads/opus/vpx>" -DCMAKE_PREFIX_PATH="" -G"Visual Studio 15 2017 Win64"
cmake --build . --target install --config Release

After this I was able to build/link qtox and have an audio/video call with a friend on linux using the toxlib I built.

This change is

CLAassistant · 2018-09-20T08:07:15Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

iphydf

Reviewed 7 of 7 files at r1.
Reviewable status: 0 of 1 approvals obtained

sphaerophoria · 2018-09-20T16:27:22Z

As mentioned in the PR, the flow for setting up dependencies is a little different. As a result the CI build for MSVC failed. This is an expected result of the changeset.

iphydf · 2018-09-20T17:40:57Z

@sphaerophoria can you update the appveyor instructions to set up the dependencies in the new way?

nurupo · 2018-09-20T19:14:45Z

Since you have changed the way dependencies are being searched, I suggest you enable all of the Travis-CI builds to make sure all of them succeed. We have macOS build, FreeBSD build, Windows cross-compilation build, etc. It's quite possible that changing how dependencies are searched could have broken some of them. Just make a separate commit removing all of if: type IN (push, api, cron) lines in .travis.yml, push it, wait for Travis-CI to build, fix any failed builds, then once all the builds pass link the Travis build URL in a comment so that we could see them passing and remove that commit that modified .travis.yml.

sphaerophoria · 2018-09-21T02:05:28Z

Thanks for the feedback, will do, wouldn't want to break other builds!

codecov · 2018-09-21T02:38:16Z

Codecov Report

Merging #1191 into master will increase coverage by <.1%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #1191     +/-   ##
========================================
+ Coverage    82.7%   82.8%   +<.1%     
========================================
  Files          82      82             
  Lines       14646   14646             
========================================
+ Hits        12122   12132     +10     
+ Misses       2524    2514     -10

Impacted Files	Coverage Δ
toxav/msi.c	`64.8% <ø> (+0.8%)`	⬆️
toxcore/net_crypto.c	`93.3% <0%> (-0.2%)`	⬇️
toxav/toxav.c	`67.1% <0%> (-0.2%)`	⬇️
toxcore/Messenger.c	`86.6% <0%> (-0.1%)`	⬇️
toxcore/onion_client.c	`96.1% <0%> (+0.3%)`	⬆️
toxcore/group.c	`76.4% <0%> (+0.3%)`	⬆️
toxcore/LAN_discovery.c	`84.9% <0%> (+0.9%)`	⬆️
auto_tests/toxav_basic_test.c	`83.5% <0%> (+1.9%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc0b2e7...f87f871. Read the comment docs.

nurupo · 2018-09-21T02:57:37Z

The Windows cross-compilation builds are failing to find libpthread.

The library is located at

$ find /usr/i686-w64-mingw32 -name '*pthread*'
/usr/i686-w64-mingw32/include/pthread.h
/usr/i686-w64-mingw32/include/pthread_unistd.h
/usr/i686-w64-mingw32/include/pthread_signal.h
/usr/i686-w64-mingw32/include/pthread_time.h
/usr/i686-w64-mingw32/include/pthread_compat.h
/usr/i686-w64-mingw32/lib/libpthread.dll.a
/usr/i686-w64-mingw32/lib/libpthread.a
/usr/i686-w64-mingw32/lib/libwinpthread.dll.a
/usr/i686-w64-mingw32/lib/libwinpthread.a
/usr/i686-w64-mingw32/lib/libwinpthread-1.dll
$ find /usr/x86_64-w64-mingw32 -name '*pthread*'
/usr/x86_64-w64-mingw32/include/pthread.h
/usr/x86_64-w64-mingw32/include/pthread_unistd.h
/usr/x86_64-w64-mingw32/include/pthread_signal.h
/usr/x86_64-w64-mingw32/include/pthread_time.h
/usr/x86_64-w64-mingw32/include/pthread_compat.h
/usr/x86_64-w64-mingw32/lib/libpthread.dll.a
/usr/x86_64-w64-mingw32/lib/libpthread.a
/usr/x86_64-w64-mingw32/lib/libwinpthread.dll.a
/usr/x86_64-w64-mingw32/lib/libwinpthread.a
/usr/x86_64-w64-mingw32/lib/libwinpthread-1.dll

The build scripts tell pkg-config and CMake's own find utilities to look into /usr/i686-w64-mingw32 and /usr/x86_64-w64-mingw32 by setting PKG_CONFIG_PATH env variable and CMAKE_FIND_ROOT_PATH CMake variable. Here is an example for i686:

export PKG_CONFIG_PATH=/root/prefix/i686/lib/pkgconfig:/root/extra-prefix/i686/lib/pkgconfig
echo "
        SET(CMAKE_SYSTEM_NAME Windows)

        SET(CMAKE_C_COMPILER   i686-w64-mingw32-gcc)
        SET(CMAKE_CXX_COMPILER i686-w64-mingw32-g++)
        SET(CMAKE_RC_COMPILER  i686-w64-mingw32-windres)

        SET(CMAKE_FIND_ROOT_PATH /usr/i686-w64-mingw32 /root/prefix/i686 /root/extra-prefix/i686)
" > windows_toolchain.cmake
cmake -DCMAKE_TOOLCHAIN_FILE=windows_toolchain.cmake [...]

nurupo · 2018-09-21T03:00:19Z

The build scripts for this are in other/docker/windows. It's preferrable if the build scripts don't have to be modified, since it looks like we are properly telling pkg-config and CMake where to look for the libs.

sphaerophoria · 2018-09-21T03:03:20Z

@nurupo I've replaced the Threads::Threads module with one that I thought was equivalent and also handles win32 pthreads correctly. It's quite possible that I accidentally overrode the mingw version as they're both win32 but are handled differently.

I'll do my best to avoid touching the build scripts and adapting my changes to avoid changing behavior for non MSVC targets as much as possible.

sphaerophoria · 2018-09-21T04:05:08Z

Alright CIs passed

https://travis-ci.org/TokTok/c-toxcore/builds/431320188?utm_source=github_status&utm_medium=notification
https://ci.appveyor.com/project/iphydf/c-toxcore/build/1.0.4567

I've squashed down the last commit and stripped the travis. Had to tell Findsodium.cmake that I wanted the static variant and only enable pthreads-win32 on msvc.

nurupo · 2018-09-21T04:28:58Z

Would it still work if you have only shared sodium library? libtoxcore is packaged in Debian Sid and the libsodium is a shared library there.

sphaerophoria · 2018-09-21T04:56:44Z

That's a good point, do you happen to know what pkg_use_module did under the hood to prioritize shared vs static linking before? I can change my PR to try one and fallback on the next

sphaerophoria · 2018-09-21T05:55:27Z

I've updated the PR to try to link sodium statically before linking it shared.

sphaerophoria · 2018-09-21T17:06:17Z

Closing/Reopening PR to re-trigger travis since it doesn't look like my change introduced the failure.

iphydf · 2018-09-21T17:36:44Z

I don't think that works. You can push an amended commit, that triggers it.

iphydf · 2018-09-21T17:37:14Z

I've restarted the build.

sphaerophoria · 2018-09-21T17:42:43Z

Thanks, I think the PR is ready to go now.

nurupo · 2018-09-21T18:09:34Z

That's a good point, do you happen to know what pkg_use_module did under the hood to prioritize shared vs static linking before?

It used Cmake's PkgConfig module

c-toxcore/cmake/ModulePackage.cmake

Lines 15 to 17 in c8359c8

    
           if(PKG_CONFIG_FOUND) 
        
             pkg_search_module(${mod} ${pkg}) 
        
           endif()

Here is documentation for it: https://cmake.org/cmake/help/v3.0/module/FindPkgConfig.html

It describes a common case and a case when you explicitly ask for a static library. We used the common case. I think the common case searches for shared first and then falls back to static. Would need to test pkg-config on my system to know for sure. I could test that if you could wait for ~8 hours.

nurupo · 2018-09-21T18:21:53Z

Btw, your simple_find_package() calls are a bit incosistent, different arguments at the same positions.

cmake/Findsodium.cmake

iphydf · 2018-09-21T20:20:27Z

cmake/Dependencies.cmake

 find_library(NSL_LIBRARIES          nsl          )
 find_library(RT_LIBRARIES           rt           )
 find_library(SOCKET_LIBRARIES       socket       )

-# For toxcore.
-pkg_use_module(LIBSODIUM            libsodium    )
+# for toxcore


For toxcore. to match the For toxav. below.

iphydf · 2018-09-21T20:22:59Z

cmake/Dependencies.cmake

+  PKGCFG_NAME opus
+  LIB_NAMES opus
+  PATH_SUFFIXES opus
+  )


Format closing parentheses consistently.

iphydf · 2018-09-21T20:24:24Z

cmake/Dependencies.cmake

+  PATH_SUFFIXES opus
+  )
+
+simple_find_package(Vpx


Order the arguments consistently.

nurupo · 2018-09-22T01:00:27Z

Heh, pkg-config just tells to link against -lsodium, both if you have only shared libsodium or only static libsodium, it doesn't specify the full filename of the library, the compiler has to figure out it on its own. Does MSVC work that way too?

$ pkg-config --libs libsodium  
-L/root/prefix/i686/lib -lsodium

$ pkg-config --static --libs libsodium  
-L/root/prefix/i686/lib -lsodium

nurupo

Reviewed 5 of 7 files at r1, 2 of 3 files at r2, 1 of 2 files at r4.
Reviewable status: 0 of 1 approvals obtained (waiting on @sphaerophoria and @iphydf)

cmake/Dependencies.cmake, line 27 at r4 (raw file):

# Try to find both static and shared variants of sodium
set(sodium_USE_STATIC_LIBS ON)

Make it look for shared first and then fallback to static, because that's what the previous behavior was.

sphaerophoria · 2018-09-22T01:38:19Z

Heh, pkg-config just tells to link against -lsodium, both if you have only shared libsodium or only static libsodium, it doesn't specify the full filename of the library, the compiler has to figure out it on its own. Does MSVC work that way too?
$ pkg-config --libs libsodium  
-L/root/prefix/i686/lib -lsodium
$ pkg-config --static --libs libsodium  
-L/root/prefix/i686/lib -lsodium

I'm honestly not sure. I'm way more familiar with the linux side of things than windows. I just started hacking away until things built. I read that cmake chooses the .so first with find package and falls back on static i general. Not sure what MSVC does.

I've updated the PR with shared followed by static instead of the other way around to keep compatibility with old builds. I've also changed from if (NOT sodium_FOUND) to if (NOT TARGET sodium), i had trouble in an ubuntu 14.04 build I was trying with qtox which seems to be resolved with that solution

nurupo

Reviewed 1 of 1 files at r5.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @sphaerophoria and @iphydf)

sphaerophoria

Reviewed 5 of 7 files at r1, 2 of 3 files at r2, 1 of 1 files at r5.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @sphaerophoria and @iphydf)

nurupo · 2018-09-24T07:04:02Z

Welp, looks like we still managed to break the Windows cross-compilation builds: https://travis-ci.org/TokTok/c-toxcore/builds/432322743

Scanning dependencies of target random_testing
make[2]: *** No rule to make target 'sodium_LIBRARY_RELEASE-NOTFOUND', needed by 'random_testing.exe'.  Stop.
make[2]: *** Waiting for unfinished jobs....
make[2]: *** No rule to make target 'sodium_LIBRARY_RELEASE-NOTFOUND', needed by 'auto_file_saving_test.exe'.  Stop.
make[2]: *** Waiting for unfinished jobs....

iphydf approved these changes Sep 20, 2018

View reviewed changes

iphydf added this to the v0.2.x milestone Sep 20, 2018

sphaerophoria force-pushed the toxcore-windows branch from 39b82e4 to c1ae5a9 Compare September 21, 2018 02:38

sphaerophoria force-pushed the toxcore-windows branch from c1ae5a9 to b843cd5 Compare September 21, 2018 02:59

sphaerophoria force-pushed the toxcore-windows branch 2 times, most recently from 96c8e01 to 172f679 Compare September 21, 2018 04:05

sphaerophoria force-pushed the toxcore-windows branch from 172f679 to 71dc932 Compare September 21, 2018 05:54

sphaerophoria force-pushed the toxcore-windows branch 2 times, most recently from 602e602 to f4e2065 Compare September 21, 2018 09:42

sphaerophoria closed this Sep 21, 2018

sphaerophoria reopened this Sep 21, 2018

iphydf requested changes Sep 21, 2018

View reviewed changes

cmake/Findsodium.cmake Show resolved Hide resolved

iphydf requested changes Sep 21, 2018

View reviewed changes

sphaerophoria force-pushed the toxcore-windows branch 2 times, most recently from 0b8755a to 3250903 Compare September 21, 2018 21:06

iphydf approved these changes Sep 21, 2018

View reviewed changes

iphydf force-pushed the toxcore-windows branch from 3250903 to 886b5f4 Compare September 21, 2018 21:54

iphydf assigned nurupo Sep 21, 2018

nurupo requested changes Sep 22, 2018

View reviewed changes

sphaerophoria force-pushed the toxcore-windows branch from 886b5f4 to 242217f Compare September 22, 2018 01:38

nurupo approved these changes Sep 22, 2018

View reviewed changes

sphaerophoria commented Sep 22, 2018

View reviewed changes

Improve cmake build for MSVC.

f87f871

iphydf force-pushed the toxcore-windows branch from 242217f to f87f871 Compare September 22, 2018 11:14

iphydf merged commit f87f871 into TokTok:master Sep 22, 2018

nurupo mentioned this pull request Sep 24, 2018

Windows cross-compilation is broken #1201

Closed

robinlinden modified the milestones: v0.2.x, v0.2.8 Oct 7, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve cmake build for MSVC #1191

Improve cmake build for MSVC #1191

sphaerophoria commented Sep 20, 2018 •

edited by iphydf

Loading

CLAassistant commented Sep 20, 2018 •

edited

Loading

iphydf left a comment

sphaerophoria commented Sep 20, 2018

iphydf commented Sep 20, 2018

nurupo commented Sep 20, 2018

sphaerophoria commented Sep 21, 2018

codecov bot commented Sep 21, 2018 •

edited

Loading

nurupo commented Sep 21, 2018 •

edited

Loading

nurupo commented Sep 21, 2018

sphaerophoria commented Sep 21, 2018

sphaerophoria commented Sep 21, 2018

nurupo commented Sep 21, 2018

sphaerophoria commented Sep 21, 2018

sphaerophoria commented Sep 21, 2018 •

edited

Loading

sphaerophoria commented Sep 21, 2018

iphydf commented Sep 21, 2018

iphydf commented Sep 21, 2018

sphaerophoria commented Sep 21, 2018

nurupo commented Sep 21, 2018 •

edited

Loading

nurupo commented Sep 21, 2018

iphydf Sep 21, 2018

sphaerophoria Sep 21, 2018

iphydf Sep 21, 2018

sphaerophoria Sep 21, 2018

iphydf Sep 21, 2018

sphaerophoria Sep 21, 2018

nurupo commented Sep 22, 2018 •

edited

Loading

nurupo left a comment

sphaerophoria commented Sep 22, 2018

nurupo left a comment

sphaerophoria left a comment

nurupo commented Sep 24, 2018

Improve cmake build for MSVC #1191

Improve cmake build for MSVC #1191

Conversation

sphaerophoria commented Sep 20, 2018 • edited by iphydf Loading

CLAassistant commented Sep 20, 2018 • edited Loading

iphydf left a comment

Choose a reason for hiding this comment

sphaerophoria commented Sep 20, 2018

iphydf commented Sep 20, 2018

nurupo commented Sep 20, 2018

sphaerophoria commented Sep 21, 2018

codecov bot commented Sep 21, 2018 • edited Loading

Codecov Report

nurupo commented Sep 21, 2018 • edited Loading

nurupo commented Sep 21, 2018

sphaerophoria commented Sep 21, 2018

sphaerophoria commented Sep 21, 2018

nurupo commented Sep 21, 2018

sphaerophoria commented Sep 21, 2018

sphaerophoria commented Sep 21, 2018 • edited Loading

sphaerophoria commented Sep 21, 2018

iphydf commented Sep 21, 2018

iphydf commented Sep 21, 2018

sphaerophoria commented Sep 21, 2018

nurupo commented Sep 21, 2018 • edited Loading

nurupo commented Sep 21, 2018

iphydf Sep 21, 2018

Choose a reason for hiding this comment

sphaerophoria Sep 21, 2018

Choose a reason for hiding this comment

iphydf Sep 21, 2018

Choose a reason for hiding this comment

sphaerophoria Sep 21, 2018

Choose a reason for hiding this comment

iphydf Sep 21, 2018

Choose a reason for hiding this comment

sphaerophoria Sep 21, 2018

Choose a reason for hiding this comment

nurupo commented Sep 22, 2018 • edited Loading

nurupo left a comment

Choose a reason for hiding this comment

sphaerophoria commented Sep 22, 2018

nurupo left a comment

Choose a reason for hiding this comment

sphaerophoria left a comment

Choose a reason for hiding this comment

nurupo commented Sep 24, 2018

sphaerophoria commented Sep 20, 2018 •

edited by iphydf

Loading

CLAassistant commented Sep 20, 2018 •

edited

Loading

codecov bot commented Sep 21, 2018 •

edited

Loading

nurupo commented Sep 21, 2018 •

edited

Loading

sphaerophoria commented Sep 21, 2018 •

edited

Loading

nurupo commented Sep 21, 2018 •

edited

Loading

nurupo commented Sep 22, 2018 •

edited

Loading