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

Improve support for OS X command line tools #194

Merged
merged 8 commits into from
Jan 14, 2014

Conversation

rivera-ernesto
Copy link
Member

Following #193, in OS X linking against AppKit should be optional:

  • Avoid relying in AppKit to check OS X versions.

As for TTY logger need for NSColor, I think the solution would be to create a B/W or CLI-compatible TTY logger, which could be in turn the supper class of our current TTY logger.

Also a subspec CocoaLumbjerack/CLI would avoid compiling non-CLI-friendly code.

Please give the branch a try and if you want you can ask for permissions to commit directly to it, as I don't actually develop much for OS X at the moment.

Avoid relying in AppKit to check OS X version.
@stmontgomery
Copy link

Looks good so far, I no longer see those two issues I mentioned at the top of #193 related to NSApp/NSApplication.

Now the only failure is in DDTTYLogger due to not finding NSColor. The changes needed to break that hard dependency will likely be a lot more significant I'm betting, and I'm thinking I'd prefer to actually use DDTTYLogger (with color) more than I'm dreading having to link in AppKit (since the color formatting stuff looks awesome).

So as far as I'm concerned I'm fine with just applying the changes you have here so that at least DDLog is no longer AppKit-dependent, meanwhile DDTTYLogger can continue requiring NSColor and hence AppKit. If I change my mind later I suppose I can just implement a custom TTY logger class that doesn't use NSColor. Thoughts? (And thanks for entertaining this idea in the first place)

s.subspec 'CLI' do |ss|
ss.dependency 'CocoaLumberjack/Core'
ss.source_files = 'Lumberjack/CLI/*.{h,m}'
end
Copy link
Member Author

Choose a reason for hiding this comment

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

New subspec for OS X projects that don't use AppKit.

@rivera-ernesto
Copy link
Member Author

Please @stmontgomery give this branch a try.

@@ -32,3 +32,6 @@ script:
- xctool -project Xcode/BenchmarkIPhone/BenchmarkIPhone.xcodeproj -scheme 'BenchmarkIPhone' -configuration Release -sdk iphonesimulator -arch i386 build
- xctool -project Xcode/Testing/TestXcodeColors/Desktop/TestXcodeColors.xcodeproj -scheme 'TestXcodeColors'
- xctool -project Xcode/UniversalApp/UniversalApp.xcodeproj -scheme 'UniversalApp' -configuration Release -sdk iphonesimulator -arch i386 build
- cd Xcode/CLI
- pod install
- xctool -workspace CLI.xcworkspace -scheme 'CLI'
Copy link
Member Author

Choose a reason for hiding this comment

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

Made sure not to break other usages.

@stmontgomery
Copy link

This looks great so far, I've been on holiday but planning to try out this branch again soon

@rivera-ernesto
Copy link
Member Author

I'm on holidays too ;)

@stmontgomery
Copy link

Apologies for the delay, but this looks great! I was able to build against this branch cleanly and I successfully un-linked AppKit.framework from OS X framework and no longer rely on NSColor. So I think this is good to go, thanks again!

@bpoplauschi
Copy link
Member

In this case, we could merge the pull request. @rivera-ernesto should I merge it?

@rivera-ernesto
Copy link
Member Author

Cool. I've just came back from holidays as well. I think this is good to merge then.

@dvor
Copy link
Member

dvor commented Jan 14, 2014

@rivera-ernesto great improvement! I'll merge it.

dvor added a commit that referenced this pull request Jan 14, 2014
Improve support for OS X command line tools
@dvor dvor merged commit 621898b into CocoaLumberjack:master Jan 14, 2014
@rivera-ernesto rivera-ernesto deleted the better_cl_support branch January 15, 2014 01:14
@bpoplauschi bpoplauschi added this to the 1.8.0 milestone Feb 18, 2014
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