Add -fvisibility=hidden to macOS dynamic framework and export required symbols by kkaefer · Pull Request #7509 · mapbox/mapbox-gl-native

kkaefer · 2016-12-21T13:13:07Z

Reroll of #7326, which apparently has broken CI statuses.

mention-bot · 2016-12-21T13:13:10Z

@kkaefer, thanks for your PR! By analyzing this pull request, we identified @1ec5, @boundsj and @incanus to be potential reviewers.

kkaefer · 2016-12-21T13:13:54Z

Grr, this seems to be broken as well...

kkaefer · 2017-01-02T10:09:44Z

Let's try approving this to see if we can merge it.

1ec5

From a cursory look through the headers included from the umbrella header, it looks like you’ve exported all the public classes and constants without any omissions. That must’ve been pretty tedious. 😄

What about protocols and categories? Do they need to be exported as well?

My concerns from #7326 (comment) still hold. Documenting the need to use MGL_EXPORT in DEVELOPING.md is a good step but not enough to avoid latent bugs. Consider what’ll happen when we merge a PR like #7377 that made a previously private class public while adding and removing other public classes.

What we need is an automated way to discover public classes. In #7569, I introduced a dependency on SourceKitten and used it (plus a regular expression) to discover all public symbols and preprocess their documentation comments. We could employ something very similar to find public classes and constants that lack the MGL_EXPORT keyword and even insert the keyword if it’s missing.

Once you restore the framework import statements, I’ll approve this PR because it solves an exigent build issue (lots of warnings). However, we’ll need to address the maintainability issue before long; otherwise, I’ll have to revert the visibility changes before the next macOS SDK release.

1ec5 · 2017-01-02T11:23:51Z

@@ -1,11 +1,12 @@
-#import <Foundation/Foundation.h>


Apparently we need these import statements or else Swift declarations in the generated documentation lack specific types: realm/jazzy#609. Importing required system frameworks is good practice anyways, unless you use a precompiled header that imports them.

(Sorry, I started this review a couple weeks ago but forgot to publish it.)

MGLTypes.h imports Foundation.h

As mentioned in that jazzy ticket, this header itself must import Foundation.h. It's unfortunate, but that shouldn't pose a problem for the visibility changes you're making, should it?

I’m not sure why, but we don’t seem to be affected by realm/jazzy#609 even with these changes. Running make xdocument turns up some broken Swift declarations, but that happens on master too. (@incanus may have a stronger opinion than I about explicitly importing frameworks in headers. My concern was limited to jazzy output.)

kkaefer · 2017-01-02T13:36:41Z

What about protocols and categories? Do they need to be exported as well?

No, we only need to export compiled code, but protocols/categories are a header-only feature.

1ec5 · 2017-01-03T06:15:16Z

 `style` and obtain the background layer using the `-[MGLStyle layerWithIdentifier:]` 
 method and passing `background` for the identifier. 
 */
+MGL_EXPORT


This change will get blown away as soon as anyone invokes make style-code. Make this change in platform/darwin/src/MGLStyleLayer.h.ejs and run make style-code to propagate the change to all the affected headers.

This is fixed

1ec5 · 2017-01-03T06:19:10Z

@@ -1,11 +1,12 @@
-#import <Foundation/Foundation.h>


I’m not sure why, but we don’t seem to be affected by realm/jazzy#609 even with these changes. Running make xdocument turns up some broken Swift declarations, but that happens on master too. (@incanus may have a stronger opinion than I about explicitly importing frameworks in headers. My concern was limited to jazzy output.)

1ec5 · 2017-01-03T09:38:33Z

What about protocols and categories? Do they need to be exported as well?

No, we only need to export compiled code, but protocols/categories are a header-only feature.

Protocols are header-only in Objective-C, but a category may or may not have an implementation in a translation unit separate from the categorized class. (Otherwise, it’s a class extension.) See platform/darwin/src/NSValue+MGLAdditions.m for example.

kkaefer · 2017-01-03T15:22:01Z

I've rebuilt this PR based on SourceKitten's output.

Here are the changes:

