Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ios): support xcframeworks in modules/platform folders #12092

Merged
merged 2 commits into from Sep 21, 2020

Conversation

sgtcoolguy
Copy link
Contributor

JIRA: https://jira.appcelerator.org/browse/TIMOB-27986

Description:
This adds support for integrating .xcframeworks in the same manner we do .frameworks for app projects.

I tested this while developing tidev/ti.barcode#134

It seems to properly integrate the ti.barcode module and the ZXingObjC.xcframework that module now includes (in it's platform folder on that PR). I fumbled through this a bit, so it may be doing some things wrong:

  • such as adding wrong/too many search paths for frameworks?
  • Assuming all xcframeworks are dynamic (tiverify.xcframework in the SDK itself isn't. I don't know the correct way to "check" for static vs dynamic xcframeworks. Just look for .framework subdirs?)
  • I didn't attempt to tackle the general issue that we (and Apple!) have long conflated architectures with arch/target combinations. This is present in our module code as well (i.e. the manifest). We can't use 'arm64' as shorthand for device arm64 anymore - there will be arm64 Apple Silicon simulators! And arm64 Apple Silicon maccatalyst targets! So I don't know what to do with the collection of architectures on xcframeworks and stripping them. I assume Xcode does that itself with xcframewokrs (they're basically pre-sliced target frameworks/binaries with multiple architectures for that target).

@build
Copy link
Contributor

build commented Sep 16, 2020

Fails
🚫 Tests have failed, see below for more information.
Warnings
⚠️ This PR has milestone set to 9.2.0, but the version defined in package.json is 9.3.0 Please either: - Update the milestone on the PR - Update the version in package.json - Hold the PR to be merged later after a release and version bump on this branch
Messages
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 1 tests have failed There are 1 tests failing and 739 skipped out of 8337 total tests.

Tests:

ClassnameNameTimeError
ios.macos.Titanium.UI.iOS.CollisionBehavior.exampleworks (10.15.4)15.096
Error: timeout of 15000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-12092/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27

New dependencies added: simple-plist.

simple-plist

Author: Joe Wollard

Description: A wrapper utility for interacting with plist data.

Homepage: https://github.com/wollardj/node-simple-plist.git

Createdalmost 7 years ago
Last Updatedabout 2 months ago
LicenseMIT
Maintainers2
Releases10
Direct Dependenciesbplist-creator, bplist-parser and plist
Keywordsplist, binary, bplist and xml

Generated by 🚫 dangerJS against aedb408

this._xobjs.PBXFileReference[fileRefUuid] = {
isa: 'PBXFileReference',
lastKnownFileType: 'wrapper.framework',
lastKnownFileType: frameworkInfo.wrapperName, // FIXME: What if framework is an xcframework?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of xcframework it should be "wrapper.xcframework" .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fixme comment can be removed, the correct file type will be returned here

@janvennemann
Copy link
Contributor

@sgtcoolguy @vijaysingh-axway i added a couple of things to this PR:

  • Parsing supported archs and platforms from Info.plist
  • Determine mach-o type from one library inside the .xcframework package. I assumed that all libraries will have the same type so i only check this one time.
  • Set platformFilter based on the parsed platforms as mentioned above by Vijay.

Copy link
Contributor Author

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't approve, only "comment", since I'm the original author 😆

Anyways, feedback is optional below. Looks good to me.

iphone/cli/hooks/frameworks.js Outdated Show resolved Hide resolved
iphone/cli/hooks/frameworks.js Outdated Show resolved Hide resolved
iphone/cli/hooks/frameworks.js Outdated Show resolved Hide resolved
iphone/cli/hooks/frameworks.js Outdated Show resolved Hide resolved
@@ -55,7 +61,12 @@ class FrameworkManager {
post: (builder, callback) => {
this._logger.trace('Starting third-party framework detection');
this._builder = builder;
this.detectFrameworks().then(callback, callback);
this.detectFrameworks().then(callback, e => {
if (this._cli.argv.platform === 'ios') {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why spit out the error stack trace only on iOS? Won't this always be true? Wouldn't we spit out the full error before?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's kinda messy (surprise!). This has been an issue on both platforms until Josh refactored the whole Android build for the Gradle support. Now the Android build manually spits out the error here. The iOS build on the other hand has no special handling and relies on the default error handling in the common builder here which only prints out the error message, effectively suppressing the error stack.

@sgtcoolguy sgtcoolguy force-pushed the TIMOB-27986 branch 2 times, most recently from 2fc9fbb to 2c83c2c Compare September 21, 2020 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants