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

Build Opus as shared library in static build, copy it into App Bundle #3646

Conversation

Projects
None yet
4 participants
@davidebeatrici
Copy link
Member

davidebeatrici commented Mar 25, 2019

Fixes #3642.

We load Opus at runtime since #3431.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:macos-shared-opus-installer-fix branch 3 times, most recently from 6f78bd1 to 02eb37a Mar 26, 2019

@davidebeatrici davidebeatrici requested review from mkrautz, hacst and Kissaki Mar 26, 2019

@@ -20,6 +20,11 @@ VPATH = ../$$SOURCEDIR
TARGET = opus
DEFINES += HAVE_CONFIG_H

CONFIG(static) {

This comment has been minimized.

Copy link
@hacst

hacst Mar 26, 2019

Member

Doesn't this also change behaviour for windows? How did we get the right binary there?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 26, 2019

Author Member

The library was already forced to be compiled as shared on Windows:

CONFIG -= static
CONFIG += shared

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 26, 2019

Author Member

I removed those lines, since they are redundant.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Mar 27, 2019

Member

That means we also introduce it for Linux though!?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 27, 2019

Author Member

That's correct.

We never noticed the issue because we don't do static builds on Linux.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:macos-shared-opus-installer-fix branch from 02eb37a to abfeae4 Mar 26, 2019

@davidebeatrici davidebeatrici requested a review from hacst Mar 26, 2019

Show resolved Hide resolved macx/scripts/osxdist.py
@@ -296,7 +296,8 @@ def package_client():
a.copy_resources(['icons/mumble.icns'])
a.update_plist()
if not options.universal:
a.add_compat_warning()
if options.compat_warning:
a.add_compat_warning()

This comment has been minimized.

Copy link
@Kissaki

Kissaki Mar 27, 2019

Member

What is this and what does it have to do with our shared opus library?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 27, 2019

Author Member

From the commit message:

QMake always passes "-arch x86_64" after the parameters specified in our project file, meaning that we can't build the application as 32 bit.

This option allows us to disable the feature (warning when trying to run Mumble on a 32 bit system) for the CI build.

This comment has been minimized.

Copy link
@hacst

hacst Mar 28, 2019

Member

Any idea why it suddenly would do that and not before?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 28, 2019

Author Member

We never built the installer on Travis CI before.

This comment has been minimized.

Copy link
@hacst

hacst Mar 28, 2019

Member

Why would it matter whether it's in travis though?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 28, 2019

Author Member

It shouldn't, indeed.

It's possible that -arch -x86_64 is passed automatically in recent QMake versions.

This comment has been minimized.

Copy link
@hacst

hacst Mar 29, 2019

Member

Does it work in internal CI? Do we have a way to experiment with that? My assumption is that adding such a binary is important for the osx bundle experience or @mkrautz wouldn't have bothered to add it in the first place.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 29, 2019

Author Member

Yes, it works in internal CI, -arch x86_64 is not passed there.

The binary's purpose is to show an informative message in case the bundle is run on a 32 bit system:

@implementation CompatApp
- (void) applicationDidFinishLaunching:(NSNotification *)notification {
(void)notification;
NSAlert *alert = [NSAlert alertWithMessageText:@"Mumble" defaultButton:@"OK" alternateButton:nil otherButton:@"Visit Website"
informativeTextWithFormat:@"This version of Mumble only runs on 64-bit Intel Macs.\n\n"
"Please download the Universal version from the Mumble website instead.\n"];
[[alert window] makeKeyAndOrderFront:self];
[[alert window] center];
NSInteger button = [alert runModal];
if (button == NSAlertOtherReturn) {
[[NSWorkspace sharedWorkspace] openURL:[NSURL URLWithString:@"http://mumble.info/"]];
}
[NSApp terminate:self];
[alert release];
}
@end

@@ -387,6 +388,7 @@ def package_server():
parser.add_option('', '--universal', dest='universal', help='Build an universal snapshot.', action='store_true', default=False)
parser.add_option('', '--only-appbundle', dest='only_appbundle', help='Only prepare the appbundle. Do not package.', action='store_true', default=False)
parser.add_option('', '--only-overlay', dest='only_overlay', help='Only create the overlay installer.', action='store_true', default=False)
parser.add_option('', '--no-compat-warning', dest='compat_warning', help='No warning message when running the image on x86.', action='store_false', default=True)

This comment has been minimized.

Copy link
@Kissaki

Kissaki Mar 27, 2019

Member

We should add why ignoring the warning is ok/necessary here in the reasoning.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 27, 2019

Author Member

"This option should only be used when the warning application can't be built as 32 bit (rendering it useless)."

What do you think?

@mkrautz

This comment has been minimized.

Copy link
Member

mkrautz commented Apr 3, 2019

There's a reason we use static libraries. Because it makes code signing easier. We have no mechanism for signing shared libraries inside the App Bundle on macOS currently/anymore. So I think that should be fixed as well before we can merge this.

@mkrautz

This comment has been minimized.

Copy link
Member

mkrautz commented Apr 4, 2019

Disregard my comment above. I was obviously wrong, since osxdist.py does just that. :-)

@mkrautz
Copy link
Member

mkrautz left a comment

LGTM except for the unrelated change.

Show resolved Hide resolved macx/scripts/osxdist.py

davidebeatrici added some commits Mar 26, 2019

3rdparty/opus-build: build as shared library even if it's a static build
This is the behavior of "celt-0.7.0-build" and "celt-0.11.0-build".

We need it for "opus-build" too because we load it at runtime since e0ee016.
macx/scripts/osxdist.py: copy Opus library into App Bundle
The script copies the CELT (dynamic) libraries into the App Bundle, so that they are installed and then loaded by Mumble.

This commit adds the Opus library to the list of files to copy, necessary because it is loaded at runtime since e0ee016.
macx/scripts/osxdist.py: add "--no-compat-warning" option
QMake always passes "-arch x86_64" after the parameters specified in our project file, meaning that we can't build the application as 32 bit.

This option allows us to disable the feature (warning when trying to run Mumble on a 32 bit system) for the CI build.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:macos-shared-opus-installer-fix branch from abfeae4 to c75aafc Apr 4, 2019

Comment addressed.

@davidebeatrici davidebeatrici merged commit 1e3d6a5 into mumble-voip:master Apr 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.