Skip to content

Comments

Remove copy frameworks script#930

Merged
frederoni merged 1 commit intomasterfrom
fred-rem-cp-frameworks
Dec 14, 2017
Merged

Remove copy frameworks script#930
frederoni merged 1 commit intomasterfrom
fred-rem-cp-frameworks

Conversation

@frederoni
Copy link
Contributor

@frederoni frederoni commented Dec 14, 2017 •
edited
Loading

Fixes #927 avoid nested dependencies

@1ec5 @bsudekum

@frederoni frederoni added build Issues related to builds and dependency management. ⚠️ DO NOT MERGE PR should not be merged! labels Dec 14, 2017
@frederoni frederoni self-assigned this Dec 14, 2017
@frederoni frederoni removed the ⚠️ DO NOT MERGE PR should not be merged! label Dec 14, 2017
@frederoni frederoni merged commit 08b729e into master Dec 14, 2017
@frederoni frederoni deleted the fred-rem-cp-frameworks branch December 14, 2017 16:41
1ec5
1ec5 reviewed Dec 15, 2017
View reviewed changes
MapboxNavigation.xcodeproj/project.pbxproj
"$(SRCROOT)/Carthage/Build/iOS/AWSPolly.framework",
"$(SRCROOT)/Carthage/Build/iOS/AWSCore.framework",
"$(SRCROOT)/Carthage/Build/iOS/Turf.framework",
"$(SRCROOT)/Carthage/Build/iOS/Solar.framework",
Copy link
Contributor

@1ec5 1ec5 Dec 15, 2017

Choose a reason for hiding this comment

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

Does the developer need to copy these dependencies in their own application now? If so, we should update the installation instructions – this is a significant change, given the number of dependencies.

Copy link
Contributor Author

@frederoni frederoni Dec 15, 2017

Choose a reason for hiding this comment

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

Yes, but this has always been a requirement when using Carthage, and it's stated in their documentation when building for iOS, tvOS, or watchOS.

IIRC, I added this phase because we were unable to test frameworks without an application target but this doesn't seem to be an issue anymore.

Copy link
Contributor

@1ec5 1ec5 Dec 15, 2017

Choose a reason for hiding this comment

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

Yeah, I’m just thinking we should list these dependencies in the readme so developers know which ones are for the navigation SDK. It isn’t obvious that this SDK depends on AWSCore, for example.

@Sealos Sealos mentioned this pull request Jan 23, 2018
Closed
@1ec5 1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Dec 18, 2018
@1ec5 1ec5 added this to the v0.12.0 milestone Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@1ec5 1ec5 1ec5 left review comments

@bsudekum bsudekum bsudekum approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@frederoni frederoni

Labels

backwards incompatible changes that break backwards compatibility of public API build Issues related to builds and dependency management.

Projects

None yet

Milestone

v0.12.0

Development

Successfully merging this pull request may close these issues.

3 participants

@frederoni @bsudekum @1ec5