Msvc/vst-rebase : msvc support #4352

ghost · 2018-05-11T13:58:38Z

Here's some proposed changes to compile using MSVC. I think using vcpkg would be ideal since it has built in support for cmake (find_package where supported). And as an added bonus it's also works on linux (although I don't see a reason for using it there)

It obviously needs more testing, but this isn't the master branch so I think we're ok.

using vcpkg

install

git clone https://github.com/microsoft/vcpkg.git
cd vcpkg
.\bootstrap-vcpkg.bat

installing libs 4 lmms

.\vcpkg.exe install fftw3 libsndfile libsamplerate qt5

cmake usage

cmake "-DCMAKE_TOOLCHAIN_FILE=<vcpkgdir>/scripts/buildsystems/vcpkg.cmake" <lmms src dir>

+locale: using path instead of individual files to reduce command line size +remotevstplugin: changed order return type & calling convention (compiler error) +lmmsobj: removed single quotes for command line defines

+added toolchain file +added toolset

lukas-w

There are two changes left to do to fix Linux compilation:

In plugins/carlabase/carla.h, a #include "plugin_export.h" is missing
In cmake/linux/package_linux.sh.in, RemoteVstPlugin.exe.so needs to be changed RemoteVstPlugin32.exe.so

Could you add these to your PR?

lukas-w · 2018-05-12T06:25:16Z

plugins/vst_base/CMakeLists.txt

@@ -27,33 +27,27 @@ SET(EXTERNALPROJECT_CMAKE_ARGS
 	"-DCMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG=${CMAKE_RUNTIME_OUTPUT_DIRECTORY}"
 	"-DCMAKE_MODULE_PATH=${CMAKE_MODULE_PATH}"
 	"-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}"
+	"-DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE}"


This needs to be moved to the MSVC-specific part. It conflicts with the toolchain file passed in line 70.

lukas-w · 2018-05-12T06:27:39Z

src/core/audio/AudioSdl.cpp

@@ -64,14 +64,22 @@ AudioSdl::AudioSdl( bool & _success_ful, Mixer*  _mixer ) :
 	m_audioHandle.callback = sdlAudioCallback;
 	m_audioHandle.userdata = this;

-  	SDL_AudioSpec actual; 
-
+	SDL_AudioSpec actual; 


FYI, these SDL2 fixes appear to be redundant with those from #3947. I think it's best to drop a414255 from this PR to avoid conflicts.

lukas-w · 2018-05-12T06:29:23Z

CMakeLists.txt

-IF(NOT SNDFILE_FOUND)
-	MESSAGE(FATAL_ERROR "LMMS requires libsndfile1 and libsndfile1-dev >= 1.0.18 - please install, remove CMakeCache.txt and try again!")
-ENDIF()
+IF(USING_VCPKG)


Ideally we'd hide these details in a FindSndFile.cmake script and only call FIND_PACKAGE(SndFile) here, but I guess that's out of scope for this PR so it's fine this way for now.

I agree.

I'm a bit relaxed about coding standards because this is only a proof of concept. If it works, I'll implement those changes. If it doesn't, I'll look for other package managers in windows (scoop?) and see if I can implement those.

+carla: exports header +package_linux: corrected RemoteVstPlugin name +vstbase: toolchain file conditional on MSVC

ghost · 2018-05-12T09:52:32Z

I've added the changes you requested and got most of the tests to pass but I don't know why the travis continuous integration is failing. I can't reproduce that error on my machine.

I removed the install of remotevstplugin64 because appimage was complaining about it. No idea if that's correct or not.

ghost · 2018-05-13T22:12:10Z

It's now possible to create an NSIS installer package using msvc.
If there are no major issues, I suggest we merge. I can keep on adding to this pull request, but if we merge this pr and also the fix for SDL then others should be able to get msvc running pretty quickly.

edit: I just remembered that in the initial install of vcpkg they have to compile qt5. So.. um... let's say they can get it running easily instead of quickly :)

lukas-w · 2018-05-14T16:02:17Z

It's now possible to create an NSIS installer package using msvc.

@justnope Although we're trying to get away from the hard-coded install paths in CMake (see #4345), this is a great addition! Thanks! 👍

Thank you for your hard work on this! It's great to see the Windows platform get some attention. I'll go through the changes again and leave some comments about minor code-style issues.

As far as I can see, RemoteVstPlugin64 and RemoteVstPlugin64.exe.so aren't installed or packaged on Linux yet. We can track this in another PR if you like.

lukas-w · 2018-05-14T16:08:12Z

plugins/vst_base/CMakeLists.txt

 # build 32 bit version of RemoteVstPlugin
 IF(LMMS_BUILD_WIN32 AND NOT LMMS_BUILD_WIN64)
 	ADD_SUBDIRECTORY(RemoteVstPlugin)
+	INSTALL(PROGRAMS "${CMAKE_CURRENT_BINARY_DIR}/../RemoteVstPlugin32.exe" "${CMAKE_CURRENT_BINARY_DIR}/../RemoteVstPlugin32.exe" DESTINATION "${PLUGIN_DIR}")


This has ${CMAKE_CURRENT_BINARY_DIR}/../RemoteVstPlugin32.exe twice.

lukas-w · 2018-05-14T16:08:26Z

plugins/vst_base/CMakeLists.txt

 	"-DCMAKE_MODULE_PATH=${CMAKE_MODULE_PATH}"
 	"-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}"
 )

+IF(MSVC)
+	LIST(APPEND EXTERNALPROJECT_CMAKE_ARGS "-DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE}")


Since this is only used in the ELSEIF(LMMS_BUILD_WIN64 AND MSVC) case, you can just move it down there, so that it reads from line 50:

ExternalProject_Add(RemoteVstPlugin32 "${EXTERNALPROJECT_ARGS}" CMAKE_GENERATOR ${GENERATOR} CMAKE_GENERATOR_TOOLSET ${CMAKE_GENERATOR_TOOLSET} CMAKE_ARGS "${EXTERNALPROJECT_CMAKE_ARGS}" -DCMAKE_TOOLCHAIN_FILE="${CMAKE_TOOLCHAIN_FILE}" "-DCMAKE_PREFIX_PATH=${QT_32_PREFIX}" )

lukas-w · 2018-05-14T16:21:10Z

plugins/vst_base/CMakeLists.txt

 		CMAKE_ARGS
 			"${EXTERNALPROJECT_CMAKE_ARGS}"
 			"-DCMAKE_PREFIX_PATH=${QT_32_PREFIX}"
 	)
+	INSTALL(PROGRAMS "${CMAKE_CURRENT_BINARY_DIR}/../RemoteVstPlugin32.exe" DESTINATION "${PLUGIN_DIR}")


Can we move these redundant INSTALL statements downwards after the ENDIF()? Something like this:

IF(TARGET RemoteVstPlugin32) IF(LMMS_BUILD_LINUX) INSTALL(PROGRAMS "${CMAKE_CURRENT_BINARY_DIR}/../RemoteVstPlugin32" "${CMAKE_CURRENT_BINARY_DIR}/../RemoteVstPlugin32.exe.so" DESTINATION "${PLUGIN_DIR}") ELSEIF(LMMS_BUILD_WIN32) INSTALL(PROGRAMS "${CMAKE_CURRENT_BINARY_DIR}/../RemoteVstPlugin32.exe" DESTINATION "${PLUGIN_DIR}") ENDIF() ENDIF()

ghost · 2018-05-15T15:47:48Z

I've tested msvc 2015 & 2017/32 & 64 bits. It all seems to work after implementing your code review suggestions (thanks btw).

It's definitely not finished as in merge into the master branch, but I think it should be safe enough to merge into msvc/vst-rebase.

TODO:
-Find out why sdl2.dll isn't copied into the build directory
-Create a junction (symbolic link in linux, windows symlinks have a slightly different meaning) in the build directory for the data dir (easier debugging if implemented in cmake).
-fluidsynth is present in vcpkg but not picked up, etc...

lukas-w · 2018-05-20T17:22:15Z

Why close this @justnope? Didn't have time to merge yet.

ghost · 2018-05-20T17:52:39Z

