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

Remove runtime dynamic loading of libmp3lame in favor of compile-time linking. #1802

Merged
merged 1 commit into from
Sep 18, 2018

Conversation

rryan
Copy link
Member

@rryan rryan commented Sep 13, 2018

Fixes Launchpad Bug #1294128.

class LAME(Dependence):
def configure(self, build, conf):
if not conf.CheckLib(['libmp3lame', 'libmp3lame-static']):
raise Exception("Could not find libmp3lame.")
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to make this optional instead of required?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we probably always want an MP3 encoder since the recording feature is non-optional and the broadcast feature is always enabled in our releases, so I don't see a reason to make it optional. Plus it would take more work to make it optional because the existing code that handled lame not being dynamically loadable was super hacky (just throws up modal error message boxes instead of properly graying out the MP3 option in the preferences).

@rryan
Copy link
Member Author

rryan commented Sep 15, 2018

Waiting on a Windows environment re-build because lame.h was installed to the wrong place.

@rryan
Copy link
Member Author

rryan commented Sep 17, 2018

Waiting on a Windows environment re-build because lame.h was installed to the wrong place.

Ok, should be fixed after 7d9b4bd.

@rryan
Copy link
Member Author

rryan commented Sep 18, 2018

@JosepMaJAZ has tested the x64 build on Windows 7.
I've tested on macOS 10.13.6.

I believe this is ready to merge.

@daschuer
Copy link
Member

LGTM! Thank you.

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

Successfully merging this pull request may close these issues.

3 participants