Add SF2 support for macOS #3446

Merged
merged 2 commits into from Mar 21, 2017

Conversation

Projects
None yet
3 participants
@tresf
Member

tresf commented Mar 21, 2017

Leveraging Homebrew, we can switch off the problematic enable-coreaudio flag by hosting our own modified fluid-synth.rb brew formula.

Closes #649

@tresf tresf referenced this pull request Mar 21, 2017

Closed

SF2 hangs on open (Apple) #649

@jasp00

This comment has been minimized.

Show comment
Hide comment
@jasp00

jasp00 Mar 21, 2017

Member

Could you add license information to cmake/apple/fluid-synth.rb following these guidelines? Besides that, the patch looks fine.

Member

jasp00 commented Mar 21, 2017

Could you add license information to cmake/apple/fluid-synth.rb following these guidelines? Besides that, the patch looks fine.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Mar 21, 2017

Member

Could you add license information to cmake/apple/fluid-synth.rb following these guidelines? Besides that, the patch looks fine.

Those guidelines don't apply, it's not our code. This is a +1 one-liner directly from Homebrew upstream. I'd like to attach something, but I'd rather not make the diff any larger than one line. Advice is welcome.

Member

tresf commented Mar 21, 2017

Could you add license information to cmake/apple/fluid-synth.rb following these guidelines? Besides that, the patch looks fine.

Those guidelines don't apply, it's not our code. This is a +1 one-liner directly from Homebrew upstream. I'd like to attach something, but I'd rather not make the diff any larger than one line. Advice is welcome.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Mar 21, 2017

Member

[...] original contributions are assumed to be under GPL-2.0+. However, we must be sure that content under an incompatible license has not been copied from another project

Doesn't apply.... GPL 2.0 is completely valid in this case, since the BSD-2 can be downgraded to GPL 2.0.

All files should have copyright information. It is general convention to include a full notice, but we only need these lines:

The copyright information is missing from the file, so this is an upstream bug. Even upstream specifies to use "Homebrew contributors" which is against the standard you've just created. This is superfluous. I'd argue this file isn't even source code. It's a config file. There's really no copyrightable logic in it from a legal perspective.

I've added it, but this is nitpicking to a new level.

Member

tresf commented Mar 21, 2017

[...] original contributions are assumed to be under GPL-2.0+. However, we must be sure that content under an incompatible license has not been copied from another project

Doesn't apply.... GPL 2.0 is completely valid in this case, since the BSD-2 can be downgraded to GPL 2.0.

All files should have copyright information. It is general convention to include a full notice, but we only need these lines:

The copyright information is missing from the file, so this is an upstream bug. Even upstream specifies to use "Homebrew contributors" which is against the standard you've just created. This is superfluous. I'd argue this file isn't even source code. It's a config file. There's really no copyrightable logic in it from a legal perspective.

I've added it, but this is nitpicking to a new level.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Mar 21, 2017

Member

Besides that, the patch looks fine.

Merging.

Member

tresf commented Mar 21, 2017

Besides that, the patch looks fine.

Merging.

@tresf tresf merged commit f57202c into LMMS:master Mar 21, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@jasp00

This comment has been minimized.

Show comment
Hide comment
@jasp00

jasp00 Mar 21, 2017

Member

Thanks.

Doesn't apply.... GPL 2.0 is completely valid in this case, since the BSD-2 can be downgraded to GPL 2.0.

@tresf, you know it, I know it, but a newcomer will not.

I'd argue this file isn't even source code.

Yet there is a class with a definition of bottle, install and test. As I see it, it is not clearly a configuration file. Better safe than sorry.

Member

jasp00 commented Mar 21, 2017

Thanks.

Doesn't apply.... GPL 2.0 is completely valid in this case, since the BSD-2 can be downgraded to GPL 2.0.

@tresf, you know it, I know it, but a newcomer will not.

I'd argue this file isn't even source code.

Yet there is a class with a definition of bottle, install and test. As I see it, it is not clearly a configuration file. Better safe than sorry.

@GamingStar

This comment has been minimized.

Show comment
Hide comment
@GamingStar

GamingStar Apr 18, 2017

I dragged the sf2 player onto the channel rack and the program crashed :/

I ran the program from the terminal, here's the result. It crashed after it said 'Illegal operation'
*Edited by tresf, added screenshot & error text
image
Illegal Instruction: 4

GamingStar commented Apr 18, 2017

I dragged the sf2 player onto the channel rack and the program crashed :/

I ran the program from the terminal, here's the result. It crashed after it said 'Illegal operation'
*Edited by tresf, added screenshot & error text
image
Illegal Instruction: 4

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Apr 18, 2017

Member

@GamingStar thanks for your unit testing today on Discord. From what I am reading online, Illegal Instruction: 4 is often caused by either an API compatibility difference or sometimes caused by compiler optimizers.

When we swapped out the fluidsynth library on your machine with one that was built using debug flags (to strip all optimization flags), it still crashed in the same fashion.

Some additional information... You're on MacOS 10.12. My build machine is on 10.11 and one other community member on Discord was on 10.12 as well however only your machine exhibits this crash. I believe your MacBook Pro is several years older than ours so it may be lacking certain CPU features.

The most effective way to troubleshoot this crash is to get a better backtrace of the problem, and that is done by creating a debug build and running a debugger against the codebase.

This takes a while to configure, build and then debug but without a machine to reproduce on our hands our tied.

Our apple build tutorial is here: https://github.com/LMMS/lmms/wiki/Compiling-lmms-(Apple)

Once we can isolate the cause we can begin to fix this problem.

Member

tresf commented Apr 18, 2017

@GamingStar thanks for your unit testing today on Discord. From what I am reading online, Illegal Instruction: 4 is often caused by either an API compatibility difference or sometimes caused by compiler optimizers.

When we swapped out the fluidsynth library on your machine with one that was built using debug flags (to strip all optimization flags), it still crashed in the same fashion.

Some additional information... You're on MacOS 10.12. My build machine is on 10.11 and one other community member on Discord was on 10.12 as well however only your machine exhibits this crash. I believe your MacBook Pro is several years older than ours so it may be lacking certain CPU features.

The most effective way to troubleshoot this crash is to get a better backtrace of the problem, and that is done by creating a debug build and running a debugger against the codebase.

This takes a while to configure, build and then debug but without a machine to reproduce on our hands our tied.

Our apple build tutorial is here: https://github.com/LMMS/lmms/wiki/Compiling-lmms-(Apple)

Once we can isolate the cause we can begin to fix this problem.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Apr 18, 2017

Member

Here's some more information on it...

muammar/mkchromecast#4 (comment)

In the event of mkchromecast project, he was able to build from a VM to fix this (this is what we do for stable releases, so this may be a viable option).

This is a lengthier process due to performance issues with macOS in a VM, but I'll begin the process now and link a new installer once completed.

Member

tresf commented Apr 18, 2017

Here's some more information on it...

muammar/mkchromecast#4 (comment)

In the event of mkchromecast project, he was able to build from a VM to fix this (this is what we do for stable releases, so this may be a viable option).

This is a lengthier process due to performance issues with macOS in a VM, but I'll begin the process now and link a new installer once completed.

@tresf

This comment has been minimized.

Show comment
Hide comment
Member

tresf commented Apr 19, 2017

@GamingStar can you try this build?

tresf added a commit to tresf/lmms that referenced this pull request Apr 18, 2018

tresf added a commit that referenced this pull request Apr 20, 2018

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