I've made another branch from this branch msvc/ng. Last time I did this the things I pushed in the new branch also ended up in the PR which wasn't the intention.

I might have screwed up the git commands when that happened, but in any case if you see extra commits they are from the new branch.

lukas-w · 2018-05-22T13:43:16Z

@justnope Only commits you push to this branch will end up in the PR. I'll merge shortly. 👍

PhysSong · 2018-05-31T03:55:53Z

include/stdshims.h

@@ -7,8 +7,10 @@
 #include <memory>
 #include <utility>

-#if (__cplusplus >= 201402L)
+#if (__cplusplus >= 201402L || _MSC_VER)


VS 2012(_MSC_VER == 1700) doesn't have std::make_unique while 2013 (_MSC_VER == 1800) has. This will cause compile errors when using VS 2012.

* locale: using path instead of individual files to reduce command line size * remotevstplugin: changed order return type & calling convention (compiler error) * lmmsobj: removed single quotes for command line defines * added vcpkg support & std::make_unique for MSVC * carla: include exports header * package_linux: corrected RemoteVstPlugin name * vstbase: toolchain file conditional on MSVC * Added install for remotevstplugin * msvc: installer works with vcpkg Remotevst 64bit install removed due to an ApImage problem

Azrael added 3 commits May 11, 2018 13:17

MSVC fixes

231a6aa

+locale: using path instead of individual files to reduce command line size +remotevstplugin: changed order return type & calling convention (compiler error) +lmmsobj: removed single quotes for command line defines

added vcpkg support & std::make_unique for MSVC

d21c59b

remotevstplugin

a4a8612

+added toolchain file +added toolset

lukas-w reviewed May 12, 2018

View reviewed changes

Azrael added 4 commits May 12, 2018 09:40

implementing PR comments

b55042f

+carla: exports header +package_linux: corrected RemoteVstPlugin name +vstbase: toolchain file conditional on MSVC

renamed second RemoteVstPlugin

78a7941

Added install for remotevstplugin

8474786

remotevst: -64bit install (appimage problem)

a569bad

Azrael added 2 commits May 13, 2018 05:28

cmake install fixes

493a0e0

msvc: installer works

dab9e04

lukas-w reviewed May 14, 2018

View reviewed changes

ghost mentioned this pull request May 14, 2018

Hard-coded library paths in CMake #4345

Closed

cmake cleanup & corrected system files install dir

42488a6

ghost closed this May 20, 2018

lukas-w reopened this May 20, 2018

lukas-w merged commit d0ca0dc into LMMS:msvc/vst-rebase May 22, 2018

PhysSong reviewed May 31, 2018

View reviewed changes

This was referenced Jun 11, 2018

Fix MSVC VST compilation #4421

Closed

MSVC: Library find modules #4438

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Msvc/vst-rebase : msvc support #4352

Msvc/vst-rebase : msvc support #4352

ghost commented May 11, 2018

lukas-w left a comment

lukas-w May 12, 2018

lukas-w May 12, 2018

lukas-w May 12, 2018

ghost May 12, 2018

ghost commented May 12, 2018

ghost commented May 13, 2018 •

edited by ghost

Loading

lukas-w commented May 14, 2018

lukas-w May 14, 2018

lukas-w May 14, 2018

lukas-w May 14, 2018

ghost commented May 15, 2018

lukas-w commented May 20, 2018

ghost commented May 20, 2018 •

edited by ghost

Loading

lukas-w commented May 22, 2018

PhysSong May 31, 2018

Msvc/vst-rebase : msvc support #4352

Msvc/vst-rebase : msvc support #4352

Conversation

ghost commented May 11, 2018

using vcpkg

install

installing libs 4 lmms

cmake usage

lukas-w left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 12, 2018

ghost commented May 13, 2018 • edited by ghost Loading

lukas-w commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 15, 2018

lukas-w commented May 20, 2018

ghost commented May 20, 2018 • edited by ghost Loading

lukas-w commented May 22, 2018

Choose a reason for hiding this comment

ghost commented May 13, 2018 •

edited by ghost

Loading

ghost commented May 20, 2018 •

edited by ghost

Loading