logicomacorp / WaveSabre

Official WaveSabre repository
MIT License
246 stars 34 forks source link

CMake build-system #11

Closed kusma closed 5 years ago

kusma commented 5 years ago

This PR adds a CMake build-system that in theory allows us to build using any version of visual studio, and might be helpful in porting to other platforms as well.

So far, only VS2017 has been tested, though. So if someone has some other VS version, it's be greatly appreciated if they could test and let me know how it worked out!

This also results in a slightly smaller executable for PlayerTest than using the pre-existing project files. So that's nice as well :)

It doesn't build the .NET sources, even though there is support in CMake for this. The reason is that this support only works on Windows, and by having a separate solution for the .NET code we might be able to build the code on macOS eventually using msbuild (or xbuild).

I'm also not yet removing the old build system. Let's wait with that until we see that this is working out for everyone first.

yupferris commented 5 years ago

Quick question: is that smaller PlayerTest before or after compression? I'd be happy to lend my new packer for testing that. :)

kusma commented 5 years ago

Before compression. I haven't tested compressing the binaries.

kusma commented 5 years ago

I've marked this as WIP, because there's still ome more work to do before this is ready to merge, I think:

  1. [x] CMake 3.13 is really new. VS2017 only has built-in support for CMake 3.11 (I think), so it would be nice to support that. The problem is that we use target_link_options, which requires 3.13. An alternative seems to be switching to set_property(TARGET ${target} APPEND_STRING PROPERTY LINK_FLAGS " ${flag}") instead, but that needs to be tested.
  2. [x] Documentation of how to build with CMake is entirely lacking
  3. [x] The Appveyor-config should be updated to also build using CMake. The Appveyor image we currently use only provides CMake 3.12, so downgrading the CMake version requirement would be nice for this also. Alternatively we could just install a newer CMake, but that will add some overhead.

Once these three issues are fixed, I think this should be good to go.

kusma commented 5 years ago

@yupferris: OK, I think all of that should be fixed now.

yupferris commented 5 years ago

Finally sitting down with this; sorry for the wait :)

I'm a total cmake n00b and I think I ran into an issue already. Steps I followed:

Output:

$ cmake -S ../ -B ./
-- Building for: Visual Studio 15 2017
-- The C compiler identification is MSVC 19.14.26433.0
-- The CXX compiler identification is MSVC 19.14.26433.0
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.14.26428/bin/Hostx86/x86/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.14.26428/bin/Hostx86/x86/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.14.26428/bin/Hostx86/x86/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.14.26428/bin/Hostx86/x86/cl.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
CMake Error at Vsts/CMakeLists.txt:6 (add_library):
  No SOURCES given to target: build

-- Build files have been written to: C:/msys64/home/ferris/dev/projects/WaveSabre/build

I'm probably missing something quite basic; seems like an opportunity to be a bit more verbose in the docs perhaps :)

yupferris commented 5 years ago

I also tried opening the CMake project directly from VS2017 (via Open->CMake and choosing the CMakeLists.txt file at the repo root) and got the same error in the VS error list/output window.

kusma commented 5 years ago

Sounds like you have a stray build-directory inside "Vsts"... Try deleting that. How did you end up with that directory there? Did you for instance do this from the wrong directory?

kusma commented 5 years ago

We can probably make the VST enumeration code more robust to prevent this, but in principle it sounds like user-error, and not in something I think should need documentation; adding random directories inside a folder that's being crawled through is a bit strange in the first place. Documenting that is a bit like documenting that people should delete random files also...

yupferris commented 5 years ago

Indeed that was the error; I had not cleaned my working dir before checking out your branch and trying this. With a clean dir cmake works as expected and indeed produces good VS files. I may suggest documenting even running cmake -S ../ -B ./ as I did above (or whatever is more preferred for cmake, if anything) as even that wasn't clear to me when running the first time (again, total cmake n00b here, and I suspect other users will be too).

Anyways, so far I've tested the VS2017 config with PlayerTest (which works just fine) and in our intro setup (with the c lib in between upgraded to VS2017; this also works wonderfully and seems to save 436 bytes over the previous config on trashpanda, which is great!).

One thing to note with those projects is that previously, the library projects themselves (eg. WaveSabreCore/WaveSabrePlayerLib) specified which other libraries they linked to; the current configuration expects that the consumers of the libs specify these (eg. PlayerTest now specifies that both WaveSabreCore.lib and Msacm32.lib should be linked, whereas before, it would only specify WaveSabreCore.lib and WaveSabreCore specified that Msacm32.lib should be linked and we'd pick that up transitively). I'm not sure which approach is better here; I don't really have a preference tbh. It seems like the current approach is probably best, albeit a bit less convenient for an end-user perhaps, even though it's nice to be explicit about what your dependencies are, especially for 64k.

I've also tested the plugins; they don't seem to be recognized by Live anymore. They're also quite a bit smaller than the release builds from the previous setup. I haven't dug too far into what's going on here, but my guess is that they're now missing the image resources they had before, and are failing to load because of it. Need to investigate further.

I'd also like to test VS2015 as well, but so far I'm quite pleased with this :) .

yupferris commented 5 years ago

The other thing I should mention is that the premake setup also configured the VST projects with a post-build step (iirc) that would copy the new .dll file to a user's specified VST directory. This is mostly for convenience when developing plugs and is quite helpful (especially for crappy DAW's that don't support more than one VST install directory), so we should probably look into supporting that again too somehow.

yupferris commented 5 years ago

Also, the new solution doesn't include the C# projects. I suppose this is to be expected since those project files aren't handled by cmake, so the generated solution wouldn't know about them either :) .

Perhaps we could split up the C++ and C# solutions so that we have a eg. WaveSabreTools solution for all the converters etc (that comes in the repo), and WaveSabreLibs solution for the C++ components (generated). There aren't any interdependent components between the two sets of projects iirc (though I think the ProjectManager expects the StandAlonePlayer to be built and placed somewhere? Any thoughts @djh0ffman ?)

yupferris commented 5 years ago

Indeed the premake config used to pull in a resource file from the Data dir (see https://github.com/logicomacorp/WaveSabre/blob/master/Vsts/premake5.lua#L69 and https://github.com/logicomacorp/WaveSabre/blob/master/Vsts/premake5.lua#L79).

Also I think it would make sure to include .def files to ensure the proper entry point was exported (see https://github.com/logicomacorp/WaveSabre/blob/master/Vsts/premake5.lua#L32-L39, https://github.com/logicomacorp/WaveSabre/blob/master/Vsts/premake5.lua#L63-L65 and https://github.com/logicomacorp/WaveSabre/blob/master/Vsts/premake5.lua#L68). I can see in dependency walker that VSTPluginMain for example is no longer being exported.

kusma commented 5 years ago

Indeed that was the error; I had not cleaned my working dir before checking out your branch and trying this. With a clean dir cmake works as expected and indeed produces good VS files. I may suggest documenting even running cmake -S ../ -B ./ as I did above (or whatever is more preferred for cmake, if anything) as even that wasn't clear to me when running the first time (again, total cmake n00b here, and I suspect other users will be too).

Good to hear. I wasn't even aware of the -S / -B flags to CMake, but yeah... I think documenting cmake -B build might be the cleanest option :)

FWIW, I'm also pretty much a cmake n00b ;)

Anyways, so far I've tested the VS2017 config with PlayerTest (which works just fine) and in our intro setup (with the c lib in between upgraded to VS2017; this also works wonderfully and seems to save 436 bytes over the previous config on trashpanda, which is great!).

Awesome :)

One thing to note with those projects is that previously, the library projects themselves (eg. WaveSabreCore/WaveSabrePlayerLib) specified which other libraries they linked to; the current configuration expects that the consumers of the libs specify these (eg. PlayerTest now specifies that both WaveSabreCore.lib and Msacm32.lib should be linked, whereas before, it would only specify WaveSabreCore.lib and WaveSabreCore specified that Msacm32.lib should be linked and we'd pick that up transitively). I'm not sure which approach is better here; I don't really have a preference tbh. It seems like the current approach is probably best, albeit a bit less convenient for an end-user perhaps, even though it's nice to be explicit about what your dependencies are, especially for 64k.

I'm not entirely sure I understand what you mean here... There's no concept of transient dependencies for static library files in themselves. The transient-ness we get here is entirely internal to cmake AFAIK. I guess what you're talking about is a similar mechanism in MSVC? If so, I doubt that this really matters, no? That list gets generated by cmake, and the end result should be the same...

I've also tested the plugins; they don't seem to be recognized by Live anymore. They're also quite a bit smaller than the release builds from the previous setup. I haven't dug too far into what's going on here, but my guess is that they're now missing the image resources they had before, and are failing to load because of it. Need to investigate further.

Yeah, sounds likely. I'll take a look!

I'd also like to test VS2015 as well, but so far I'm quite pleased with this :) .

Cool :)

kusma commented 5 years ago

The other thing I should mention is that the premake setup also configured the VST projects with a post-build step (iirc) that would copy the new .dll file to a user's specified VST directory. This is mostly for convenience when developing plugs and is quite helpful (especially for crappy DAW's that don't support more than one VST install directory), so we should probably look into supporting that again too somehow.

Yeah, sounds like a good idea.

Also, the new solution doesn't include the C# projects. I suppose this is to be expected since those project files aren't handled by cmake, so the generated solution wouldn't know about them either :) .

Yeah, exactly.

Perhaps we could split up the C++ and C# solutions so that we have a eg. WaveSabreTools solution for all the converters etc (that comes in the repo), and WaveSabreLibs solution for the C++ components (generated). There aren't any interdependent components between the two sets of projects iirc (though I think the ProjectManager expects the StandAlonePlayer to be built and placed somewhere? Any thoughts @djh0ffman ?)

I already have patches for something like this in my remove-premake branch.

kusma commented 5 years ago

Indeed the premake config used to pull in a resource file from the Data dir (see https://github.com/logicomacorp/WaveSabre/blob/master/Vsts/premake5.lua#L69 and https://github.com/logicomacorp/WaveSabre/blob/master/Vsts/premake5.lua#L79).

Also I think it would make sure to include .def files to ensure the proper entry point was exported (see https://github.com/logicomacorp/WaveSabre/blob/master/Vsts/premake5.lua#L32-L39, https://github.com/logicomacorp/WaveSabre/blob/master/Vsts/premake5.lua#L63-L65 and https://github.com/logicomacorp/WaveSabre/blob/master/Vsts/premake5.lua#L68). I can see in dependency walker that VSTPluginMain for example is no longer being exported.

Thanks for digging!

Yeah, I guess the lack of VSTPluginMain is the reason this won't load at all. And the rc-stuff needs to be fixed as well! I'll take a look!

yupferris commented 5 years ago

All of that sounds good, as do the patches on your remove-premake branch :)

yupferris commented 5 years ago

Oh yeah, and about this:

I'm not entirely sure I understand what you mean here... There's no concept of transient dependencies for static library files in themselves. The transient-ness we get here is entirely internal to cmake AFAIK. I guess what you're talking about is a similar mechanism in MSVC? If so, I doubt that this really matters, no? That list gets generated by cmake, and the end result should be the same...

Yeah I'm talking about MSVC's lib dependencies. The only place it really matters is when specifying dependencies for the final binary, and since everything in the WS repo is handled correctly I think that's fine. A user (who may not use cmake) might still have to specify the dependencies by hand (which is what I end up doing for our intros), but again, they can look at what PlayerTest does and just do the same. So I'm happy with this :)

kusma commented 5 years ago

@yupferris: OK, I think I fixed what you pointed out, minus the auto-installation feature.

yupferris commented 5 years ago

Indeed these plugs appear to work fab!

Is the auto-install feature something you could look into as well?

kusma commented 5 years ago

Yeah, totally. I just haven't had the time to look at it yet!

djh0ffman commented 5 years ago

Also, the new solution doesn't include the C# projects. I suppose this is to be expected since those project files aren't handled by cmake, so the generated solution wouldn't know about them either :) .

Perhaps we could split up the C++ and C# solutions so that we have a eg. WaveSabreTools solution for all the converters etc (that comes in the repo), and WaveSabreLibs solution for the C++ components (generated). There aren't any interdependent components between the two sets of projects iirc (though I think the ProjectManager expects the StandAlonePlayer to be built and placed somewhere? Any thoughts @djh0ffman ?)

Shouldn't be a problem I don't think. I just copy the StandAlonePlayer exe into the ProjectManager folder whenever we end up with a new version, which isn't that often.

kusma commented 5 years ago

@yupferris: Done.

yupferris commented 5 years ago

Absolutely beautiful, works perfectly! I'm gonna go ahead and merge this and make this my default workflow from now on 👍