-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[ios Fix static lib issues for signed device builds and iTunes connect uploads #4455
[ios Fix static lib issues for signed device builds and iTunes connect uploads #4455
Conversation
@1ec5 @friedbunny I've tested the following scenarios (all with a simple Objc client app linked to the static lib created with the proposed changes): All of this was done with Xcode 7.3. Before these changes, only first test "Build and run on simulator" passed.
|
@@ -95,8 +95,13 @@ If your application targets iOS 7.x, you’ll need to install the static framewo | |||
|
|||
1. Build from source manually per above. | |||
|
|||
1. Open the project editor and select your application target. Drag `build/ios/pkg/static/Mapbox.framework` into the “Embedded Binaries” section of the General tab. (Don’t drag it into the “Linked Frameworks and Libraries” section; Xcode will add it there automatically.) In the sheet that appears, make sure “Copy items if needed” is checked, then click Finish. | |||
|
|||
1. Copy the contents of `build/ios/pkg/static` into your project. It should happen automatically, but ensure that: |
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.
Developers have indicated that they’d like this step to be as explicit as possible, so let’s say to drag those files into the Project navigator, checking “Copy items if needed”.
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.
Thanks. Will fix.
@1ec5 I'm not sure why the node / android builds are now failing. I've tried rerunning but no luck so far. Will ignore since I'm certain this change set is unrelated to those builds. |
@@ -207,17 +222,18 @@ SEM_VERSION=$( git describe --tags --match=ios-v*.*.* --abbrev=0 | sed 's/^ios-v | |||
SHORT_VERSION=${SEM_VERSION%-*} | |||
if [[ ${BUNDLE_RESOURCES} ]]; then | |||
cp -pv LICENSE.md "${OUTPUT}/static/${NAME}.framework" | |||
cp -rv platform/ios/app/Settings.bundle "${OUTPUT}/static/${NAME}.framework" | |||
cp -rv platform/ios/app/Settings.bundle ${STATIC_SETTINGS_DIRECTORY} |
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.
We need to rethink $BUNDLE_RESOURCES
. I added this variable to support make ifabric
, which outputs an all-in-one static framework and nothing else. But now make ipackage
needs to simultaneously output a static framework, a dynamic framework, and some auxiliary resources like Settings.bundle.
Before this change, make ipackage
would place the LICENSE.md and Settings.bundle directly under pkg/, so that users of the dynamic framework can also access those resources easily. Now they’re hidden inside the pkg/static/ directory.
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.
Understood and I agree. I'll think about how to make this better. Will check with you with some ideas tomorrow.
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.
Ok, I think I'm finally with you @1ec5. So we would like something like (focusing on the static side of the fence and the files moving around in this change):
- ifabric
- pkg/static/Mapbox.framework (holds everything)
- ipackage* flavors
- pkg/
- Mapbox.bundle
- Settings.bundle
- static/
- pkg/static/
- Mapbox.framework
- pkg/
I'll adjust accordingly.
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.
More like:
- ifabric
- pkg/static/Mapbox.framework (holds everything)
- ipackage* flavors
- pkg/
- Settings.bundle
- *.md
- static/
- dynamic/
- pkg/static/
- Mapbox.framework
- Mapbox.bundle (resources in here)
- pkg/dynamic/
- Mapbox.framework (resources in here)
- Mapbox.framework.dSYM (for -symbols builds)
- pkg/
Remember to update pod-README.md, which gets copied into the build output as README.md. This is the primary place for developers to read the installation instructions. |
@jfirebaugh added a bitrise.yml to master in 231e0b5 and set up Bitrise to look for that configuration file. So that change needs to be cherry-picked into this branch; otherwise, the Node build is guaranteed to fail. Alternatively, we might be able to change the workflow script’s |
@1ec5 I'll make a note in the INSTALL doc that libc++.tbd, libsqlite3.tbd, libz.tbd are actually dylib files in older xcode versions |
Passed this test case with Xcode 6.4 and a device with iOS 8.4.1
|
d310897
to
0efc8d5
Compare
@1ec5 I took another stab at this. Please let me know if you think this will work or if a slightly larger refactor of package.sh is required. I have tested that ipackage* and ifrabric produce results as outlined above. |
1. Drag the Mapbox.bundle and Mapbox.framework files in `build/ios/pkg/static` into the Project navigator, checking "Copy items if needed". It should happen automatically, but ensure that: | ||
|
||
- `Mapbox.framework` is listed in your `Link Binary With Libraries` build phase. | ||
- The path to the project's local copy of `Mapbox.framework` is in your *Framework Search Paths* (`FRAMEWORK_SEARCH_PATHS`) build setting. |
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.
Is this accurate? FRAMEWORK_SEARCH_PATHS
is automatically set to $(inherited) $(PROJECT_DIR)
in my test project and it seems to be working just fine.
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 think the wording is just imprecise. How about:
Your *Framework Search Paths* (`FRAMEWORK_SEARCH_PATHS`) build setting includes the directory that contains `Mapbox.framework`. For most projects, the default value of `$(inherited) $(PROJECT_DIR)` should be sufficient.
Can you add a blurb about these fixes to the changelog as well? |
@1ec5 I think all issues are addressed. If there is nothing else, I'll merge after a +1. Thanks again! |
@@ -84,6 +84,7 @@ Known issues: | |||
- The Improve This Map tool now uses the same zoom level that is currently being shown in the map view. ([#4068](https://github.com/mapbox/mapbox-gl-native/pull/4068)) | |||
- Fixed a formatting issue in the documentation for `MGLCoordinateBoundsIsEmpty()`. ([#3958](https://github.com/mapbox/mapbox-gl-native/pull/3958)) | |||
- The maximum maximum zoom level is now 21. ([#4417](https://github.com/mapbox/mapbox-gl-native/pull/4417))) | |||
- Fixed issues with configuration and documentation that caused problems when installing the SDK as a static binary ([#4455](https://github.com/mapbox/mapbox-gl-native/pull/4455)) |
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.
Let’s point out that this should also fix App Store submission problems, since the installation itself appears to work without a hitch until you go to release the application.
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.
Done. Also rebased and added this blurb to the ios specific changelog.
b590411
to
b3b1b34
Compare
👍 |
539f23b
to
6cc674f
Compare
This will also fix #3656. |
Fix issues with static build configuration that caused it to be problematic for host applications when they were installed on device. Also fixes issues that broke the iTunes Connect validation and upload process. This also updates the `binary` instructions in INSTALL.md to reflect these changes.
6cc674f
to
62f636f
Compare
Add a flavor of static build that:
This also updates the
binary
instructions in INSTALL.md to reflect these changes.Fixes #4118
cc @1ec5