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

Create ReactiveSwift and ReactiveSwiftTests targets #2980

Closed
wants to merge 13 commits into from
Closed

Create ReactiveSwift and ReactiveSwiftTests targets #2980

wants to merge 13 commits into from

Conversation

eimantas
Copy link
Contributor

  • Adjust import statement based on compiled target
  • Extract DynamicProperty (and its specs) to separate file and add it to mac targets
  • Swift files are still shared among ReactiveSwift and other targets

@eimantas eimantas changed the title Create ReactiveSwift and ReactiveSwiftTests target Create ReactiveSwift and ReactiveSwiftTests targets Jun 13, 2016
@mdiep
Copy link
Contributor

mdiep commented Jun 19, 2016

The ReactiveSwift target doesn't build for me. 😕

We also need watchOS, iOS, and tvOS targets. And it'd be a good idea to add these things to the build matrix on Travis. That will be large for a while, but it's only temporary.

@mdiep
Copy link
Contributor

mdiep commented Jun 19, 2016

Can you also set the xcconfigs for these targets?

configs

@eimantas
Copy link
Contributor Author

I'm not really god with configs so I defined generic "Framework" and "Application" defines for the framework and unit tests' targets appropriately.

We also need watchOS, iOS, and tvOS targets.

Do you mean "ReactiveSwift-watchOS", "ReactiveSwift-iOS" and "ReactiveSwift-tvOS"?

@eimantas
Copy link
Contributor Author

The target now builds. A couple of tests started failing (probably a fluke?).

@andersio
Copy link
Member

andersio commented Jul 2, 2016

@eimantas We need a target for each of the four OSes. The ReactiveSwift target in this branch is just for Mac.

@eimantas
Copy link
Contributor Author

eimantas commented Jul 2, 2016

Got it. I'll add the missing ones then. How would I go adding these now that the RAC5 is imminent with changes for Property? I should probably try and rebase...

@andersio
Copy link
Member

andersio commented Jul 2, 2016

You can just focus on getting these items right. Those changes would be taken care of when they are landed in master, eh, eventually. 😸

@eimantas
Copy link
Contributor Author

eimantas commented Jul 2, 2016

The targets are all there. There are few main points though:

  • I can't seem to get the ReactiveSwift-iOSTests to build properly. All good with mac and tvOS.
  • The target name in all four targets is hardcoded to ReactiveSwift as other targets use$(PROJECT_NAME) which resolves to ReactiveCocoa
  • I've added -D REACTIVE_SWIFT to Other Swift Flags in order for the swift files to correctly compile for both - ReactiveCocoa and ReactiveSwift targets.

@mdiep
Copy link
Contributor

mdiep commented Jul 5, 2016

CI didn't run for some reason. 😕

There are also some conflicts. Want to resolve them and hopefully that will trigger CI?

Eimantas Vaiciunas added 9 commits July 5, 2016 08:31
- Adjust import statement based on compiled target
- Extract `DynamicProperty` to separate file and add it to mac targets
Also update `Result` and `Nimble` submodules
- ReactiveSwift-Mac -> ReactiveSwift
- Add ReactiveSwift-iOS. Product: ReactiveSwift

Note: the "ReactiveSwift" value is hardcoded and does not come
from any build environment variable (unlike for ReactiveCocoa-X
products).
Also add `-D REACTIVE_SWIFT` flag to all configurations of `ReactiveSwift` targets
@eimantas
Copy link
Contributor Author

eimantas commented Jul 5, 2016

Seems to be working now.

@eimantas
Copy link
Contributor Author

eimantas commented Jul 5, 2016

Things still missing:

  • No xcconfig changes;
  • I am not sure how to add new targets to CI travis (probably don't have the permissions to that anyway);

@mdiep
Copy link
Contributor

mdiep commented Jul 5, 2016

I am not sure how to add new targets to CI travis (probably don't have the permissions to that anyway);

You do have permissions! Edit .travis.yml. The configuration is versioned with the repository!

buildForRunning = "YES"
buildForProfiling = "YES"
buildForArchiving = "YES"
buildForAnalyzing = "YES">
Copy link
Member

Choose a reason for hiding this comment

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

Only buildForTesting should be YES since this is a testing dependency.

name = Debug;
};
530F8B8A1D0DB70B00009C2C /* Test */ = {
isa = XCBuildConfiguration;
Copy link
Member

Choose a reason for hiding this comment

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

Please set Mac-Application xcconfig.

@mdiep
Copy link
Contributor

mdiep commented Jul 20, 2016

It looks like some of @ikesyo's notes above are still unaddressed.

Additionally, CI is failing because the upstream changes changed .travis.yml to build with both Xcode 7 and Xcode 8. This will require some changes here, but also gives us the opportunity to add Xcode 8 builds for ReactiveSwift as well.

@mdiep
Copy link
Contributor

mdiep commented Aug 16, 2016

Superseded by #3137.

@mdiep mdiep closed this Aug 16, 2016
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