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

Update project structure for simplicity and fixing carthage #1422

Merged
merged 22 commits into from
Sep 17, 2016

Conversation

pmairoldi
Copy link
Collaborator

A while ago a came across a post that greatly simplifies how many targets we need for multiple platforms. So basically the removes the need for a different target for each platform so now there is only 2, Charts and ChartsRealm.

This change address two issues having to do with Carthage (#1326 and #1119).

This first change is removing the prebuilt binaries inside of the ChartsRealm project and replacing them with ones that come from Carthage when running "carthage bootstrap" or "carthage update". There has been a lot talk in #1119 about the possible solutions and to me this seems to be the best for now. This means that contributors wanting to update the ChartsRealm project will have to have Carthage install and run "carthage bootstrap" to get the Realm dependencies. A note in the CONTRIBUTING.md should be added for newcomers.

The second change sort of goes hand in hand with the first. It changes how ChartsRealm requires Charts.framework in order to work with Carthage. Having the two targets inside the same project and having a Carthage.xcconfig telling carthage where to look for dependencies seems to address the Carthage issues that have been seen.

I believe this addresses most of the Carthage issues. It seems to work consistently now. And it is now more streamlined, 1 target for all platforms!

@pmairoldi
Copy link
Collaborator Author

@danielgindi, @liuxuan30 what do you guys think? I like this a lot and it makes contributing much easier. 1 project, 1 target, simple.

@liuxuan30
Copy link
Member

liuxuan30 commented Sep 9, 2016

to confirm, I just check out the branch, and ChartsDemo needs to run "carthage bootstrap" first to get Realm and continue compile right?
I simply tried to build Realm and warns me no Realm framework

@pmairoldi
Copy link
Collaborator Author

ya anything realm based will need a carthage bootstrap. everything else should work as before

@liuxuan30
Copy link
Member

guess let @danielgindi review the structures.
BTW, seeing old directories like Charts/classes/Charts/ is empty, actually they are in source/. Do we need to delete the empty directories?

@pmairoldi
Copy link
Collaborator Author

run git clean -xfd they will be gone. You just have old stuff

@liuxuan30
Copy link
Member

git clean work on my side, thanks

@Cramsden
Copy link

Is this pull request going to be accepted? Would love to be able to use Charts without Realm dependancies.

@danielgindi
Copy link
Collaborator

@petester42
I apologize, being busy with other stuff I didn't look into this branch.
Any chance that you rebase it on master?

Also - will people be able to build the project without Carthage?
Many developers seem to be unable to read simple instructions, or open an Xcode project and hitting "Run", so I want to avoid all those "Issues" from being created in the first place ;-)

@pmairoldi
Copy link
Collaborator Author

Ya will do no problem.

As far as just hitting run, for Charts its fine. For ChartsRealm there is a carthage dependency and as discussed in #1119 there is no real good solution. So contributors will have to run carthage bootstrap to contribute to ChartsRealm. Seeing as ChartsRealm is just a data layer implementation I think its ok for now.

I might create scripts in the future to simplify the process so the contributor will just run an init script and have the project setup automatically done for them.

@pmairoldi pmairoldi mentioned this pull request Sep 17, 2016
@pmairoldi
Copy link
Collaborator Author

I'm gonna merge this to make the swift 3.0 version ready.

@pmairoldi pmairoldi merged commit a540403 into master Sep 17, 2016
@pmairoldi pmairoldi deleted the workspacing branch September 17, 2016 15:32
@danielgindi
Copy link
Collaborator

👍
Do you want to update the README to tell people how to handle this?

@danielgindi
Copy link
Collaborator

By the way, Realm should point to master branch, not to a release, as only master support Swift 3.0

@liuxuan30
Copy link
Member

liuxuan30 commented Sep 20, 2016

Realm 1.1.0 should be Swift 3.0 now?

v1.1.0
@realm-ci realm-ci released this 3 days ago · 2 commits to master since this release
This release brings official support for Xcode 8, Swift 2.3 and Swift 3.0.
Prebuilt frameworks are now built with Xcode 7.3.1 and Xcode 8.0.

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

Successfully merging this pull request may close these issues.

None yet

4 participants