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

Added workaround for consuming projects that add Ableton Link via CPM (package manager) #138

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ianacaburian
Copy link

Changes:

  • Extracted config scripts into new files wrapped in functions to allow customisation of PATH_TO_LINK dir.
  • Enables consuming projects to specify the path as the automatically generated package dir provided by the CPM cmake package manager.

Rationale:

  • CPM is suited for CMake packages that are structured for consumption via cmake built-ins 'find_package()' or 'add_subdirectory()'.
  • Alternatively, CPM allows manual configuration for incompatible packages (like Link or Lua or range-v3) by allowing the consumer to download the package without configuration then exposes the downloaded content's location via CMake cache variables.
  • Ableton Link's root CMakeLists.txt is designed to build for package development, meaning that when CPM calls 'add_subdirectory(PATH_TO_LINK)', many build errors result while also unnecessarily building development content (e.g. examples and tests).
  • Ableton advises to configure Link as a dependency via AbletonLinkConfig.cmake and AsioStandaloneConfig.cmake which both hard code the PATH_TO_LINK dir as "CMAKE_CURRENT_LIST_DIR".
  • The consumer could simply re-implement this config file or Ableton can provide some support for CPM like this commit (although obviously not an ideal solution).

Installing with CPM

Simply add the following to a consuming project's CMakeLists.txt that already uses CPM.

CPMAddPackage(Link
  NAME Link
  GIT_TAG 3.1.0
  GITHUB_REPOSITORY ableton/link
  DOWNLOAD_ONLY YES
  SYSTEM YES
  EXCLUDE_FROM_ALL YES
)
if(Link_ADDED)  
  include(${Link_SOURCE_DIR}/cmake_include/ConfigureAbletonLink.cmake)
  ConfigureAbletonLink(${Link_SOURCE_DIR})
endif()

That's it!

Thanks for thinking about this.

…or consumers that use CPM to manage dependencies. Rationale and installation instructions provided in the pull request.
…_TO_LINK customization. Fixed bug that set UNIX properties for APPLE.
@fgo-ableton
Copy link
Collaborator

fgo-ableton commented Nov 8, 2023

Hi @ianacaburian, thanks for the PR!
I have not really looked into CPM yet and I won't get to do it in the next few weeks. I'll have a look when I have some time available.

@fgo-ableton
Copy link
Collaborator

fgo-ableton commented Jun 28, 2024

Hey @ianacaburian, sorry for getting only back now.
I took a look at this and it makes sense to me in general.
I'd like to avoid duplicating so much CMake config though. I think nicer approach would be to just move the entire content of AbletonLinkConfig.cmake to cmake_include/ConfigureAbletonLink.cmake and then call the latter from from the main config.
As I have not been able to get back for so long, let me know if I should take care of this.

@ianacaburian
Copy link
Author

Hey @fgo-ableton,

Duplicate config content has been replaced with calls to the new configure helpers.
The root file AbletonLinkConfig.cmake has been retained to avoid breaking changes for consumers and documentation updates.

Cheers,
Ian.

@fgo-ableton
Copy link
Collaborator

@ianacaburian Thanks for the updates!
Unfortunately the changes contain some errors and don't pass CI. I opened new PR as a reference that contains some fixup commits.

I can't really merge this PR as it is because the commits on it don't build. It seems you have already merged to master so I guess you don't want to squash the fixups. If you'd like to have a PR opened by you merged I'd somehow require a cleaner branch where all commits build. Otherwise I'd just merge the other PR I opened to add CPR support.

Also, I need a CLA to merge: http://ableton.github.io/cla/

Sorry for the complications...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants