Integration: app targets have been separated from framework. #961

lolgear · 2018-10-16T10:14:37Z

New Pull Request Checklist

I have read and understood the CONTRIBUTING guide
I have read the Documentation
I have searched for a similar pull request in the project and found none
I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)
I have added the required tests to prove the fix/feature I am adding
I have updated the documentation (if necessary)
I have run the tests and they pass
I have run the lint and it passes (pod lib lint)

This merge request fixes / refers to the following issues: ...

Pull Request Description

App targets have been separated from framework.

Check watchOS and iOS Swift integration app targets.
Remove old integration targets.
Update travis configuration file.

UPD:
Unknown behavior in watchOS target.

ld: URGENT: building for watchOS Simulator simulator, but linking against dylib (~/Library/Developer/Xcode/DerivedData/Integration-cpyxjtzsyeaofnaanurdqqcwsezf/Build/Products/Release-iphonesimulator/CocoaLumberjackSwift.framework/CocoaLumberjackSwift) built for iOS Simulator. Note: This will be an error in the future.

codecov-io · 2018-10-16T10:35:57Z

Codecov Report

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

@@           Coverage Diff           @@
##           master     #961   +/-   ##
=======================================
  Coverage   23.87%   23.87%           
=======================================
  Files          18       18           
  Lines        2685     2685           
=======================================
  Hits          641      641           
  Misses       2044     2044

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 e97da34...2177779. Read the comment docs.

lolgear · 2018-10-16T11:51:32Z

@CocoaLumberjack/collaborators does anybody know how to setup watchOS extension with CocoaLumberjackSwift framework? ( by manual installation ).

bpoplauschi · 2018-10-16T13:18:50Z

@lolgear in #959 I created a Podfile for all the integration apps to link to the library. Why do you want to change that? There you can see the proper linking of the watch os extension.

lolgear · 2018-10-16T13:41:15Z

@bpoplauschi three points:

Zero dependencies.
Manual installation.
CI speed up.

Zero dependencies

CocoaLumberjack doesn't have any external pod spec dependency. It should not have this dependencies in case of dependencies diversity.

Manual installation.

There is no reason to install CocoaLumberjack to Integration and Tests targets by external Dependency manager. Tests and Integration are testing result framework, not external dependency.

For CocoaPods it could contain also dependency installation folder:

CocoaPods.
Carthage.
SwiftPM.
By hands.
Submodules.
Automagically.
...
etc.

CI speed up.

In case of manual installation

   # can be removed with whole "cocoapods dependencies repository"
    - pod --version
    - pod setup --silent > /dev/null
    - pod repo update --silent

lolgear · 2018-10-16T15:12:21Z

@bpoplauschi could you check my comment?

lolgear · 2018-10-16T15:19:11Z

#881

bpoplauschi · 2018-10-16T17:17:09Z

@lolgear answers inline

@bpoplauschi three points:

Zero dependencies.

Manual installation.

CI speed up.

Zero dependencies

CocoaLumberjack doesn't have any external pod spec dependency. It should not have this dependencies in case of dependencies diversity.

Manual installation.

There is no reason to install CocoaLumberjack to Integration and Tests targets by external Dependency manager. Tests and Integration are testing result framework, not external dependency.

One reason was to have a strong test of the CocoaPods installation.
But the main reason was: when I merged together the frameworks #959, you could not link to CocoaLumberjack.framework since there were 4 platform frameworks with the same name and the linker would get confused. That is why I did the CocoaPods installer.
If you can make it work without CocoaPods, be my guest :)

For CocoaPods it could contain also dependency installation folder:

CocoaPods.

Carthage.

SwiftPM.

By hands.

Submodules.

Automagically.
...

etc.

CI speed up.

In case of manual installation
   # can be removed with whole "cocoapods dependencies repository"
    - pod --version
    - pod setup --silent > /dev/null
    - pod repo update --silent

We can't remove those, as there are other projects using even other pods (for example Tests or Demos).

lolgear · 2018-10-16T17:36:08Z

@bpoplauschi

Tests should be rewritten on top of manual approach due to internal parts access.
Demos are third-party citizens. It doesn't matter how they are interact with main framework. Moreover, they should interact with main framework in different approaches ( manual, cocoapods, carthage, swift pm, etc)

Ah, I see, million podfiles in Demos directory. Nice approach. It should be rewritten in different way - just put one project with CocoaLumberjack Demos.xcworkspace.

OSSPolice · 2018-10-17T19:59:18Z

	2 Warnings
⚠️	Big PR
⚠️	The Carthage project was modified but CocoaPods podspec wasn’t. Did you forget to update the podspec?

Generated by 🚫 Danger

ffried · 2018-10-18T13:38:04Z

Yep, starting to look good from my side as well @lolgear. Thanks for your patience!

So I think the tasks left are:

