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

🐛 Fixes #46 Carthage build #47

Merged
merged 3 commits into from
Jul 10, 2017
Merged

🐛 Fixes #46 Carthage build #47

merged 3 commits into from
Jul 10, 2017

Conversation

esttorhe
Copy link
Member

@esttorhe esttorhe commented Jul 9, 2017

Due to a misconfiguration in the project settings when building using Carthage was failing because the project was unable to find RxSwift dependencies

Due to a misconfiguration in the project settings when building using
Carthage was failing because the project was unable to find RxSwift
dependencies
@esttorhe esttorhe self-assigned this Jul 9, 2017
@esttorhe esttorhe requested a review from freak4pc July 9, 2017 19:14
@esttorheBot
Copy link

esttorheBot commented Jul 9, 2017

Generated by 🚫 danger

@@ -6,7 +6,7 @@
// Copyright © 2016 Esteban Torres. All rights reserved.
//

#import <UIKit/UIKit.h>
#import <Foundation/Foundation.h>
Copy link
Member

Choose a reason for hiding this comment

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

What was the reason for changing the import here? Supporting multiple targets? Do we even need any of these imports for this to work? Rest looks great !

Copy link
Member Author

Choose a reason for hiding this comment

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

Including UIKit seemed illogical given that the framework supports all 4 platforms and after the changes I made the macOS target wasn't compiling

Copy link
Member

Choose a reason for hiding this comment

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

Yeah makes total sense, I just wondered if we even need the Foundation import there to begin with or it would probably work without?

@freak4pc
Copy link
Member

freak4pc commented Jul 9, 2017

Very interesting yet probably not related to this PR, from the tvOS failed CI on Travis

fatal error: Warning: Recursive call or synchronization error!: file /Users/travis/build/RxSwiftCommunity/RxViewModel/Demo/Pods/RxSwift/RxSwift/Observables/Implementations/AnonymousObservable.swift, line 27

Worth looking into at some point @esttorhe

Need to double check if this is present on `RxSwift`
@codecov-io
Copy link

codecov-io commented Jul 10, 2017

Codecov Report

Merging #47 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #47   +/-   ##
=======================================
  Coverage   86.82%   86.82%           
=======================================
  Files           5        5           
  Lines         258      258           
=======================================
  Hits          224      224           
  Misses         34       34

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fc9da6...92c3541. Read the comment docs.

@freak4pc
Copy link
Member

I don't know the CLANG_WARNIG_INFINITE_RECURSION flag but the fact it was disabled doesn't just silences the warning instead of taking care of the root cause? Or that would be a separate ticket?

@esttorhe
Copy link
Member Author

@freak4pc I left the flag as it was originally for now.

As stated on the commit message I need to dig deeper and see how/if RxSwift has this flag on because the error is happening deep into RxSwift and not on RxViewModel level which means that first we would have to patch/fix it there; then turn the flag back on here and update with the latest version.

@freak4pc
Copy link
Member

@esttorhe AFAIK that recursion error is a warning within RxSwift about code using RxSwift, e.g. it does seem like it is something within RxViewModel but its interesting it only occurred when building on tvOS. If I have a few minutes later this week I'll try building it with the flag and see if I can find anything funky :)

@esttorhe
Copy link
Member Author

@freak4pc you might be right :)

I'm going to check this week and see if I can find the root cause of the issue (given that Travis is refusing to load that log for me)

Thanks 😄

Bumped `RxSwift` to latest version and re-enabled the redundancy check
flag
@esttorhe
Copy link
Member Author

Looks like its passing (for now) going to merge and will keep monitoring this issue.

Sadly TravisCI refuses to serve me the failed log and thus makes it quite hard for me at the moment to try and fix.

Will keep try locally but i'm going to merge this now

@esttorhe esttorhe merged commit e0518bc into master Jul 10, 2017
@esttorhe esttorhe deleted the fix/46 branch July 10, 2017 12:38
@freak4pc
Copy link
Member

Sounds good. I'll also try keeping an eye for this issue.
And yeah, Travis is horrid. I still keep hoping everyone will move their OSS projects to Buddybuild 👼

@esttorhe esttorhe mentioned this pull request Jul 10, 2017
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.

4 participants