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

Not actually Carthage compatible #85

Closed
lilyball opened this issue Sep 26, 2016 · 26 comments
Closed

Not actually Carthage compatible #85

lilyball opened this issue Sep 26, 2016 · 26 comments

Comments

@lilyball
Copy link

Your README has a "Carthage compatible" badge, but it's not. You're providing a prebuilt framework download that works with Carthage, but if I run Carthage with the --no-use-binaries flag (which disables the use of prebuilt frameworks), this repo cannot be built by Carthage. And the reason it can't be built is because this repo doesn't provide a project file that can be used to build it.

@briancaw
Copy link
Contributor

HI @kballard

Thanks for reporting this.

Can you let us know what command this is failing for? If it's build, checkout, or update, could you instead specify the names of the libraries you want to build from source (space separated)? i.e. Those commands allow space separated lists of dependencies to apply the action to.

Thanks,
Brian

@lilyball
Copy link
Author

@briancaw Specifying the dependency name isn't going to change anything. This repo simply is not Carthage-compatible. You don't have a project file. There's no possible way you can be Carthage-compatible if you don't have a project file (and shared schemes within that project). The specific error Carthage gives is

*** Skipped building appboy-ios-sdk due to the error:
Dependency "appboy-ios-sdk" has no shared framework schemes for any of the platforms: watchOS, iOS

If you want to reproduce this yourself, you need to pass the --no-use-binaries flag to Carthage, or you can checkout this repo and run carthage build --no-skip-current --platform iOS from within the repo to get the same error.

@briancaw
Copy link
Contributor

briancaw commented Sep 26, 2016

Hi @kballard,

I'm wondering if you're able to do, for example:

carthage update "dep1" "dep2" --no-use-binaries
carthage update "Appboy-iOS-SDK"

such that you don't apply that flag to the update of our library.

If that's not feasible for you it would be great to know what's blocking things so we can strategize the if/what/when of an update. Apologies for the confusion on Carthage compatibility - the documentation on their github doesn't make entirely clear that our support wouldn't be considered actual support (and indeed several other folks have used our library with Carthage successfully).

Thanks,
Brian

@lilyball
Copy link
Author

update is the wrong command for building, that would be either bootstrap (which does fetch, checkout, and build) or build. In any case, while it is possible to specify all of the dependencies manually like that, that makes it significantly harder to actually work with Carthage because we can no longer just say carthage bootstrap. And it still doesn't make this repo qualify as Carthage-compatible.

And besides, we actually want to build Appboy from source. Right now we're getting

ld: warning: Some object files have incompatible Objective-C category definitions. Some category metadata may be lost. All files containing Objective-C categories should be built using the same compiler.

and at this point I've tested with building everything else from source except for Appboy. The only way to fix this warning is to build all dependencies (that have Obj-C categories) with the same version of the compiler that's used to build the app.

the documentation on their github doesn't make entirely clear that our support wouldn't be considered actual support

Yes it does. The documentation states you need shared Xcode schemes, and also describes how to test with carthage build --no-skip-current. This repo doesn't have shared Xcode schemes, because it doesn't have an Xcode project at all, and it fails the carthage build --no-skip-current test.

@lilyball
Copy link
Author

Hmm, I just noticed this repo has a file libAppboyKitLibrary.a, which presumably means that building from source won't help anyway >_<

@briancaw
Copy link
Contributor

Hi again @kballard,

Indeed we don't publish source at this time so you won't be able to build from source. Our roadmap does include open-sourcing elements of our SDK (notably our UI - see #44), but we do not have plans to open source the entirety of our SDK in the very near future. I apologize for the inconvenience.

Regarding advertising Carthage compatibility - I've updated our README to broadcast the specifics of our support (i.e. that we support via prebuilt framework).

Please let me know if you have any other comments/questions/concerns, and thanks again for reporting this.

Brian

@lilyball
Copy link
Author

@briancaw

Thank you for updating the README to be more accurate. An even better fix would be to add an xcodeproj to this repo that is set up with a shared scheme that produces the expected framework. Even though it wouldn't actually be building from source but would be using the prebuilt static library, this would allow Carthage users to be able to use Appboy when they specify --no-use-binaries, which a number of projects do (especially now with the transition to Xcode 8 and Swift 3, where a lot of prebuilt binaries aren't usable).

