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

Run pod install on Example #41

Merged
merged 1 commit into from Oct 28, 2018
Merged

Run pod install on Example #41

merged 1 commit into from Oct 28, 2018

Conversation

sbarow
Copy link
Contributor

@sbarow sbarow commented Oct 28, 2018

Example.xcworkspace is generated by pod install so there is no need to check it in. Removing this allows carthage install to work again with Swift 4.2

If Carthage detects that there is a .workspace then it will try build that instead of ContextMenu.xcodeproj.

@rnystrom
Copy link
Member

Won’t this require a pod install to get examples working now? I’d like the repo to all work just by downloading.

Sent with GitHawk

@sbarow
Copy link
Contributor Author

sbarow commented Oct 28, 2018

@rnystrom yeah it would, best practise is not to checkin .xcworkspace & Pods folders.

@rnystrom
Copy link
Member

@sbarow any reason why? All of our repos in this org check in all assets so someone doesn’t need CocoaPods to get working. Projects are just there and working.

Why does this need removed for Carthage?

Sent with GitHawk

@sbarow
Copy link
Contributor Author

sbarow commented Oct 28, 2018

Main reason is they can be generated, checking them in will lead to merge conflicts and other issues down the line, its an unnecessary pain point for engineers - this generally outweighs people having to have cocoapods pods installed and running pod install

I believe the issue why Carthage isn't able to build this is that its detecting a .xcworkspace and trying to build ContextMenu from the workspace scheme, the problem is when the 4.2 update was done I don't think pod install was run on the example directory, so the workspace is still technically building the 4.0 version.

Another reason not to checkin generated code ;-)

I can update the diff with a fix for the cocoa pods issue if thats how you'd prefer to keep it.

@rnystrom
Copy link
Member

rnystrom commented Oct 28, 2018

Ya if rather keep this repo inline with the others instead of introduce another maintenance model. If were gonna do this, wed want to do it to everything (and I’m not convinced it matters after maintaining large OSS work with checked-in Pods for 3 years).

IMO let’s just do the minimal fix to make Cartage work.

Sent with GitHawk

@sbarow
Copy link
Contributor Author

sbarow commented Oct 28, 2018

Sounds good, just ran pod install & reverted all the other changes.

@rnystrom
Copy link
Member

Thank you!

Sent with GitHawk

@rnystrom rnystrom merged commit d6757be into GitHawkApp:master Oct 28, 2018
@sbarow sbarow changed the title Remove Example.xcworkspace Run pod install on Example Oct 28, 2018
@telip007
Copy link

telip007 commented Nov 5, 2018

Carthage still not working. Getting error code 65. @sbarow @rnystrom

@telip007
Copy link

telip007 commented Nov 5, 2018

Ah sorry, used the latest release but it does not include this change :) So with this commit it works. 👍

@rnystrom
Copy link
Member

rnystrom commented Nov 5, 2018

Will update!

Sent with GitHawk

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

Successfully merging this pull request may close these issues.

None yet

3 participants