Moved the MGL_EXPORT macro to a MGLFoundation.h to mirror other Cocoa frameworks usage of the *_EXPORT macro.
Left includes for Foundation.h in place
Added a script that checks all exported symbols for presence of a MGL_EXPORT macro
Added the automated script to make xpackage

These symbols are not exposed in the public headers, but the Xcode CI unit tests include the private headers and needs them to be visible in the library. These types of symbols are not checked by the above script, but we'll catch those easily because the test will fail to run on CI.

MGLAttributionButton
MGLAltitudeForZoomLevel
MGLZoomLevelForAltitude
MGLFeaturesFromMBGLFeatures
MGLTileSetFromTileURLTemplates
MGLGeoJSONOptionsFromDictionary

rebuild PR

kkaefer · 2017-01-03T15:27:24Z

Protocols are header-only in Objective-C, but a category may or may not have an implementation in a translation unit separate from the categorized class.

I checked this and it seems they don't need the MGL_EXPORT statement and are automatically exported. The CI tests + macOS app uses some of those symbols and can be successfully linked.

1ec5

In addition to the logistical comments below, can you add a blurb to the macOS changelog if a significant size reduction results from these changes?

1ec5 · 2017-01-03T21:53:07Z

+
+#import <Foundation/Foundation.h>
+
+#define MGL_EXPORT __attribute__((visibility ("default")))


How do we decide when something goes in MGLFoundation.h versus MGLTypes.h? Should we bring more macros over here, like the nullability and lightweight generics shims?

As per convention in other system frameworks, it has things that are required in virtually every header file. Most other *Foundation.h files I checked only have the export (or extern) macro.

1ec5 · 2017-01-03T21:55:55Z


 To add an Objective-C class, protocol, category, typedef, enumeration, or global constant to the macOS SDK’s public interface:

+1. _(Optional.)_ Add the macro `MGL_EXPORT` prior to the declaration for classes and global constants. To use this macro, include `MGLFoundation.h`. You can check whether all public symbols are exported correctly by running `platform/darwin/scripts/check-public-symbols.js macOS`.


If someone working on the iOS SDK adds a class to a header in platform/darwin/src/, that’ll cause the class to become public in the macOS SDK too. So we should add a note about running this script to the iOS DEVELOPING.md as well?

1ec5 · 2017-01-03T21:56:32Z


 To add an Objective-C class, protocol, category, typedef, enumeration, or global constant to the macOS SDK’s public interface:

+1. _(Optional.)_ Add the macro `MGL_EXPORT` prior to the declaration for classes and global constants. To use this macro, include `MGLFoundation.h`. You can check whether all public symbols are exported correctly by running `platform/darwin/scripts/check-public-symbols.js macOS`.


Can we make this invocation a little more memorable by adding a Make rule?

Added make check-public-symbols

Great. Can you refer to make check-public-symbols in both DEVELOPING.md documents? Thanks!

1ec5 · 2017-01-03T22:01:05Z

+
 /// Project version number for Mapbox.
-FOUNDATION_EXPORT double MapboxVersionNumber;
+FOUNDATION_EXPORT MGL_EXPORT double MapboxVersionNumber;


Make sure MapboxVersionNumber and MapboxVersionString are correctly set in the output build. I would hope that Xcode isn’t dependent on the particular syntax used to declare these constants, but you never know.

I get this linker error referencing MapboxVersonNumber within Swift code:

Undefined symbols for architecture x86_64: "_MapboxVersionNumber", referenced from: Invincibility.ViewController.viewDidLoad () -> () in ViewController.o ld: symbol(s) not found for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation)

Reproduces with or without MGL_EXPORT. Doesn’t reproduce on master as of 654061c.

From a C build perspective, this is very weird. FOUNDATION_EXPORT is only macro for extern, meaning it should be defined in some translation unit (but it's not). Should we manually define this version number?

Looks like Xcode automatically generates a file called Mapbox_vers.c

kkaefer · 2017-01-04T11:12:34Z

can you add a blurb to the macOS changelog if a significant size reduction results from these changes?

There is no size reduction with this PR. This already happened in #7130. This one is just a followup that also compiles the Objective-C code with -fvisibility=hidden to fix #7281.

kkaefer · 2017-01-04T11:15:03Z

We're now also adding the visibility change to the auto-generated file.

Clever hack. (I think this build setting is usually used for Apple to prepend a class prefix like “NS” or “UI” to the variable name.)

kkaefer · 2017-01-04T11:15:52Z

Added a test here to check linkage. This is not a Swift-specific issue; it also happened with Objective-C.

Note that MapboxVersionString is also available to Objective-C (but not to Swift, owing to its type). I assume it’s also covered by VERSION_INFO_PREFIX.

Yeah, it is.

1ec5 · 2017-01-04T17:23:35Z

iOS builds are failing because the iOS version of Mapbox.h doesn’t import MGLFoundation.h:

❌  /Users/vagrant/git/platform/darwin/src/MGLAccountManager.h:3:9: include of non-modular header inside framework module 'Mapbox.MGLAccountManager'

#import "MGLFoundation.h"
        ^

… required symbols

1ec5

Looks great, thanks for fixing this!

kkaefer requested review from 1ec5 and incanus January 2, 2017 10:09

1ec5 suggested changes Jan 2, 2017

View reviewed changes

1ec5 force-pushed the 7509-macos-sdk-symbol-visibility branch from b7623c8 to 38dab67 Compare January 2, 2017 11:46

1ec5 previously requested changes Jan 3, 2017

View reviewed changes

kkaefer force-pushed the 7509-macos-sdk-symbol-visibility branch from 38dab67 to e8c4ac2 Compare January 3, 2017 15:18

kkaefer requested a review from 1ec5 January 3, 2017 15:25

1ec5 reviewed Jan 3, 2017

View reviewed changes

kkaefer mentioned this pull request Jan 4, 2017

default to -fvisibility=hidden #898

Closed

kkaefer force-pushed the 7509-macos-sdk-symbol-visibility branch from e8c4ac2 to 43d16e9 Compare January 4, 2017 11:14

kkaefer commented Jan 4, 2017

View reviewed changes

kkaefer added 2 commits January 4, 2017 21:19

[build] add -fvisibility=hidden to macOS dynamic framework and export…

06e1426

… required symbols

[macOS] use SourceKit to ensure that all public symbols are exported

60055ae

kkaefer force-pushed the 7509-macos-sdk-symbol-visibility branch 2 times, most recently from ee1d84c to 59088d3 Compare January 4, 2017 21:22

1ec5 approved these changes Jan 4, 2017

View reviewed changes

[docs] Add note about MGL_EXPORT to macOS documentation

e26042e

kkaefer force-pushed the 7509-macos-sdk-symbol-visibility branch from 59088d3 to e26042e Compare January 4, 2017 21:32

kkaefer merged commit b515762 into master Jan 4, 2017

kkaefer deleted the 7509-macos-sdk-symbol-visibility branch January 4, 2017 22:06

kkaefer mentioned this pull request Jan 5, 2017

Add -fvisibility=hidden to iOS #7604

Merged


		#import <Foundation/Foundation.h>

		#define MGL_EXPORT __attribute__((visibility ("default")))


		To add an Objective-C class, protocol, category, typedef, enumeration, or global constant to the macOS SDK’s public interface:

		1. _(Optional.)_ Add the macro `MGL_EXPORT` prior to the declaration for classes and global constants. To use this macro, include `MGLFoundation.h`. You can check whether all public symbols are exported correctly by running `platform/darwin/scripts/check-public-symbols.js macOS`.

Conversation

kkaefer commented Dec 21, 2016

Uh oh!

mention-bot commented Dec 21, 2016

Uh oh!

kkaefer commented Dec 21, 2016

Uh oh!

kkaefer commented Jan 2, 2017

Uh oh!

1ec5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkaefer commented Jan 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kkaefer commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kkaefer commented Jan 3, 2017

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkaefer commented Jan 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 commented Jan 4, 2017

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

1ec5 left a comment •

edited

Loading

1ec5 commented Jan 3, 2017 •

edited

Loading

kkaefer commented Jan 3, 2017 •

edited

Loading

1ec5 Jan 3, 2017 •

edited

Loading

1ec5 Jan 3, 2017 •

edited

Loading