@briancaw
Copy link
Contributor

Hi @kballard

Thanks for that recommendation - we had considered that when initially doing Carthage support but avoided it because 1) it wasn't clear the prebuilt framework option was as limiting as you describe and 2) it introduces a bit of a maintenance burden on our end. That said, we always aim to have our SDK be as useable as possible and we're happy to reconsider that solution.

We can't commit to a timeline right now, but I'll update this ticket when we've decided next steps.

Thanks,
Brian

@dmiluski
Copy link

dmiluski commented Sep 19, 2017

Came here to second this ^ Not Carthage compatible.

Sadly, I reported it would be an easy install to my product team after reviewing the AppBoy page, and was surprised when I found out not Carthage compatible. I'm embarrassed I mentioned that before attempting integration.

As a team (Venmo), we include Carthage dependencies via:
carthage update --no-build --use-ssh --use-submodules

Which pulled in this project, but with the lack of project/framework to drop in.. not sure about next steps, as we have stopped including cocoa pod dependencies.

I'm not sure what you're worried about open sourcing your client code if it requires your platform to function? Linking to the Carthage dependencies you mention comes free by including your own cart file.

Without it, we may consider manual integration... but that leads to problems ensuring versions are up to date and properly labeled.

@nickrobin
Copy link

nickrobin commented Sep 21, 2017

Hi @dmiluski

Thank you for reaching out. In order to address your specific concerns as related to this account I will reach out to you directly to discuss further offline as to not flood the comments here. Please keep an eye out for an email to follow!

@jeffctown
Copy link

jeffctown commented Oct 26, 2017

@nickrobin I ran into the same issue, but it was an error in Carthage v0.25. Updating to 0.26.2 works fine.

Jeffs-MacBook-Pro:cnnios jefflett$ carthage build --platform iOS appboy-ios-sdk
*** xcodebuild output can be found in /var/folders/fz/gs89nbjj1s32lxkq2k3sbc480000gn/T/carthage-xcodebuild.m7mZ7Q.log
*** Skipped building appboy-ios-sdk due to the error:
Dependency "appboy-ios-sdk" has no shared framework schemes for any of the platforms: iOS

If you believe this to be an error, please file an issue with the maintainers at https://github.com/appboy/appboy-ios-sdk/issues/new
Jeffs-MacBook-Pro:cnnios jefflett$ carthage version
0.25.0

@Wenzhi
Copy link

Wenzhi commented Oct 26, 2017

Hey @jeffctown,

Apologies for the incompatibilities that you’re experiencing. Our Carthage support is currently limited to providing our SDK as a prebuilt framework, something we call out in our README. We’re actively working to reconsider our approach, and we appreciate all the public feedback.

In addition, our Customer Success and Support teams will be reaching out to you directly to enable a more in-depth conversation around your current build system and dependency requirements. Thanks for your engagement here—our team depends on these sorts of interfaces to build a great client-facing product!

Best,
Wenzhi

@marcvanolmen
Copy link

marcvanolmen commented Nov 7, 2017

hey Appboy Support

have you seen how Crashlytics and Carthage is done, they provide framework (static one in their case) and take advantage of the binary specifier in the Cartfile

Then the other 3rd party dependencies needed to be added.

For example:

binary "https://raw.githubusercontent.com/Appboy/Specs/master/Carthage/Appboy-iOS.json"
github "Flipboard/FLAnimatedImage"       
github "rs/SDWebImage"

