-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add -fvisibility=hidden to macOS dynamic framework and export required symbols #7509
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| 'use strict'; | ||
|
|
||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const execFileSync = require('child_process').execFileSync; | ||
| const _ = require('lodash'); | ||
|
|
||
| const keyword = /\bMGL_EXPORT\b/; | ||
|
|
||
| let scanned = []; | ||
|
|
||
| function hasMissingSymbols(os) { | ||
| let missing = false; | ||
| let sdk = os === 'iOS' ? 'iphonesimulator' : 'macosx'; | ||
| let sysroot = execFileSync('xcrun', ['--show-sdk-path', '--sdk', sdk]).toString().trim(); | ||
| let umbrellaPath = `platform/${os.toLowerCase()}/src/Mapbox.h`; | ||
| let docArgs = ['doc', '--objc', umbrellaPath, '--', | ||
| '-x', 'objective-c', '-I', 'platform/darwin/src/', '-isysroot', sysroot]; | ||
| let docStr = execFileSync('sourcekitten', docArgs).toString().trim(); | ||
| let docJson = JSON.parse(docStr); | ||
| _.forEach(docJson, function (result) { | ||
| _.forEach(result, function (structure, path) { | ||
| // Prevent multiple scans of the same file. | ||
| if (scanned.indexOf(path) >= 0) return; | ||
| scanned.push(path); | ||
|
|
||
| const src = fs.readFileSync(path, 'utf8').split('\n'); | ||
| _.forEach(structure['key.substructure'], function (substructure) { | ||
| switch (substructure['key.kind']) { | ||
| case 'sourcekitten.source.lang.objc.decl.class': | ||
| if (!(keyword.test(src[substructure['key.doc.line'] - 1]) || keyword.test(src[substructure['key.doc.line'] - 2]))) { | ||
| console.warn(`- missing symbol export for class ${substructure['key.name']} in ${path}:${substructure['key.doc.line']}:${substructure['key.doc.column']}`); | ||
| missing = true; | ||
| } | ||
| break; | ||
| case 'sourcekitten.source.lang.objc.decl.constant': | ||
| if (!keyword.test(src[substructure['key.doc.line'] - 1])) { | ||
| console.warn(`- missing symbol export for constant ${substructure['key.name']} in ${path}:${substructure['key.doc.line']}:${substructure['key.doc.column']}`); | ||
| missing = true; | ||
| } | ||
| break; | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| return missing; | ||
| } | ||
|
|
||
| function ensureSourceKittenIsInstalled() { | ||
| try { | ||
| execFileSync('which', ['sourcekitten']); | ||
| } catch (e) { | ||
| console.log(`Installing SourceKitten via Homebrew…`); | ||
| execFileSync('brew', ['install', 'sourcekitten']); | ||
| } | ||
| } | ||
|
|
||
| if (process.argv.length < 3) { | ||
| console.warn(`Usage: ${path.relative(process.cwd(), process.argv[1])} [macOS|iOS] ...`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| ensureSourceKittenIsInstalled(); | ||
|
|
||
| let missing = false; | ||
| for (var i = 2; i < process.argv.length; i++) { | ||
| let os = process.argv[i]; | ||
| if (os == 'iOS' || os == 'macOS') { | ||
| missing |= hasMissingSymbols(os); | ||
| } else { | ||
| console.warn(`Argument must be one of iOS or macOS`); | ||
| process.exit(1); | ||
| } | ||
| } | ||
|
|
||
| if (missing) { | ||
| process.exit(1); | ||
| } else { | ||
| console.warn(`All symbols are correctly exported.`); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| // This file is generated. | ||
| // Edit platform/darwin/scripts/generate-style-code.js, then run `make style-code-darwin`. | ||
|
|
||
| #import "MGLFoundation.h" | ||
| #import "MGLStyleValue.h" | ||
| #import "MGLStyleLayer.h" | ||
|
|
||
|
|
@@ -12,6 +13,7 @@ NS_ASSUME_NONNULL_BEGIN | |
| `style` and obtain the background layer using the `-[MGLStyle layerWithIdentifier:]` | ||
| method and passing `background` for the identifier. | ||
| */ | ||
| MGL_EXPORT | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change will get blown away as soon as anyone invokes
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fixed |
||
| @interface MGLBackgroundStyleLayer : MGLStyleLayer | ||
|
|
||
| - (instancetype)initWithIdentifier:(NSString *)identifier NS_DESIGNATED_INITIALIZER; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| #pragma once | ||
|
|
||
| #import <Foundation/Foundation.h> | ||
|
|
||
| #define MGL_EXPORT __attribute__((visibility ("default"))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MGLTypes.himportsFoundation.hThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure why, but we don’t seem to be affected by realm/jazzy#609 even with these changes. Running
make xdocumentturns 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.)