Remove local_travis.yml and local_travis_run.sh (and maybe put them into a separate PR).
Remove the empty xcassets and storyboards (like @bpoplauschi proposed)
Adjust the xcconfig inheritance according to @bpoplauschi's list above
Remove some more leftover files (see comment by @sushichop)
Re-enable watchOS build in travis (as mentioned here)

Did I miss anything?

bpoplauschi · 2018-10-18T13:39:01Z

@ffried I think local_travis* files were removed.

ffried · 2018-10-18T13:41:06Z

@bpoplauschi Ah, you're right. Was looking at an old changes list, by bad!

sushichop · 2018-10-18T13:42:22Z

Just FYI.
I rebooted travis CI test because it had failed with unknown reason(I have experienced it sometimes).
Now it turns green.

lolgear · 2018-10-18T15:35:39Z

@CocoaLumberjack/collaborators
Is it fixed?

ffried · 2018-10-18T15:37:44Z

Almost...
There are still assets. Also, some Info.plists still reference some storyboards (which might no longer exist?).
Also, I think @bpoplauschi wanted to move all the xcconfigs into one directory. Is that already the case?

lolgear · 2018-10-18T15:45:32Z

@ffried
I am a bit worried about watchOS targets.
I do not know which resources could be removed in them.
For example, I suppose, that watchOS app should contain at least one storyboard. ( am I right? )

Well, configs in one directory - hmm... for what purpose?

ffried · 2018-10-18T15:49:39Z

For example, I suppose, that watchOS app should contain at least one storyboard. ( am I right? )

For the watchOS targets I agree. And I just checked. You're right, those are the only ones left. Task marked as done (with the new commit cleaning the Info.plists).

lolgear · 2018-10-18T15:51:29Z

@ffried
Could you mark checklist as "unfoldable" or "favorite" and hide all other messages?

ffried · 2018-10-18T15:51:31Z

Well, configs in one directory - hmm... for what purpose?

Mainly to keep them discoverable, I think.

It's what @bpoplauschi requested here:

I would keep them in the same ./Configs folder as the Module* ones

ffried · 2018-10-18T15:59:20Z

I've just read through the review comments and open checklists.
I'm not sure about the following comments by @bpoplauschi (about the Formatters being moved to a separate target, if I understood it correctly):
#961 (comment)
#961 (comment)
#961 (comment)

Let's wait for @bpoplauschi to have a last word about the comments I've just mentioned. Then LGTM!

Thanks again for all the patience! It's certainly a great improvement you're contributing here!

bpoplauschi · 2018-10-18T18:27:25Z

I still have minor comments, but instead of just keeping this open, I will merge it and make an additional PR.
Good job @lolgear.
Following our new pattern, we would like to invite you to become a maintainer – no pressure to accept! You can pitch in with what seems comfortable: comment on open issues/PRs, triage, improve documentation, write your own PRs. See a broader discussion here #941. Let me know if you are interested.

- removed from workspace the Pods.xcodeproj that used to live in /Framework - added Integration.xcodeproj to the workspace - fixed macOS build running issue - removed some xcconfig files from copy build phase - continued the Xcode settings cleanup - moved most of them to xcconfig - set Integration Apps WATCHOS_DEPLOYMENT_TARGET to 5.0 because of some APIs used - fixed some app ids that were incorrect - made sure we always use `DDOSLogger` instead of `DDTTYLogger` - the iOS and tvOS targets AppDelegate.swift was not creating the ViewController instance - set explicit dependencies between the Integration targets and their corresponding CocoaLumberjack dependencies - cleanup old remaining AppIcon

bpoplauschi · 2018-10-18T18:35:28Z

@ffried @sushichop thanks for the contribution

lolgear · 2018-10-18T18:50:10Z

@bpoplauschi
Yes, I am interested in becoming contributor. ( That is what I did today 😄 )

bpoplauschi · 2018-10-18T19:03:36Z

Glad to hear that. I have sent you an invitation to our organization

lolgear · 2018-10-18T19:12:21Z

@bpoplauschi
So, next step - clean up tests targets, I think.

- added Integration.xcodeproj to the workspace - fixed macOSSwiftIntegration build running issue (the AppDelegate.swift was never called - replaced with main.swift) - removed xcconfig files from copy build phase - continued the Xcode settings cleanup - moved most of them to xcconfig - set Integration Apps WATCHOS_DEPLOYMENT_TARGET to 5.0 because of some APIs used - fixed some app ids that were incorrect (org.example.* -> com.deusty.*) - the iOS and tvOS targets AppDelegate.swift was not creating the ViewController instance. Fixed - set explicit dependencies between the Integration targets and their corresponding CocoaLumberjack dependencies - cleanup old remaining AppIcon - watchOSSwiftIntegration now builds and runs (uses Debug config) - PREPROCESSOR_MACROS is actually not used by Xcode, so the DD_LOG_LEVEL=DDLogLevel* setting was not working. Changed to GCC_PREPROCESSOR_DEFINITIONS and now it works. Set DDLogLevelWarning for release and DDLogLevelAll for debug builds - only applies to Integration targets - related PRs: #961 #970

integration: app targets have been separated from framework.

868e329

lolgear changed the title ~~integration: app targets have been separated from framework.~~ Integration: app targets have been separated from framework. Oct 16, 2018

lolgear mentioned this pull request Oct 16, 2018

Demos and tests are abusing Pods relentlessly. #963

Closed

lolgear added 5 commits October 16, 2018 20:47

integration: cleanup.

2e87d7b

integration: watchos extension app has been fixed.

e268a5c

integration: cleanup iOS and watchOS apps.

5645cc4

integration: local travis has been added.

5661bfe

ci: travis has been updated.

608f3cf

lolgear added 3 commits October 17, 2018 23:05

ci: travis has been updated. missing target has been wiped out.

23cb7ca

integration: app schemes have been added.

3bc0274

ci: travis redundant pod install has been removed.

3003ae4

CocoaLumberjack deleted a comment Oct 17, 2018

ffried approved these changes Oct 18, 2018

View reviewed changes

lolgear added 5 commits October 18, 2018 17:44

integration: assets have been removed.

e077f18

integration: configurations have been updated.

42c28dc

framework: redundant podfile has been removed.

0401440

framework: pods clues have been wiped out.

8dd8987

ci: travis integration app for watchOS has been enabled.

e9c6889

integration: plists have been cleaned up.

8f14549

integration: configurations have moved to root Configuration folder.

2177779

bpoplauschi merged commit 4bf2941 into CocoaLumberjack:master Oct 18, 2018

bpoplauschi added this to the 3.5.0 milestone Oct 18, 2018

bpoplauschi mentioned this pull request Oct 18, 2018

Cleanups #965

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration: app targets have been separated from framework. #961

Integration: app targets have been separated from framework. #961

lolgear commented Oct 16, 2018 •

edited

codecov-io commented Oct 16, 2018 •

edited

lolgear commented Oct 16, 2018

bpoplauschi commented Oct 16, 2018

lolgear commented Oct 16, 2018 •

edited

lolgear commented Oct 16, 2018

lolgear commented Oct 16, 2018

bpoplauschi commented Oct 16, 2018

Zero dependencies

Manual installation.

CI speed up.

lolgear commented Oct 16, 2018

OSSPolice commented Oct 17, 2018 •

edited

ffried commented Oct 18, 2018 •

edited

bpoplauschi commented Oct 18, 2018

ffried commented Oct 18, 2018

sushichop commented Oct 18, 2018

lolgear commented Oct 18, 2018

ffried commented Oct 18, 2018

lolgear commented Oct 18, 2018

ffried commented Oct 18, 2018

lolgear commented Oct 18, 2018

ffried commented Oct 18, 2018

ffried commented Oct 18, 2018

bpoplauschi commented Oct 18, 2018

bpoplauschi commented Oct 18, 2018

lolgear commented Oct 18, 2018

bpoplauschi commented Oct 18, 2018

lolgear commented Oct 18, 2018

Integration: app targets have been separated from framework. #961

Integration: app targets have been separated from framework. #961

Conversation

lolgear commented Oct 16, 2018 • edited

New Pull Request Checklist

Pull Request Description

codecov-io commented Oct 16, 2018 • edited

Codecov Report

lolgear commented Oct 16, 2018

bpoplauschi commented Oct 16, 2018

lolgear commented Oct 16, 2018 • edited

Zero dependencies

Manual installation.

CI speed up.

lolgear commented Oct 16, 2018

lolgear commented Oct 16, 2018

bpoplauschi commented Oct 16, 2018

Zero dependencies

Manual installation.

CI speed up.

lolgear commented Oct 16, 2018

OSSPolice commented Oct 17, 2018 • edited

ffried commented Oct 18, 2018 • edited

bpoplauschi commented Oct 18, 2018

ffried commented Oct 18, 2018

sushichop commented Oct 18, 2018

lolgear commented Oct 18, 2018

ffried commented Oct 18, 2018

lolgear commented Oct 18, 2018

ffried commented Oct 18, 2018

lolgear commented Oct 18, 2018

ffried commented Oct 18, 2018

ffried commented Oct 18, 2018

bpoplauschi commented Oct 18, 2018

bpoplauschi commented Oct 18, 2018

lolgear commented Oct 18, 2018

bpoplauschi commented Oct 18, 2018

lolgear commented Oct 18, 2018

lolgear commented Oct 16, 2018 •

edited

codecov-io commented Oct 16, 2018 •

edited

lolgear commented Oct 16, 2018 •

edited

OSSPolice commented Oct 17, 2018 •

edited

ffried commented Oct 18, 2018 •

edited