Skip to content
This repository was archived by the owner on Dec 18, 2022. It is now read-only.

Add CMake modules and build without vendored dependencies using vcpkg#228

Merged
emabrey merged 14 commits intotenacityteam:masterfrom
Be-ing:build_rewrite
Aug 18, 2021
Merged

Add CMake modules and build without vendored dependencies using vcpkg#228
emabrey merged 14 commits intotenacityteam:masterfrom
Be-ing:build_rewrite

Conversation

@Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Jul 12, 2021

Signed-off-by: Be be@mixxx.org

This branch replaces the brittle dependency finding code using Conan with CMake modules that work with both Linux distribution packages and packages from vcpkg. vcpkg is used automatically on Windows and macOS. It can be enabled on Linux by passing -D VCPKG=ON to the CMake configuration command. Refer to the rewritten BUILDING.md document in this branch for details about how to build it.

All vendored dependencies and the hacky CMakeLists.txt in the cmake-proxies directory for building them have been removed except for libnyquist because Tenacity has a function that mixes private symbols from libnyquist with wxString.

  • I have signed off my commits using -s or Signed-off-by* (See: Contributing § DCO)
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code*
  • I made sure the title of the PR reflects the core meaning of the issue you are solving*
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"*

* indicates required

@Be-ing Be-ing marked this pull request as draft July 12, 2021 02:05
@Be-ing Be-ing force-pushed the build_rewrite branch 2 times, most recently from a7437c6 to 2dea5f4 Compare July 12, 2021 04:44
@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 12, 2021

There was a FindwxWidgets.cmake module in there that was interfering with find_package. The one that comes with CMake works, however I had to uninstall the Fedora packages for wxWidgets 3 because it was preferring 3 to 3.1.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 12, 2021

src/KeyboardCapture.cpp has a #include <gtk/gtk.h>... what version of GTK does it depend on?

@Be-ing Be-ing force-pushed the build_rewrite branch 2 times, most recently from a352f9e to dfc535b Compare July 12, 2021 05:18
@AnotherFoxGuy
Copy link
Contributor

But how would this work on windows? Are you required to build with MSYS2/Cygwin/MinGW or need to use vcpkg?

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 12, 2021

Use vcpkg for Windows and macOS.

@sykhro
Copy link

sykhro commented Jul 12, 2021

not strictly related to this pr, what does tenacity need glib for? It's the heck of a dependency which seems a bit odd for a c++ project

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 12, 2021

Good question... also, why is Tenacity directly using GTK APIs if it uses wxWidgets??

@falkTX
Copy link
Contributor

falkTX commented Jul 12, 2021

audacity uses stuff beyond wxwidgets for some native platform support.
you can find the platform-dependent macros everywhere throughout the code.

For gtk (and win32 and macOS), audacity has a custom file dialog which uses native calls to set up file filters, among other things. There are other places where workarounds are in place, or custom OS functionality.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 12, 2021

😱

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 12, 2021

@falkTX do you know if the GTK APIs are GTK2 or 3?

@falkTX
Copy link
Contributor

falkTX commented Jul 12, 2021

afaik it depends on the WxWidgets being in use. some time ago they made it be gtk3 compatible.
at least the version as packaged in Ubuntu 20.04 uses gtk3.

@sykhro
Copy link

sykhro commented Jul 12, 2021

audacity has a custom file dialog which uses native calls to set up file filters

So they're using the whole glib for something that can be easily done with native APIs? It's a couple of ifdefs at best

@falkTX
Copy link
Contributor

falkTX commented Jul 12, 2021

Gtk uses glib, so on linux you cant escape that dependency.

@sykhro
Copy link

sykhro commented Jul 12, 2021

Gtk uses glib, so on linux you cant escape that dependency.

escaping it on windows and macos wouldn't be a bad idea. it's especially painful to compile it on windows. besides, on linux you just need to link to gtk

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 12, 2021

It compiles! Now to get it to link...

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 13, 2021

Woooo it builds on my machine with Fedora 34! I copied the FindGTK.cmake module from WebKitGTK and wrote little CMakeLists.txt's for portsmf which has no Fedora package and libnyquist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WTAF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#including private headers of your vendored dependencies is a totally normal thing to do right???? 🙃

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 13, 2021

