Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Devendor ableton link #4836

Merged
merged 1 commit into from
Mar 22, 2020
Merged

Conversation

dvzrv
Copy link
Member

@dvzrv dvzrv commented Mar 21, 2020

Purpose and Motivation

This enables package maintainers to use a system version of Ableton Link by specifying -DSYSTEM_ABLETON_LINK=ON when running cmake.

This fixes #4818 in a packaging context (because a system version of Ableton Link is preferred over the vendored version).

Types of changes

  • New feature

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review

@dvzrv dvzrv requested a review from mossheim March 21, 2020 12:21
lang/CMakeLists.txt Outdated Show resolved Hide resolved
${ABLETON_LINK_INCLUDE}/ableton/Link.hpp
)
else()
include(../external_libraries/link/AbletonLinkConfig.cmake)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be included in a package distribution of Link? IIUC that is the correct way to distribute and use libraries with CMake support

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, maybe you're right. This is what Debian ships.

I have not included the cmake files in packaging, because I didn't know where to put them.
Would /usr/share/ableton-link/ or /usr/share/link by the correct place for the .cmake files? Is there a standard for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

That being said: Debian also patches some of the files to make it compatible with a system asio (I do a similar thing, but ignore the .cmake files later on).

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a standard for this?

AFAIK there is no single best practice, but a couple things that will work. according to the documentation for find_package, the following paths are tried with Linux:

<prefix>/(lib/<arch>|lib|share)/cmake/<name>*/          (U)
<prefix>/(lib/<arch>|lib|share)/<name>*/                (U)
<prefix>/(lib/<arch>|lib|share)/<name>*/(cmake|CMake)/  (U)

where <name> is Link if find_package() is called with "Link".

if you call find_package without NO_SYSTEM_ENVIRONMENT_PATH then <prefix> will eventually be tried with /usr/. so a lot of potential options here. let me see if my copy of Professional CMake has a recommended best practice ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, on my Arch system I see this:

[bheim@archlinux supercollider]$ find /usr -name *.cmake | grep -v /usr/lib/cmake | grep -v /usr/share/cmake | sort | less

