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

Add Carla support for MacOS #4558

Merged
merged 20 commits into from Sep 15, 2018

Conversation

Projects
None yet
3 participants
@tresf
Member

tresf commented Aug 27, 2018

  • Bump Homebrew formula to use beta version (It'll work with 1.9.9 onward now)
  • Bump to beta version by addressing code changes in #4593
  • Wrap precompiler check around window->winId(); to prevent it from having focus bug.

screen shot 2018-08-27 at 1 59 47 am

Steps:

  1. Install Carla to /Applications:
    https://github.com/falkTX/Carla/releases/download/v1.9.9/Carla_2.0-beta7-macos.dmg (this version has the hang-on-relaunch bug, wait for 2.0 RC2 or stable)
  2. Install LMMS to /Applications:
    https://github.com/tresf/lmms/releases/download/v1.2.0-RC7/lmms-1.2.0-rc6.36-mac10.13.dmg (this version has the focus bug, 1.2.0-RC7 onward will be fixed)
    Details:
  • Adds a new DYNAMIC linking option for BuildPlugin.cmake for -undefined dynamic_lookup linking against an optional .dylib.
  • Adds falktx/Carla as a submodule to LMMS to provide carla-standalone.pc and required headers. These should eventually be moved to a Homebrew recipe.
    • Installs carla-standalone.pc to the build directory with some regex changes. These are hackish and slightly volatile, but the submodule will ensure we're on a dedicated commit hash.
  • For the plugins to show up, LMMS assumes Carla is side-by side (e.g. /Applications/LMMS.app and /Applications/Carla.app).
    • Unfortunately, LMMS is currently hard-coded to look in /Applications/Carla.app when launching the python scripts, so the plugin may appear in the plugin listing but fail to launch if installed elsewhere (e.g. ~/Applications/Carla.app, etc.). This is an easy runtime detection that can be fix if needed, located at plugins/carlabase/carla.cpp#L165
    • I haven't tested this. I don't use Carla, so I'm not sure what's needed to test this out.

Closes #2689

@tresf tresf requested a review from lukas-w Aug 27, 2018

tresf added some commits Aug 27, 2018

@tresf tresf referenced this pull request Aug 27, 2018

Closed

Carla support for OSX #2689

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Aug 27, 2018

Member

The problem is that the build system will try to link with the Carla library even if it doesn't exist. pkg-config seems not to check if the library file is available. I think you should use the pkg-config trick only if the .dylib file is available.
Additionally, you may want to install Carla on Travis-CI.

Member

PhysSong commented Aug 27, 2018

The problem is that the build system will try to link with the Carla library even if it doesn't exist. pkg-config seems not to check if the library file is available. I think you should use the pkg-config trick only if the .dylib file is available.
Additionally, you may want to install Carla on Travis-CI.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Aug 27, 2018

Member

I think you should use the pkg-config trick only if the .dylib file is available.

This is why I've added a new BUILD_PLUGIN LINK option.

I don't think it will make sense to use -bundle_loader for SHARED on macOS. How about keep using SHARED and testing for it instead of the new DYNAMIC?

That was my original thought, to just strip the -bundle_loader flag when building SHARED, but from what I recall it just resulted in more errors.

Note that this library was already using SHARED since #2578. At compile time I don't think we'll know if the target is a .dylib or an .so because the libs are referenced by path and name (not by extension). Correct me if I'm wrong. :) Here's my initial trial and error: #2689 (comment).

Additionally, you may want to install Carla on Travis-CI.

Might have to. I'm pretty sure I got it building locally using -undefined dynamic_lookup without Carla.app installed but I'll have to do some more testing.

Member

tresf commented Aug 27, 2018

I think you should use the pkg-config trick only if the .dylib file is available.

This is why I've added a new BUILD_PLUGIN LINK option.

I don't think it will make sense to use -bundle_loader for SHARED on macOS. How about keep using SHARED and testing for it instead of the new DYNAMIC?

That was my original thought, to just strip the -bundle_loader flag when building SHARED, but from what I recall it just resulted in more errors.

Note that this library was already using SHARED since #2578. At compile time I don't think we'll know if the target is a .dylib or an .so because the libs are referenced by path and name (not by extension). Correct me if I'm wrong. :) Here's my initial trial and error: #2689 (comment).

Additionally, you may want to install Carla on Travis-CI.

Might have to. I'm pretty sure I got it building locally using -undefined dynamic_lookup without Carla.app installed but I'll have to do some more testing.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Aug 28, 2018

Member

I think you should use the pkg-config trick only if the .dylib file is available.

I mean something like this: PhysSong@65983c5

That was my original thought, to just strip the -bundle_loader flag when building SHARED, but from what I recall it just resulted in more errors.

See e.g. PhysSong@c57a607. I'm not sure about the CMP0068 part though.

At compile time I don't think we'll know if the target is a .dylib or an .so

IIRC it's determined when linking.

Member

PhysSong commented Aug 28, 2018

I think you should use the pkg-config trick only if the .dylib file is available.

I mean something like this: PhysSong@65983c5

That was my original thought, to just strip the -bundle_loader flag when building SHARED, but from what I recall it just resulted in more errors.

See e.g. PhysSong@c57a607. I'm not sure about the CMP0068 part though.

At compile time I don't think we'll know if the target is a .dylib or an .so

IIRC it's determined when linking.

tresf added some commits Aug 28, 2018

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Aug 28, 2018

Member

See e.g. PhysSong/lmms@c57a607.

Did you mean to completely remove the SHARED|BUNDLE logic for all systems there? I'm out of my expertise here with the linking stuff. If I'm doing it wrong, feel free to help fix it. I've tried a bunch of combinations and the SHARED + -undefined dynamic_lookup seems to work best for Carla. I'm not sure if it would break other parts of the software though so I'm hesitant to change it.

I'm not sure about the CMP0068 part though.

I have that fixed now. Specified the old cmake behavior.

Member

tresf commented Aug 28, 2018

See e.g. PhysSong/lmms@c57a607.

Did you mean to completely remove the SHARED|BUNDLE logic for all systems there? I'm out of my expertise here with the linking stuff. If I'm doing it wrong, feel free to help fix it. I've tried a bunch of combinations and the SHARED + -undefined dynamic_lookup seems to work best for Carla. I'm not sure if it would break other parts of the software though so I'm hesitant to change it.

I'm not sure about the CMP0068 part though.

I have that fixed now. Specified the old cmake behavior.

tresf added some commits Aug 30, 2018

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Aug 30, 2018

Member

All concerns addressed, just waiting for Homebrew/homebrew-core#31560 to be merged.

Member

tresf commented Aug 30, 2018

All concerns addressed, just waiting for Homebrew/homebrew-core#31560 to be merged.

@tresf tresf added this to the 1.2.0 milestone Sep 7, 2018

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Sep 13, 2018

Member

Due to some changes in Carla, the Homebrew recipe requires updating and then we'll have to copy in the patches kindly provided by @falkTX in #4593.

SInce ee6d502, b38dd02 it'll build with 1.9.9 or 2.0.0 now. We'll automatically take the new carla version when it's ready. 👍

Member

tresf commented Sep 13, 2018

Due to some changes in Carla, the Homebrew recipe requires updating and then we'll have to copy in the patches kindly provided by @falkTX in #4593.

SInce ee6d502, b38dd02 it'll build with 1.9.9 or 2.0.0 now. We'll automatically take the new carla version when it's ready. 👍

Show resolved Hide resolved CMakeLists.txt

tresf added some commits Sep 14, 2018

@falkTX

This comment has been minimized.

Show comment
Hide comment
@falkTX

falkTX Sep 14, 2018

Contributor

I am not comfortable with this... feels like a regression.
I added the new library specifically so we can use it, to do things in a proper way.

users with older carla just have to update it first. and it is for their own good, as the new version is the most stable yet by a long margin.

Contributor

falkTX commented on b38dd02 Sep 14, 2018

I am not comfortable with this... feels like a regression.
I added the new library specifically so we can use it, to do things in a proper way.

users with older carla just have to update it first. and it is for their own good, as the new version is the most stable yet by a long margin.

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Sep 14, 2018

Member

Homebrew just accepted 1.9.9 so per our discussion on Discord we've decided to keep this for now. I have no attachment to supporting the old versions, but there are likely to be edgecases with Linux distros that package our beta that would be surprised by this. TL;DR from #4593 here: #4593 (comment).

Screenshot of our Discord convo

image

Member

tresf replied Sep 14, 2018

Homebrew just accepted 1.9.9 so per our discussion on Discord we've decided to keep this for now. I have no attachment to supporting the old versions, but there are likely to be edgecases with Linux distros that package our beta that would be surprised by this. TL;DR from #4593 here: #4593 (comment).

Screenshot of our Discord convo

image

Show resolved Hide resolved CMakeLists.txt
@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Sep 14, 2018

Member

@PhysSong once Travis-CI passes, this is ready for merge. The only thing we need decision on is the style of the precompiler checks in #4558 (review).

Member

tresf commented Sep 14, 2018

@PhysSong once Travis-CI passes, this is ready for merge. The only thing we need decision on is the style of the precompiler checks in #4558 (review).

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Sep 15, 2018

Member

Updates:

  • Wrapped a precompiler check around window->winId(); to disable it on macOS to prevent it from having a major focus bug. Possibly related: #4183, QTBUG-63787, QTBUG-30181, QTBUG-69343.
  • @falkTX was able to fix a pretty serious bug preventing LMMS from being relaunched after Carla was loaded on MacOS thus closing falkTX/Carla#737, a blocker for the Carla 2.0 release. This was also affecting Ardour and it took a very long time to locate. See #programming channel on Discord, search for "semaphore" if interested in the saga... just to explain the severity of this bug, you'd have to logout from the macOS Desktop and back in again to get LMMS to relaunch from the Dock and there was no backtrace. He debugged this line-by-line by logging out and back in. Thanks again @falkTX. 🎉
  • All code comments addressed.

Waiting for Travis-CI, then merging.

Member

tresf commented Sep 15, 2018

Updates:

  • Wrapped a precompiler check around window->winId(); to disable it on macOS to prevent it from having a major focus bug. Possibly related: #4183, QTBUG-63787, QTBUG-30181, QTBUG-69343.
  • @falkTX was able to fix a pretty serious bug preventing LMMS from being relaunched after Carla was loaded on MacOS thus closing falkTX/Carla#737, a blocker for the Carla 2.0 release. This was also affecting Ardour and it took a very long time to locate. See #programming channel on Discord, search for "semaphore" if interested in the saga... just to explain the severity of this bug, you'd have to logout from the macOS Desktop and back in again to get LMMS to relaunch from the Dock and there was no backtrace. He debugged this line-by-line by logging out and back in. Thanks again @falkTX. 🎉
  • All code comments addressed.

Waiting for Travis-CI, then merging.

@tresf tresf merged commit 153f15f into LMMS:stable-1.2 Sep 15, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment