Add -fvisibility=hidden to macOS dynamic framework and export required symbols

[kkaefer]

Fix for #7281

[1ec5]

In addition to the comments below, the contributor documentation needs to be updated to note that MGLTypes.h needs to be imported and MGL_EXTERN needs to be used on all new public symbols:

[1ec5]

Does Interface Builder still recognize MGLMapView as a designable? IB’s Objective-C scanner unfortunately doesn’t know anything about macros.

[kkaefer]

How do I find out whether IB does that? I'm not familiar with it.

[1ec5]

Open MapDocument.xib. Xcode will automatically start building. After awhile, this:

will turn into this:

It’s a bit underwhelming on macOS, but the iOS SDK version of this displays some important setup instructions, so we should make sure designables in general still work.

[kkaefer]

I tried this, but couldn't get to a conclusive answer since Xcode's rebuilding is very erratic and it doesn't seem to detect changed files. From what I could tell, it worked both in the MGL_EXTERN IB_DESIGNABLE order and the other way around. I changed the order though so that IB_DESIGNABLE now is directly before @interface.

[1ec5]

If I’m not mistaken, placing this macro on a separate line between the comment block and the signature breaks Quick Help, which only looks one line up to find a comment block. Would this macro work if we stick it above the comment block, or better yet on the same line as @interface? We should also make sure jazzy – which like Quick Help also relies on Clang to parse out comment blocks – is OK with whatever we end up with.

[kkaefer]

What is Quick Help? How do I invoke Quick Help?

[frederoni]

⌥ + click on class, property or method.

[kkaefer]

@frederoni I cannot confirm that behavior; it works for me with the macro as well.

[frederoni]

nvm @kkaefer I was using an invalid macro. 👍

[1ec5]

Do we really need to extern symbols in headers with private or project visibility? What about symbols declared in .m files, then?

[kkaefer]

Theoretically we shouldn't, since it should be all internal to the binary we're building, however the test target also includes this private header, and linking the lib to the test fails without exporting this symbol. Another way would be to not export this, and change the test so that it doesn't require this symbol, or to move this symbol to the public header.

[1ec5]

Ah, OK, so we need this on anything with Private visibility but not Project visibility (using Xcode terminology here).

[1ec5]

My mistake, this belongs in the next section (which could mention standalone functions as well). This section is for adding anything to a public header, including a property or method.

[1ec5]

Putting MGL_EXTERN keyword on a separate line before the IB_DESIGNABLE keyword seems to break Quick Help and the index in general in Xcode 8.2. If you ⌥-click a member of a class or open the code completion popup, you get the documentation about the class itself rather than the member. If you ⌘-click to jump to the definition of a member, you get the line above it instead.

release-ios-v3.4.0 on this line:

macos-sdk-symbol-visibility on the same line:

Putting the two keywords on the same line seems to fix the problem, but I’ve only tried this so far inside macos.xcodeproj so far.

[1ec5]

I'm concerned that we're going to forget to add MGL_EXTERN to a new public class at some point and we won't catch it before a developer notices that their project won't link. Ideally, we'd have tests that cover every class, but we generally don't have tests for UI classes yet. Also, we'd still have to be vigilant about new classes getting pro forma tests. Very often we forget to add a new helper class to the jazzy table of contents, even though we know to look for that change. (Sometimes a PR adds a significant class with some helper classes.)

[frederoni]

Ideally, we'd have tests that cover every class, but we generally don't have tests for UI classes yet.

If we would have tests that would cover every MGL class, the test scheme would link and we wouldn't be able to catch incorrect symbol visibility in time? If that's not the case, it would be fairly easy to use the Objective-C runtime to get a list of all loaded classes and validate visibility on them in a unit test.

[1ec5]

If we would have tests that would cover every MGL class, the test scheme would link and we wouldn't be able to catch incorrect symbol visibility in time?

I'm not worried about classes we already have. I'm worried about a PR that adds a few public classes, one of which is significant enough that the contributor remembers to make it visible, while they overlook the fact that the other helper classes are also public (or have Private target membership!) and need to be visible. If we're vigilant enough to remind the contributor to add a pro forma "test", we'd already be vigilant enough to remind them to add the macro in the first place. But I estimate that we'd miss a class until after a prerelease about 30% of the time without an automated system. The only way "test everything" would work is if we gather code coverage and already had 100% code coverage, so that every dip in coverage would be detectable.

If that's not the case, it would be fairly easy to use the Objective-C runtime to get a list of all loaded classes and validate visibility on them in a unit test.

This PR is about linker visibility, not runtime visibility. How would we distinguish between a class that's invisible because it should be versus one that's invisible because we forgot to add the macro? NSClassFromString() has to be able to return all classes, including private, invisible classes. Otherwise the class code could never run.

Maybe there's a way to enumerate SourceKitten's generated public symbol list, mangle the symbols, and ensure that they have the right entry in otool output for the framework? Sounds really complicated.

[kkaefer]

I removed the extern/extern "C" keyword from the macro, and renamed it to MGL_EXPORT.