Conversation
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.
Approach LGTM, but would be great to make it as simple as possible.
TARGETED_DEVICE_FAMILY = "1,2"; | ||
VERSIONING_SYSTEM = "apple-generic"; | ||
VERSION_INFO_PREFIX = ""; | ||
}; |
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.
Could you remove the unnecessary build settings set here? For example, no need for Metal and Swift related options.
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.
👍🏼 These are just the defaults generated by Xcode, but I'll remove the unnecessary ones.
|
||
//! Project version string for Shimmer. | ||
FOUNDATION_EXPORT const unsigned char ShimmerVersionString[]; | ||
|
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.
It doesn't look like you're defining these anywhere; remove them if they aren't set?
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.
This is what Xcode generates automatically. I always presumed there was a reason for it, but to be honest I don't know
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.
Ah, maybe this is paired with the versioning support file Xcode also generates. Either way, I don't see a good reason for it, so I'd be fine with disabling the versioning support in Xcode and removing it here.
Support Carthage by adding a framework scheme and target.
@grp I made the requested changes squashed them into a single commit. |
@grp anything else that needs to happen for this to get merged? |
@grp the other two PRs adding this can be closed to probably. |
@klaaspieter : there is still issue wth Carthage and the issue has not been fixed ! |
Support Carthage by adding a framework scheme and target.
I'm aware there are two PRs (#53 and #55) open for this but they appear to be dead and have merge conflicts.