Pass ICU to libtool in package script #7773

boundsj · 2017-01-18T20:24:37Z

In #6984 we started using ICU in core. This causes linker errors in an iOS application that uses a static build of the Mapbox iOS SDK.

This passes ICU (via Mason) to libtool when the iOS static build is created so that the dependency is available when the Mapbox iOS SDK is used in a host iOS application.

The ICU lib actually replaces libgeojson. On our build machines libgeojson was not actually getting passed to libtool since the path used was provided by {IOS_SDK_VERSION} (created with xcrun --sdk iphonesimulator|iphoneos --show-sdk-version was greater than the 8.0 value that is currently hard coded in the mason.sh script. I'm still unclear about why libgeojson was added but it does seem safe to remove it.

To attempt to start to sync up with the mason.sh file, this also adds a MASON_PLATFORM_VERSION variable to the iOS package.sh script that is now hardcoded to 8.0 just like the mason.sh script. This is required to get things to link but is fragile and I'm unclear about how this dependency should actually be handled in the various scripts. I'm not sure that hard coding this value in mason itself is a good idea.

** edit **

As noted below, we ended up using the cmake cache to find the ICU archive and to avoid having the package script know about the mason and ICU versions that are already specified in the cmake config.

cc @1ec5 @ChrisLoer @jfirebaugh

mention-bot · 2017-01-18T20:24:40Z

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

1ec5 · 2017-01-18T22:11:11Z

A minimum deployment target of 8.0 is also defined in CMake configuration for mbgl.xcodeproj. For its part, ios.xcodeproj specifies a minimum deployment target of 7.0 that applies to the static target. (The dynamic target overrides the minimum deployment target to 8.0 because iOS 7.0 doesn’t support dynamic frameworks.) This all makes me wonder whether the SDK really does support deploying back to iOS 7.0 anymore. Unfortunately, I don’t have a good way to check.

Also related: mapbox/mason#290.

/cc @kkaefer

kkaefer · 2017-01-19T10:01:01Z

Background on libgeojson is that it used to have a compiled component, but that changed in #6494

kkaefer · 2017-01-19T10:02:52Z

How is mapbox/mason#290 related to this?

@boundsj can we add a CI task that checks the static library for linking errors?

1ec5 · 2017-01-19T10:28:23Z

How is mapbox/mason#290 related to this?

Not strictly related. I just figured it was worth mentioning in the context of the binaries we’re getting from Mason.

can we add a CI task that checks the static library for linking errors?

We used to build the dynamic+static scheme on CI, but it got trimmed down as part of a push to speed up CI builds. I’d appreciate building dynamic+static again, in order to catch issues like #6194 sooner.

kkaefer

I think we should move the framework bundle creation to the Xcode stage. http://stackoverflow.com/a/16492468 has a guide as to how to do it.

kkaefer · 2017-01-19T17:41:02Z

platform/ios/scripts/package.sh

@@ -117,7 +119,7 @@ if [[ ${BUILD_FOR_DEVICE} == true ]]; then
            -o ${OUTPUT}/static/${NAME}.framework/${NAME} \
            ${LIBS[@]/#/${PRODUCTS}/${BUILDTYPE}-iphoneos/lib} \
            ${LIBS[@]/#/${PRODUCTS}/${BUILDTYPE}-iphonesimulator/lib} \
-            `find mason_packages/ios-${IOS_SDK_VERSION} -type f -name libgeojson.a`
+            `find mason_packages/ios-${MASON_PLATFORM_VERSION} -type f -name libicuuc.a`


This breaks badly if you have multiple versions of ICU in your mason_packages folder, e.g. when we upgrade ICU at some point.

kkaefer · 2017-01-19T17:41:26Z

platform/ios/scripts/package.sh

@@ -13,6 +13,8 @@ BUILDTYPE=${BUILDTYPE:-Debug}
 BUILD_FOR_DEVICE=${BUILD_DEVICE:-true}
 SYMBOLS=${SYMBOLS:-YES}

+MASON_PLATFORM_VERSION="8.0" # Deployment target version


I don't think we should duplicate the version here. Can we find another way of getting that number into here (if we need it at all)

boundsj · 2017-02-14T08:06:37Z

I think we should move the framework bundle creation to the Xcode stage. http://stackoverflow.com/a/16492468 has a guide as to how to do it.

@kkaefer thanks for the suggestion. That SO post walks through creating a Bundle configured as a relocatable object file. Although this seems to allow for more flexibility with regards to symbol hiding, I'd prefer to defer that change for now and stick to something along the lines of our existing solution.

In e9d1138, I cleaned the up package.sh so that libtool is no longer tasked with combining the SDK and libicuuc. Instead, I configured the static target to link with libicuuc with a LIBRARY_SEARCH_PATHS that points to the mason location. The next step could be to update iOS's config.cmake to make these changes. I think this would be more future proof since the cmake system is aware of the exact path to libicuuc and the version in use. For example, it writes something like mbgl_core_LINK_LIBRARIES = "/Users/boundsj/mapbox-gl-native/mason_packages/ios-8.0/icu/58.1/lib/libicuuc.a" "-lz" to the generated config.xcconfig file.

boundsj · 2017-02-14T23:11:28Z

Actually, @kkaefer @1ec5 what do you think of 1c72fc2? It reverts e9d1138 and uses libtool again to combine libicuuc in the fat binary. However, it uses cmake's cache to get the correct version of ICU.

This fixes the problem and seems pretty reliable. Maybe we can use something like this approach for now so we can close this release blocker? We can ticket additional static build system changes (if required) and a future upgrade to CI to catch this sort of issue.

1ec5 · 2017-02-15T01:24:29Z

platform/ios/ios.xcodeproj/project.pbxproj

@@ -616,6 +616,7 @@
 		40CFA6501D787579008103BD /* MGLShapeSourceTests.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; name = MGLShapeSourceTests.mm; path = ../../darwin/test/MGLShapeSourceTests.mm; sourceTree = "<group>"; };
 		40EDA1BD1CFE0D4A00D9EA68 /* MGLAnnotationContainerView.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MGLAnnotationContainerView.h; sourceTree = "<group>"; };
 		40EDA1BE1CFE0D4A00D9EA68 /* MGLAnnotationContainerView.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MGLAnnotationContainerView.m; sourceTree = "<group>"; };
+		40F066C41E52E89D002B1DB5 /* libicuuc.a */ = {isa = PBXFileReference; lastKnownFileType = archive.ar; name = libicuuc.a; path = "../../mason_packages/ios-8.0/icu/58.1/lib/libicuuc.a"; sourceTree = "<group>"; };


Is this reference still needed?

Thanks. I squashed all the previous commits into 2c9bcb78f8f7c1e1786ee641c4ff2703b8d05453 that only has the path change for libtool.

kkaefer · 2017-02-16T10:13:33Z

platform/ios/scripts/package.sh

@@ -117,7 +117,7 @@ if [[ ${BUILD_FOR_DEVICE} == true ]]; then
            -o ${OUTPUT}/static/${NAME}.framework/${NAME} \
            ${LIBS[@]/#/${PRODUCTS}/${BUILDTYPE}-iphoneos/lib} \
            ${LIBS[@]/#/${PRODUCTS}/${BUILDTYPE}-iphonesimulator/lib} \
-            `find mason_packages/ios-${IOS_SDK_VERSION} -type f -name libgeojson.a`
+            `cmake -LA -N ${DERIVED_DATA} | grep MASON_PACKAGE_icu_LIBRARIES | cut -d= -f2`


Instead of using this, we could also use MASON_PLATFORM=ios MASON_PLATFORM_VERSION=8.0 ./scripts/mason.sh LIBRARIES icu VERSION 58.1

I like looking up the value in the cmake cache because it automatically gets the correct path that (I think) is a result of mason_use(icu VERSION 58.1) and target_add_mason_package(mbgl-core PUBLIC icu) in the config.cmake file in the iOS platform directory. This way, this package.sh script does not need to know anything about the platform (although that would not be bad), the mason platform version, or the version of ICU. That can all come from the result of cmake and its config.cmake.

boundsj added build iOS Mapbox Maps SDK for iOS labels Jan 18, 2017

boundsj added this to the ios-v3.5.0 milestone Jan 18, 2017

boundsj self-assigned this Jan 18, 2017

boundsj requested a review from 1ec5 January 18, 2017 20:24

kkaefer suggested changes Jan 20, 2017

View reviewed changes

boundsj added the release blocker Blocks the next final release label Jan 24, 2017

boundsj force-pushed the boundsj-icu-in-static-build branch from 0a9d243 to e9d1138 Compare February 14, 2017 07:43

1ec5 reviewed Feb 15, 2017

View reviewed changes

boundsj force-pushed the boundsj-icu-in-static-build branch from 1c72fc2 to 2c9bcb7 Compare February 15, 2017 19:59

[ios] Search for ICU path using cmake cache

bcc2f4b

boundsj force-pushed the boundsj-icu-in-static-build branch from 2c9bcb7 to bcc2f4b Compare February 15, 2017 21:12

1ec5 approved these changes Feb 15, 2017

View reviewed changes

kkaefer reviewed Feb 16, 2017

View reviewed changes

kkaefer approved these changes Feb 16, 2017

View reviewed changes

boundsj merged commit 655b7c0 into master Feb 16, 2017

boundsj deleted the boundsj-icu-in-static-build branch February 16, 2017 18:59

boundsj mentioned this pull request Feb 17, 2017

Move static packaging script into an Xcode workflow #8089

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass ICU to libtool in package script #7773

Pass ICU to libtool in package script #7773

boundsj commented Jan 18, 2017 •

edited

Loading

mention-bot commented Jan 18, 2017

1ec5 commented Jan 18, 2017 •

edited

Loading

kkaefer commented Jan 19, 2017

kkaefer commented Jan 19, 2017

1ec5 commented Jan 19, 2017

kkaefer left a comment

kkaefer Jan 19, 2017

kkaefer Jan 19, 2017

boundsj commented Feb 14, 2017

boundsj commented Feb 14, 2017

1ec5 Feb 15, 2017

boundsj Feb 15, 2017

kkaefer Feb 16, 2017

boundsj Feb 16, 2017

Pass ICU to libtool in package script #7773

Pass ICU to libtool in package script #7773

Conversation

boundsj commented Jan 18, 2017 • edited Loading

mention-bot commented Jan 18, 2017

1ec5 commented Jan 18, 2017 • edited Loading

kkaefer commented Jan 19, 2017

kkaefer commented Jan 19, 2017

1ec5 commented Jan 19, 2017

kkaefer left a comment

Choose a reason for hiding this comment

kkaefer Jan 19, 2017

Choose a reason for hiding this comment

kkaefer Jan 19, 2017

Choose a reason for hiding this comment

boundsj commented Feb 14, 2017

boundsj commented Feb 14, 2017

1ec5 Feb 15, 2017

Choose a reason for hiding this comment

boundsj Feb 15, 2017

Choose a reason for hiding this comment

kkaefer Feb 16, 2017

Choose a reason for hiding this comment

boundsj Feb 16, 2017

Choose a reason for hiding this comment

boundsj commented Jan 18, 2017 •

edited

Loading

1ec5 commented Jan 18, 2017 •

edited

Loading