The only work for Appboy is to maintain the spec file https://raw.githubusercontent.com/Appboy/Specs/master/Carthage/Appboy-iOS.json
that keeps track of the version.

More details here:

Carthage/Carthage#2036

bu the spec file above for Crashlytics looks like this:

{
  "3.9.3": "https://kit-downloads.fabric.io/ios/com.twitter.crashlytics.ios/3.9.3/com.twitter.crashlytics.ios-default.zip",
  "3.9.0": "https://kit-downloads.fabric.io/ios/com.twitter.crashlytics.ios/3.9.0/com.twitter.crashlytics.ios-default.zip",
  "3.8.6": "https://kit-downloads.fabric.io/ios/com.twitter.crashlytics.ios/3.8.6/com.twitter.crashlytics.ios-default.zip",
  "3.8.5": "https://kit-downloads.fabric.io/ios/com.twitter.crashlytics.ios/3.8.5/com.twitter.crashlytics.ios-default.zip",
  "3.8.4": "https://kit-downloads.fabric.io/ios/com.twitter.crashlytics.ios/3.8.4/com.twitter.crashlytics.ios-default.zip"
}

these zip files contains just the framework of Crashlytics.

@dmiluski
Copy link

dmiluski commented Nov 7, 2017 via email

@dmiluski
Copy link

dmiluski commented Nov 7, 2017

@marcvanolmen I didn't realize there was a "binary" Carthage prefix. I'll experiment with this.

As for FLAnimatedImage and SDWebImage, I believe earlier I ran into an issue as SDWebImage duplicates classes that are in FLAnimatedImage? As part of the AppBoy podspec, the spec actually strips out a couple components of one to get it to function?

@dmiluski
Copy link

dmiluski commented Nov 7, 2017

@marcvanolmen any chance this could be drive by GitHub releases? Wondering if the separately managed json endpoint is scalable?

@dmiluski
Copy link

dmiluski commented Nov 7, 2017

But this looks potentially doable. @appboyci any chance of supporting this by offering a hosted hosted json file to reference?

@mnvr
Copy link

mnvr commented Dec 29, 2017

We use SDWebImage 4.2 in our project, and thus we cannot use the Appboy-iOS-SDK as it stands. Newer versions of SDWebImage integrate the FLAnimatedImage code (if we use the Carthage version), and thus we end up with duplicate code if we use the FLAnimatedImage bundled with the Appboy SDK.

These are the steps I had to do to integrate it with our project:

  • Download the Appboy-iOS-SDK release.
  • From the zip file, copy only Appboy_iOS_SDK.framework and add it to our project.
  • At runtime, the Appboy_iOS_SDK.framework goes looking for @rpath/SDWebImage.framework/SDWebImage and @rpath/FLAnimatedImage.framework/FLAnimatedImage. The former is present in my app bundle (since we use SDWebImage) but the latter is not (since, as I mention, this code now lives in SDWebImage itself). To work around this, I create an empty framework project called FLAnimatedImage, build it using carthage build --platform ios --no-skip-current, and the embed the resulting FLAnimatedImage.framework into the app as part of the carthage copy-frameworks step.

Needless to say, this is very hacky.

