search

Ableton / link

Ableton Link
Other
1.06k stars 145 forks source link

Allow 32- and 64-bit builds on Windows, cure VS2019 build problems #98

Closed Arakula closed 3 years ago

Arakula commented 3 years ago

The first is just a little management issue; I added /build32/ and /build64/ to the .gitignore list and documented how to use them to builds for 32- and 64-bit on Windows using Visual Studio.

The second is a bit more severe. Personally, I think that the "/WX" compiler flag causes more harm than good, but since it's there, it should WORK ... which it didn't. With the latest VS2019 update, at least, warning C5220 crept in, and with the current Windows Platform SDK, warning C4668 appears. Both prevent successful compilation with /WX, so I added them to the Ignore list.

Arakula commented 3 years ago

The "LinkC" thing ... it actually wasn't my intention to add that to the pull request. But as it's there and I don't see how I could remove it, I might as well document it ...

Since I'm in the process of adding Link to pre-C++11 projects, I've started to create a little C wrapper and putting it into a Link DLL that I can load from programs done with older compilers (I use VS2008 and Visual Studio 6 from 1998 - I like to keep backwards compatibility intact).

If you like the idea, keep it. If not, please ignore it.

fgo-ableton commented 3 years ago

Thanks for opening the PR!

Regarding LinkC: Thanks for putting in the work! To add this to the repo we would need good test coverage. Especially because we don't use the API ourselves. Unfortunately it is quite complicated to add integration tests to the Link code as it is. So I'd rather not merge this here. However, if you want to put the code in a public repo, I'd be happy to refer to it here for others to find.

Regarding the VS Warnings: I makes sense to silence them. The latest version of VS is only available as a preview on AppVeyor currently. That is why we didn’t notice them on CI yet. However, the errors are thrown in Asio and Catch and not in Link itself. So they should be disabled in CatchWrapper.hpp (4668) and AsioWrapper.hpp (4668 & 5220).

Regarding the build instructions: I’d rather not add the addition to the readme. I think this is more general CMake documentation. Building for different architectures seems to be a very specific requirement. However, if you see benefit of adding build32 and build64 to gitignore rather than having it in your global one (maybe because of some convention I am not aware of) let’s do that.

Let me know if you want to update the PR. If so, please also send us a (CLA)[https://ableton.github.io/cla/]. Otherwise I could also disable the warnings for now.

Arakula commented 3 years ago

As said, I didn't intend to add the LinkC stuff ... as a casual user, I have no idea how I could prevent GitHub's annoying "feature" of adding follow-up commits I'm sending to my Link fork to this pull request :-)

I'm happy if the VS19 compile problem has been brought to your attention and is going to be fixed in an upcoming commit. There's no need to incorporate my changes 1:1, so if you feel like it, just close this PR.

fgo-ableton commented 3 years ago

Alright. Thanks a lot! I just silenced the VS warnings on master: https://github.com/Ableton/link/commit/63353f54330874bd420d6e366d9f38095a30aa41