Add libdjinterop external project dependency #2932

mr-smidge · 2020-07-11T19:10:34Z

This PR adds the libdjinterop library as an external project dependency to the CMake build system for Mixxx. It is a precursor to PR #2753 , which will add library export functionality using libdjinterop.

Notes:

An existing installation of the library, if available, is used in preference. If this is not available, a snapshot is downloaded from the libdjinterop GitHub repo.
The scons build has not been touched, on the assumption that it will be removed soon.
The library should be considered in beta at present. As such, the library is added only on Linux builds for now, following prior discussion with devs in PR Lib export with libdjinterop #2753 .
- The libdjinterop library has been compiled and tested on 64-bit Windows in isolation, but I haven't integrated this in with Mixxx yet.
- MacOS builds will fail with the current build servers because Apple LLVM 9 doesn't support std::optional<T> (only experimental), which is in the public library headers.

mr-smidge · 2020-07-11T19:12:52Z

CMakeLists.txt

+    # libdjinterop depends on zlib and sqlite3.
+    # Mixxx already has a dependency on sqlite3, but not zlib, which we add now.
+    find_package(ZLIB 1.2.8 REQUIRED)


This might be considered slightly fishy: we're linking statically to libdjinterop, but libdjinterop depends on SQLite3 and ZLib. It just so happens that Mixxx already has the SQLite3 dependency, but not ZLib.

Are there any concerns with encoding implicit knowledge of libdjinterop's dependencies here? I'm not aware of a better way with static build.

@Holzhaus Opinions?

I think this should be move to the FindDJInterop module. For the external project build, this can be taken care of in libdjinterop's own CMakeLists.txt.

For the external project build, this can be taken care of in libdjinterop's own CMakeLists.txt.

The CMakeLists.txt file from libdjinterop already has a find_package(ZLIB 1.2.8 REQUIRED) line. However, I was not able to get the dependencies of the external project to transfer through to mixxx-lib (and search engines were not fruitful for me). Do you know of a way in CMake to do this?

Note: this line is only in the external project section: it isn't required if the library is found via FindDjInterop, as that would be linking to a shared library which already has the dependency explicit.

I just noticed that the ChromaPrint dependency already does this (with FFTW), so at least the situation here is in line with the status quo.

uklotzde · 2020-07-12T08:24:47Z

You could adopt our workaround in util/optional.h to enable compatibility with macOS. The pre-standard version is missing some methods from final standard and might require some workarounds.

uklotzde · 2020-07-12T08:35:10Z

Please rebase on (upstream) master.

CMakeLists.txt

uklotzde · 2020-07-12T09:07:49Z

CMakeLists.txt

+    # libdjinterop depends on zlib and sqlite3.
+    # Mixxx already has a dependency on sqlite3, but not zlib, which we add now.
+    find_package(ZLIB 1.2.8 REQUIRED)


@Holzhaus Opinions?

CMakeLists.txt

mr-smidge · 2020-07-12T09:15:11Z

You could adopt our workaround in util/optional.h to enable compatibility with macOS. The pre-standard version is missing some methods from final standard and might require some workarounds.

@uklotzde , I thought about this approach, but wanted to check: are there plans to upgrade the version of Apple LLVM on the build servers? If there are, I might prefer that rather than add workarounds for old compilers.

mr-smidge · 2020-07-12T09:27:28Z

Please rebase on (upstream) master.

Erm, it already is (at the time of writing) - did you see something to indicate that it wasn't? Thanks.

uklotzde · 2020-07-12T10:15:44Z

@uklotzde , I thought about this approach, but wanted to check: are there plans to upgrade the version of Apple LLVM on the build servers? If there are, I might prefer that rather than add workarounds for old compilers.

We cannot update the CI build servers as long as we want to support older macOS releases. Please add the workaround if you want your library included in macOS builds.

uklotzde · 2020-07-12T10:17:41Z

Please rebase on (upstream) master.

Erm, it already is (at the time of writing) - did you see something to indicate that it wasn't? Thanks.

If cloned your branch locally and tried to rebase on master -> conflicts

uklotzde · 2020-07-12T10:59:27Z

Please rebase on (upstream) master.

Erm, it already is (at the time of writing) - did you see something to indicate that it wasn't? Thanks.

If cloned your branch locally and tried to rebase on master -> conflicts

Sorry, I forgot to pull master.

.travis.yml

Add -DDJINTEROP=ON to Ubuntu/CMake travis and appveyor builds. Add shallow git clone.

mr-smidge · 2020-07-12T12:58:58Z

@uklotzde , I thought about this approach, but wanted to check: are there plans to upgrade the version of Apple LLVM on the build servers? If there are, I might prefer that rather than add workarounds for old compilers.