/usr/lib32/cmake/DBus1/DBus1Config.cmake
/usr/lib32/cmake/DBus1/DBus1ConfigVersion.cmake
/usr/lib32/cmake/harfbuzz/harfbuzz-config.cmake
/usr/lib32/cmake/libxml2/libxml2-config.cmake
/usr/lib32/cmake/Ogg/OggConfig.cmake
/usr/lib32/cmake/Ogg/OggConfigVersion.cmake
/usr/lib32/cmake/Ogg/OggTargets.cmake
/usr/lib32/cmake/Ogg/OggTargets-noconfig.cmake
/usr/lib32/cmake/PulseAudio/PulseAudioConfig.cmake
/usr/lib32/cmake/PulseAudio/PulseAudioConfigVersion.cmake
/usr/lib/CLucene/CLuceneConfig.cmake
/usr/lib/openjpeg-2.3/OpenJPEGConfig.cmake
/usr/lib/openjpeg-2.3/OpenJPEGTargets.cmake
/usr/lib/openjpeg-2.3/OpenJPEGTargets-release.cmake
/usr/local/clang-8/lib/cmake/clang/ClangConfig.cmake
/usr/local/clang-8/lib/cmake/clang/ClangTargets.cmake
/usr/local/clang-8/lib/cmake/clang/ClangTargets-release.cmake
/usr/local/clang-8/lib/cmake/llvm/AddLLVM.cmake
/usr/local/clang-8/lib/cmake/llvm/AddLLVMDefinitions.cmake
/usr/local/clang-8/lib/cmake/llvm/AddOCaml.cmake
/usr/local/clang-8/lib/cmake/llvm/AddSphinxTarget.cmake
/usr/local/clang-8/lib/cmake/llvm/CheckAtomic.cmake
/usr/local/clang-8/lib/cmake/llvm/CheckCompilerVersion.cmake
/usr/local/clang-8/lib/cmake/llvm/CheckLinkerFlag.cmake
/usr/local/clang-8/lib/cmake/llvm/ChooseMSVCCRT.cmake
/usr/local/clang-8/lib/cmake/llvm/CrossCompile.cmake
/usr/local/clang-8/lib/cmake/llvm/DetermineGCCCompatible.cmake
/usr/local/clang-8/lib/cmake/llvm/FindLibpfm.cmake
/usr/local/clang-8/lib/cmake/llvm/FindOCaml.cmake
/usr/local/clang-8/lib/cmake/llvm/FindSphinx.cmake
/usr/local/clang-8/lib/cmake/llvm/GenerateVersionFromCVS.cmake
/usr/local/clang-8/lib/cmake/llvm/GetSVN.cmake
/usr/local/clang-8/lib/cmake/llvm/HandleLLVMOptions.cmake
/usr/local/clang-8/lib/cmake/llvm/HandleLLVMStdlib.cmake
/usr/local/clang-8/lib/cmake/llvm/LLVM-Config.cmake
/usr/local/clang-8/lib/cmake/llvm/LLVMConfig.cmake
/usr/local/clang-8/lib/cmake/llvm/LLVMConfigVersion.cmake
/usr/local/clang-8/lib/cmake/llvm/LLVMExports.cmake
/usr/local/clang-8/lib/cmake/llvm/LLVMExports-release.cmake
/usr/local/clang-8/lib/cmake/llvm/LLVMExternalProjectUtils.cmake
/usr/local/clang-8/lib/cmake/llvm/LLVMInstallSymlink.cmake
/usr/local/clang-8/lib/cmake/llvm/LLVMProcessSources.cmake
/usr/local/clang-8/lib/cmake/llvm/TableGen.cmake
/usr/local/clang-8/lib/cmake/llvm/VersionFromVCS.cmake
/usr/share/doc/gc/README.cmake
/usr/share/graphite2/graphite2.cmake
/usr/share/graphite2/graphite2-release.cmake

Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

IMO Ableton should really distribute https://github.com/Ableton/link/blob/master/AbletonLinkConfig.cmake or a modified version of it as part of the library so it can be included here. if not, we should turn it into a Find module in cmake_modules and use find_package.

IIUC the goal of these 'system' switches is to allow any platform to use an out-of-source installation of a package, not just Linux. i am able to use SYSTEM_BOOST and SYSTEM_YAMLCPP on macOS. but you've hardcoded the library configuration to Linux here.

there is also the complexity of needing to find and configure the asio standalone library. are you sure this is worth it?

@dvzrv
Copy link
Member Author

dvzrv commented Mar 21, 2020

I agree. The integration with asio will have to be patched anyways (that's what Debian does and that's also what I can do). That shouldn't be a problem IMHO.
I think this can be made fairly transparent.

Also: Yes, worth it as there will be more things building against those headers.

Let me check how I can improve the Arch Linux package to include the .cmake files, then I'll build supercollider again against it.

@mossheim
Copy link
Contributor

great, thank you. if Ableton themselves / the other package maintainers are not already doing this, then it will probably be expedient to simply do it ourselves and raise the issue with those package maintainers later. this is an unfortunately neglected aspect of CMake package distribution, which is too bad since it is actually set up to work quite well when everyone acts responsibly.

@mossheim
Copy link
Contributor

based on what I'm seeing on Arch Linux, /usr/lib/cmake/<name> seems to be a very popular destination. that is also what the authors of Professional CMake recommend -- ${CMAKE_INSTALL_LIBDIR}/cmake/MyProj, where CMAKE_INSTALL_LIBDIR is lib (or perhaps lib32 or lib64 depending on host/architecture)

@dvzrv
Copy link
Member Author

dvzrv commented Mar 21, 2020

Yeah, I'm also seeing plenty of /usr/share/<package>/ locations... it's a weird mixed bag.
/usr/lib/cmake/<package> should be preferred though I think.

@dvzrv
Copy link
Member Author

dvzrv commented Mar 21, 2020

Alright, I have revamped the link package on Arch Linux. It now ships AbletonLinkConfig.cmake in /usr/lib/cmake/link. The Debian package ableton-link packages it in /usr/share/ableton-link.
This makes HINTS in the find_package() call necessary, but I think this way it's now much much cleaner and more widely applicable on all operating systems.

Hint: I had to fix the paths in AbletonLinkConfig.cmake to be able to actually use it in a system-wide packaging context.
This works now though! \o/

@dvzrv dvzrv requested a review from mossheim March 21, 2020 20:25
@dvzrv
Copy link
Member Author

dvzrv commented Mar 21, 2020

Closing: I think that both Debian and Arch need to ship AbletonLinkConfig.cmake in {/usr/lib/cmake,/usr/share}/abletonlink/ to be able to drop the HINTS.

@mossheim
Copy link
Contributor

a classic example of why names are important!

@dvzrv
Copy link
Member Author

dvzrv commented Mar 21, 2020

Ableton sadly doesn't make that super obvious for downstreams.

Also, I noticed, that some travis test on macOS got stuck. I can squash and force push to trigger a new build.

@mossheim
Copy link
Contributor

after a close reading of the CMake documentation, it will work if you pass NAMES link ableton-link abletonlink. i verified this locally by getting cmake to find a package file at /usr/share/goose/chicken-config.cmake with find_package(bird NAMES chicken goose REQUIRED). until someone publishes a package called Link with a LinkConfig.cmake file (IMO unlikely), that should be pretty solid, but do what you think is best.

no idea why the CI is failing. other people are experiencing it too: https://travis-ci.community/t/macos-build-an-error-occurred-while-generating-the-build-script/7684/5. will probably resolve on its own.

@mossheim
Copy link
Contributor

aside from what i just mentioned, this looks good, thank you @dvzrv

@dvzrv
Copy link
Member Author

dvzrv commented Mar 21, 2020

Cool! Thanks for yet another round of nerding out about cmake ;-)
I'll finish this up tomorrow morning (and modify to your suggestions). Gotta get some sleep!

@dvzrv
Copy link
Member Author

dvzrv commented Mar 22, 2020

For further clarification and reference as abletonlink(!) is packaged all weird in all downstreams currently:

When AbletonLinkConfig.cmake is located in /usr/lib/cmake/abletonlink/AbletonLinkConfig.cmake or /usr/share/abletonlink/AbletonLinkConfig.cmake, it is sufficient to call find_package(AbletonLink REQUIRED).

As Debian calls their package ableton-link and Arch (currently) link, the configuration is not found in the correct search path: /usr/share/ableton-link/AbletonLinkConfig.cmake and /usr/lib/cmake/link/AbletonLinkConfig.cmake (respectively).
Therefore we currently have to call find_package(AbletonLink NAMES AbletonLink ableton-link link REQUIRED), so that both AbletonLinkConfig.cmake and ableton-link/link are properly resolved.

@dvzrv dvzrv added the comp: build CMake build system label Mar 22, 2020
Adding SYSTEM_ABLETON_LINK (defaults to OFF) option to indicate, that the system version of Ableton Link should be used.

lang/CMakeLists.txt: Calling find_package() for AbletonLink with appropriate NAMES.

Currently downstreams package AbletonLink as ableton-link and link, which places AbletonLinkConfig.cmake in the wrong paths
(e.g. /usr/share/ableton-link and /usr/lib/cmake/link for Debian and Arch respectively).
Therefore find_package() for AbletonLink has to be called with appropriate NAMES, so that both the path *and* the file can be resolved.
Also, setting REQUIRED, so that cmake will fail generically on not finding AbletonLink.
Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@mossheim mossheim merged commit 8f87c09 into supercollider:develop Mar 22, 2020
@dvzrv dvzrv mentioned this pull request Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: build CMake build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Imported target "Ableton::Link" includes non-existent path
2 participants