The UI scaling is all borked on my 3840 x 2160 screen, but hey, it's something!
image

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 13, 2021

Please test building this. There is definitely more work to do to complete this and clean it up, but at least this is a proof of concept that this is a viable route forward.

@nbsp
Copy link

nbsp commented Jul 13, 2021

Tested this PR on an Arch build, and I've managed to successfully build Tenacity without Conan. 🎉

(build log, for anyone interested)

One minor caveat, though: the Arch repositories have an outdated version of wxgtk, so I had to build it from source; this led to a build time of 36 minutes.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 13, 2021

(build log, for anyone interested)

That was not built, only configured.

Be-ing and others added 3 commits August 16, 2021 13:22
Add `CXX_WARNINGS_SILENCE_DEFINES` variable.

Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>
Signed-off-by: Be <be@mixxx.org>
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 16, 2021

Could you also add the space requirements to BUILDING.md? you need at least 10GB on windows to build

Done.

Signed-off-by: Be <be@mixxx.org>
Remove linking option containing erroneous reference to `z.lib`.
Add `set` commands to provide documented `id3tag::id3tag` vars.

Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>
Signed-off-by: Be <be@mixxx.org>
Co-authored-by: Emily Mabrey <emabrey@tenacityaudio.org>
Co-authored-by: Be <be@mixxx.org>
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 18, 2021

Thanks to everyone who tested and reviewed, especially @emabrey! And thanks to @Holzhaus for originally writing many of the CMake find modules for Mixxx and introducing me to vcpkg.

@caughtquick
Copy link
Member

@Be-ing this appear to create a large increase in binary size, any idea why?

@AnotherFoxGuy
Copy link
Contributor

AnotherFoxGuy commented Aug 30, 2021

this appear to create a large increase in binary size, any idea why?

The executable size has increased after this PR from 22MB to 361MB on Ubuntu

@leio
Copy link
Member

leio commented Aug 30, 2021

Nothing is wrong with this PR in this regard, other than maybe the CI rules. The build defaults to CMAKE_BUILD_TYPE=Debug, and the CI workflows seem to set CMAKE_BUILD_TYPE=RelWithDebInfo. This ending up in a 300MB+ tenacity binary is expected when the debug information doesn't get split out into a separate -dbg or -debug package, which it probably isn't.

The cmake build system looks great to me, but the CI build rules may need some tweaking, either going to Release build type or doing the plumbing to split out debuginfo into a separate package like proper distro packages would do.

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 1, 2021

I disagree. I think we should keep using RelWithDebInfo on CI. It would be very hard to get backtraces from Windows and macOS users without debug info in our builds. Distro packages can set the CMake build mode themselves according to their policies.

@RJVB
Copy link

RJVB commented Oct 7, 2021

Quick question: is vcpkg (or your CMake logic calling it) clever enough to detect when dependencies are available? Audacity has logic to force the use of system-provided versions of its "vendored dependencies", it looks like you didn't keep that?

Context: I'm the maintainer of the Audacity port in MacPorts, and wondering if I'm going to have to spend a lot of time (and not-yet grey hairs ;)) trying to get your build system to behave "the MacPorts way", or rather spend that time finding and patching out the bad bits from the original code...
(Audacity's move to CMake made port maintenance a lot easier for me, in part because my feedback was heeded.)

@AnotherFoxGuy
Copy link
Contributor

Quick question: is vcpkg (or your CMake logic calling it) clever enough to detect when dependencies are available?

Nope, it isn't. There are only two ways to build, using only system packages or using only vcpkg

@Be-ing
Copy link
Contributor Author

Be-ing commented Oct 7, 2021

Yes there is an option for that: https://vcpkg.io/en/docs/users/manifests.html#vcpkg_prefer_system_libs

But I don't know why you would use vcpkg in a package for another package manager. vcpkg isn't even usable within most package managers because it requires a network connection. Downloads can be done ahead of time, but that's not really how it's intended to be used.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Configuring Tenacity with CMake failed due to either pthread or conan How-to build without conan is not specified Mirror conan packages