We cannot update the CI build servers as long as we want to support older macOS releases. Please add the workaround if you want your library included in macOS builds.

Raised xsco/libdjinterop#14 for the libdjinterop library. I'll focus on Linux builds for this PR and will enable it for Mixxx Mac builds in a later PR.

Holzhaus · 2020-07-12T20:01:24Z

CMakeLists.txt

+    set(DJINTEROP_GIT_REPO_URL https://github.com/xsco/libdjinterop.git)
+    set(DJINTEROP_GIT_TAG tags/0.11.0)
+    set(DJINTEROP_INSTALL_DIR "${CMAKE_CURRENT_BINARY_DIR}/lib/libdjinterop-install")
+    if(DJINTEROP_MESON_BUILD)


Why do we need to support both build systems? If libdjinterop has a cmake build, let's just use that one. The user might not even have meson installed and custom build flags can't be passed to meson anyway with this implementation.

Short bit of background: Meson has been the main build system for libdjinterop, based mainly on my personal preferences whilst developing the library, and the easier time with cross-platform dependency management. I originally added a CMake build on a "best efforts" basis with the idea that it would make it easier to integrate with Mixxx. As you note (and as written in a comment), the typical user compiling Mixxx may not even have Meson/Ninja installed.

So I have to think about whether I, as the libdjinterop maintainer, should support two build systems for the library. Given the current simplicity of the libdjinterop build, maybe I can make the CMake build "official".

Nonetheless, I am not about to go and deliberately break the CMake build (!), so I don't mind removing the conditional regardless.

Holzhaus · 2020-07-12T20:05:36Z

CMakeLists.txt

+    # libdjinterop depends on zlib and sqlite3.
+    # Mixxx already has a dependency on sqlite3, but not zlib, which we add now.
+    find_package(ZLIB 1.2.8 REQUIRED)


I think this should be move to the FindDJInterop module. For the external project build, this can be taken care of in libdjinterop's own CMakeLists.txt.

CMakeLists.txt

Holzhaus

Works fine, thanks!

CMakeLists.txt

appveyor.yml

Holzhaus

LGTM, thanks.

mr-smidge · 2020-07-14T13:29:12Z

Many thanks @uklotzde and @Holzhaus .

The Visual Studio AppVeyor build appears to have failed tests with a large number of file-not-found errors. I presume these are transient? If so, please could someone kick off this job again?

mr-smidge · 2020-07-14T15:40:57Z

A re-run failed too. I might need some help with these AppVeyor VS errors - I can't see how these are connected to the commits in this PR. Example:

        Start 219: EngineMicrophoneTest.TestInputMatchesOutput
219/618 Test #219: EngineMicrophoneTest.TestInputMatchesOutput .....................................***Timeout  30.01 sec
Note: Google Test filter = EngineMicrophoneTest.TestInputMatchesOutput
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from EngineMicrophoneTest
[ RUN      ] EngineMicrophoneTest.TestInputMatchesOutput
Loading resources from  "C:/projects/mixxx/cmake_build/res/"
BaseTrackPlayerImpl::slotLoadTrack "[Channel1]"
CachingReaderWorker - "[Channel1]" File not found "C:/projects/mixxx/cmake_build/src/test/sine-30.wav"

Any ideas?

uklotzde · 2020-07-14T16:31:44Z

Strange. I'll push this branch to my personal AppVeyor account for testing.

uklotzde · 2020-07-14T20:43:44Z

Test failures are probably caused by #2938

uklotzde · 2020-07-15T08:25:16Z

Please merge master to verify that the build succeeds now.

mr-smidge · 2020-07-15T11:36:57Z

Please merge master to verify that the build succeeds now.

Done, and the VS AppVeyor build now passes 😄.

Unfortunately, the OSX Scons build failed after timing out! 🤦 Please could this be re-run for a (hopefully) clean bill of health?

mr-smidge · 2020-07-15T13:30:09Z

All green - ok to merge?

Then I'll update PR #2753 .

Holzhaus · 2020-07-15T13:47:55Z

Huh? Shouldn't you merge this into #2753? This doesn't make sense in master on its own...

uklotzde · 2020-07-15T14:00:42Z

@Holzhaus I think we should merge this PR before #2753 now that it is ready. It doesn't do any harm, build times are increased only slightly and the feature can easily be disabled.

Holzhaus · 2020-07-15T14:13:59Z

@uklotzde okay, then go ahead

uklotzde · 2020-07-15T15:59:30Z

LGTM

uklotzde · 2020-07-15T16:01:12Z

Our build servers are not affected until we enable this option. Should be safe.

mr-smidge commented Jul 11, 2020

View reviewed changes

Add libdjinterop external project dependency

3bd92eb

mr-smidge force-pushed the enh/djinterop-external-project branch from fb83912 to 3bd92eb Compare July 11, 2020 23:16

uklotzde suggested changes Jul 12, 2020

View reviewed changes

mr-smidge mentioned this pull request Jul 12, 2020

Lib export with libdjinterop #2753

Merged

mr-smidge force-pushed the enh/djinterop-external-project branch from 6f5008c to b0a6b4c Compare July 12, 2020 09:36

uklotzde reviewed Jul 12, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

Make djinterop explicit opt-in.

0aa65b2

Add -DDJINTEROP=ON to Ubuntu/CMake travis and appveyor builds. Add shallow git clone.

mr-smidge force-pushed the enh/djinterop-external-project branch from b0a6b4c to 0aa65b2 Compare July 12, 2020 12:51

mr-smidge marked this pull request as ready for review July 12, 2020 17:28

Be-ing requested a review from Holzhaus July 12, 2020 17:29

Holzhaus reviewed Jul 12, 2020

View reviewed changes

Use only CMake build from libdjinterop

5f7b77c

uklotzde approved these changes Jul 13, 2020

View reviewed changes

Holzhaus requested changes Jul 13, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

appveyor.yml Outdated Show resolved Hide resolved

Build options named by feature rather than dependency

3c73b2a

mr-smidge requested a review from Holzhaus July 13, 2020 20:59

Holzhaus approved these changes Jul 14, 2020

View reviewed changes

Merge branch 'master' into enh/djinterop-external-project

de24940

uklotzde merged commit edb49dd into mixxxdj:master Jul 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add libdjinterop external project dependency #2932

Add libdjinterop external project dependency #2932

mr-smidge commented Jul 11, 2020 •

edited

Loading

mr-smidge Jul 11, 2020

uklotzde Jul 12, 2020

Holzhaus Jul 12, 2020

mr-smidge Jul 12, 2020 •

edited

Loading

mr-smidge Jul 13, 2020

uklotzde commented Jul 12, 2020

uklotzde commented Jul 12, 2020 •

edited

Loading

uklotzde Jul 12, 2020

mr-smidge commented Jul 12, 2020

mr-smidge commented Jul 12, 2020

uklotzde commented Jul 12, 2020

uklotzde commented Jul 12, 2020

uklotzde commented Jul 12, 2020

mr-smidge commented Jul 12, 2020

Holzhaus Jul 12, 2020

mr-smidge Jul 12, 2020

Holzhaus Jul 12, 2020

Holzhaus left a comment

Holzhaus left a comment

mr-smidge commented Jul 14, 2020

mr-smidge commented Jul 14, 2020

uklotzde commented Jul 14, 2020

uklotzde commented Jul 14, 2020

uklotzde commented Jul 15, 2020

mr-smidge commented Jul 15, 2020

mr-smidge commented Jul 15, 2020

Holzhaus commented Jul 15, 2020

uklotzde commented Jul 15, 2020 •

edited

Loading

Holzhaus commented Jul 15, 2020 •

edited

Loading

uklotzde commented Jul 15, 2020

uklotzde commented Jul 15, 2020

Add libdjinterop external project dependency #2932

Add libdjinterop external project dependency #2932

Conversation

mr-smidge commented Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-smidge Jul 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Jul 12, 2020

uklotzde commented Jul 12, 2020 • edited Loading

Choose a reason for hiding this comment

mr-smidge commented Jul 12, 2020

mr-smidge commented Jul 12, 2020

uklotzde commented Jul 12, 2020

uklotzde commented Jul 12, 2020

uklotzde commented Jul 12, 2020

mr-smidge commented Jul 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus left a comment

Choose a reason for hiding this comment

Holzhaus left a comment

Choose a reason for hiding this comment

mr-smidge commented Jul 14, 2020

mr-smidge commented Jul 14, 2020

uklotzde commented Jul 14, 2020

uklotzde commented Jul 14, 2020

uklotzde commented Jul 15, 2020

mr-smidge commented Jul 15, 2020

mr-smidge commented Jul 15, 2020

Holzhaus commented Jul 15, 2020

uklotzde commented Jul 15, 2020 • edited Loading

Holzhaus commented Jul 15, 2020 • edited Loading

uklotzde commented Jul 15, 2020

uklotzde commented Jul 15, 2020

mr-smidge commented Jul 11, 2020 •

edited

Loading

mr-smidge Jul 12, 2020 •

edited

Loading

uklotzde commented Jul 12, 2020 •

edited

Loading

uklotzde commented Jul 15, 2020 •

edited

Loading

Holzhaus commented Jul 15, 2020 •

edited

Loading