Please provide a variant of your SDK that works for projects that are using Carthage, and are also using SDWebImage 4.x already in their code. The suggestion by @marcvanolmen (#85 (comment)) appears to be a good and easy enough approach to get started on.

@Nonnus
Copy link

Nonnus commented Apr 4, 2018

is there any solution about this? we were just bitten by the same issue and are trying to find any way to keep building our app without temporarily removing appboy or sdwebimage

@Nonnus
Copy link

Nonnus commented Apr 4, 2018

by the way, until now we were able to avoid this problem by using SDWebImage version 4.1.0 but that versions is having too many issues and we needed to update it so we are now faced with the FLAnimatedImage collision

@artxj
Copy link
Contributor

artxj commented May 2, 2018

Hello,

We have updated the Braze SDK, so now you can include only thin framework plus its dependencies by adding the following lines in your Cartfile:

binary "https://raw.githubusercontent.com/Appboy/appboy-ios-sdk/master/appboy_ios_sdk.json"
github "rs/SDWebImage"
github "Flipboard/FLAnimatedImage"

Please let us know if it doesn't work for you. Thanks again for pointing this out!

@artxj artxj closed this as completed May 2, 2018
mnvr added a commit to coretech/mparticle-apple-integration-appboy that referenced this issue May 23, 2018
The default Appboy SDK bundles SDWebImage and FLAnimatedImage which
conflicts with our existing uses of those frameworks:

Appboy/appboy-ios-sdk#85 (comment)

So we make the integration point to the thin variant of Appboy using
the instruction at:

Appboy/appboy-ios-sdk#85 (comment)
mnvr added a commit to coretech/mparticle-apple-integration-appboy that referenced this issue May 23, 2018
The default Appboy SDK bundles SDWebImage and FLAnimatedImage which
conflicts with our existing uses of those frameworks:

Appboy/appboy-ios-sdk#85 (comment)

So we make the integration point to the thin variant of Appboy using
the instruction at:

Appboy/appboy-ios-sdk#85 (comment)
@mnvr
Copy link

mnvr commented May 23, 2018

Thank you for progress on this 👍🏼

Unfortunately this still doesn't work for us, because the Appboy SDK directly uses FLAnimatedImage
instead of using the copy of FLAnimatedImage that comes with SDWebImage (which happens by
default, if one uses Carthage, and AFAIK, is not overridable).

i.e., the Appboy SDK uses

#import <FLAnimatedImage/FLAnimatedImage.h>

However, if we install SDWebImage via Carthage, then the FLAnimatedImage
that is linked with our code is:

#import <SDWebImage/FLAnimatedImage.h>

Note that we cannot work around this by adding a direct reference to the FLAnimatedImage
framework in our Cartfile because that leads to duplicate symbols (and the associated
compile/runtime errors).

So for now, we still have to use the workaround outlined in #85 (comment)

@dmiluski
Copy link

dmiluski commented May 23, 2018

I'm generally confused why this package uses both SDWebImage + FLAnimatedImage, given SDWebImage now supports GIFs by adding the GIF decoder included in their code.

+        SDWebImageCodersManager.sharedInstance().addCoder(SDWebImageGIFCoder.shared())

Given it's decoded, and the image is animatable:
imageView.startAnimating()

@artxj
Copy link
Contributor

artxj commented May 23, 2018

Hi,

Thank you for letting us know and apologies for the incompatibilities you’re experiencing!

We believe it's an SDWebImage + Carthage package manager issue that leads to FLAnimatedImage sources to be put inside of SDWebImage framework instead of being added as a dependency (see SDWebImage/SDWebImage#2279)

Unfortunately we can't simply change #import <FLAnimatedImage/FLAnimatedImage.h> in our code with #import <SDWebImage/FLAnimatedImage.h> because the SDK still requires FLAnimatedImage to be linked, otherwise it won't work via Cocoapods integration.

Also, duplicate symbols when adding a reference to FLAnimatedImage in the Cartfile shouldn't lead to any compile/runtime errors (warnings only) if you use same FLAnimatedImage versions in SDWebImage and as separate framework.

As soon as SDWebImage replaces FLAnimatedImage with its own implementation SDWebImage/SDWebImage#2140, we'll update the SDK to solve this issue.

Best, Artem

@dmiluski
Copy link

@artxj , what do you use FLAnimatedImage for that SDWebImage UIImage does not offer?

@artxj
Copy link
Contributor

artxj commented May 24, 2018

We've been using FLAnimatedImage for GIF animation (this issue arose only in SDWebImage v.4 when it started bundling FLAnimatedImage).

We're investigating the option of dropping FLAnimatedImage in favor of using SDWebImage GIF coder - we'll update as we make progress there.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants