Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added ddLogLevel as extern in DDLog.h #162

Merged
merged 1 commit into from Nov 11, 2013

Conversation

Projects
None yet
5 participants
Contributor

augustj commented Nov 8, 2013

When loading CocoaLumberjack via Cocoapods, I've often had problems with duplicate symbol _ddLogLevel when a dependency also has depended upon CocoaLumberjack. My solution is to declare ddLogLevel as extern in the dependency as well as CocoaLumberjack.

Is there any disadvantage to always declaring ddLogLevel as extern in DDLog.h?

Thanks for a great library!

Member

bpoplauschi commented Nov 10, 2013

@augustj Glad you enjoy Lumberjack. I'm not sure I follow, from what I know, if you have a Podfile that refers Lumberjack directly and indirectly via another podspec, cocoapods will only download it once (since all pods are living on the same folder structure). How could you get CocoaLumberjack referenced twice?

Contributor

augustj commented Nov 10, 2013

The problem is that anything that refers to CocoaLumberjack also needs
ddLogLevel defined. If one is in Pods, and another is in the main project,
I end having to define ddLogLevel once in Pods, and then a second time in
my project. But then I have a problem of duplicate symbols. I usually end
up modifying the dependency (SVGKit in this case) to have 'extern int
ddLogLevel;' to get it to stop complaining. Since CocoaLumber expects
ddLogLevel to be defined, I thought it might be a good idea to just define
with extern in CocoaLumberjack. I would not consider my an expert in this
area, so I'd also like to learn if there is a better way to handle this.
Thanks.

On Sun, Nov 10, 2013 at 10:12 AM, Bogdan Poplauschi <
notifications@github.com> wrote:

@augustj https://github.com/augustj Glad you enjoy Lumberjack. I'm not
sure I follow, from what I know, if you have a Podfile that refers
Lumberjack directly and indirectly via another podspec, cocoapods will only
download it once (since all pods are living on the same folder structure).
How could you get CocoaLumberjack referenced twice?


Reply to this email directly or view it on GitHubhttps://github.com/robbiehanson/CocoaLumberjack/pull/162#issuecomment-28156254
.

Member

rivera-ernesto commented Nov 11, 2013

I think that just defining it as extern, just to say that there will be a ddLogLevel is a good idea.

@rivera-ernesto rivera-ernesto added a commit that referenced this pull request Nov 11, 2013

@rivera-ernesto rivera-ernesto Merge pull request #162 from augustj/add_extern_ddLogLevel
Added ddLogLevel as extern in DDLog.h
6a1f0c1

@rivera-ernesto rivera-ernesto merged commit 6a1f0c1 into CocoaLumberjack:master Nov 11, 2013

Member

rivera-ernesto commented Nov 11, 2013

Possible downside for people who like to define ddLogLevel as a const:

Xcode/Testing/TestXcodeColors/Desktop/TestXcodeColors/AppDelegate.m:6:18: Redefinition of 'ddLogLevel' with a different type: 'const int' vs 'int'

Member

rivera-ernesto commented Nov 11, 2013

And another problem with ddLogLevel being static or not...

Xcode/Testing/TestXcodeColors/Desktop/TestXcodeColors/AppDelegate.m:6:12: Static declaration of 'ddLogLevel' follows non-static declaration

I'll revert this pull request for 1.6.3 so we can better test it for 1.6.4.

I'm adding Travis CI support so we can better check pull requests (e.g. errors for this pull request here).

@rivera-ernesto rivera-ernesto added a commit that referenced this pull request Nov 11, 2013

@rivera-ernesto rivera-ernesto Revert "Added ddLogLevel as extern in DDLog.h"
This temporarily reverts commit 76114be of #162.
0f76916
Contributor

augustj commented Nov 11, 2013

I was concerned about those who wished for a static declaration. I will
keep searching for, and keep my fingers crossed for a solution that works
for the majority. Thanks again for a great library.

On Sun, Nov 10, 2013 at 6:55 PM, Ernesto Rivera notifications@github.comwrote:

And another problem with ddLogLevel being static or not...

Xcode/Testing/TestXcodeColors/Desktop/TestXcodeColors/AppDelegate.m:6:12:
Static declaration of 'ddLogLevel' follows non-static declaration

I'll revert this pull request for 1.6.3 so we can better test it for 1.6.4.


Reply to this email directly or view it on GitHubhttps://github.com/robbiehanson/CocoaLumberjack/pull/162#issuecomment-28169791
.

Member

rivera-ernesto commented Nov 11, 2013

I agree that we should keep looking into it.

adamgit commented Nov 15, 2013

Re: SVGKit - if there's something we can change in how SVGkit is using Lumberjack, so that these problems go away, we'll do it ... so long as it has no negative effects on people who aren't using Pods yet.

(we had a couple of attempts at integrating Lumberjack in a cleaner way - e.g. as an externally linked lib, with external config files - before giving up and just embedding the source. I'm not sure we understood the static definition of LOG_LEVEL properly. It seemed to be a big pain for little gain - IIRC we wanted to get rid of it, make Lumberjack behave like a normal lib, but weren't sure how to)

I use LJ on a lot of apps, where the LOG_LEVEL stuff is fine, but when integrating it into a lib, it's a lot more of a head-scratcher. I couldn't find any FAQs on this in the LJ pages, so ... when a contributor got it working as a full embed of source, I gave up and went with it :).

Member

rivera-ernesto commented Nov 18, 2013

Actually what you could do for your library is to redefine your log macros such as to refer to something else than a global ddLogLevel.

For instance:

#define SVGLogError(frmt, ...)   LOG_OBJC_MAYBE(LOG_ASYNC_ERROR,   svgLogLevel, LOG_FLAG_ERROR,   0, frmt, ##__VA_ARGS__)
// ...

And then use SVGLogXXX inside your library.

You could also let developers using your library to set this svgLogLevel along their app's ddLogLevel.

Finally you could take a look to the wrapper our company is using here.

adamgit commented Nov 19, 2013

To check I understand this correctly:

Right now, when I build the library, apps lose the ability to control log-levels of Lumberjack, right?

They are stuck with whatever level was chosen by the person who compiled the library?

...because Lumberjack is architected to only support a single app at a time?

Adding an "svgLogLevel" wouldn't fix anything, because you'd have the exact same problem: as soon as the library is compiled that variable no longer exists.

(bear in mind: most libraries are shipped as static libs, pre-compiled. Few people are compiling from source)

Contributor

augustj commented Nov 20, 2013

In order to use SVGKit as a pod, I had to roll my own SVGKit podspec that
excluded all the lumberjack files (DD*.{h,m} because I was already building
lumberjack into the libPod. The problem is that anything using lumberjack
needs ddLogLevel defined - I think that the heart of the issue. The
problem in the context of CocoaPods is that the lib with all the pods is
compiled first, followed by the application code. And since the developer
is going to want to define their own ddLogLevel, any pod using LJ won't
compile since it doesn't have ddLogLevel. I had to fork CocoaLumberjack
and define ddLogLevel as extern. But this solution won't work for people
who want it defined as static. I really don't know a way to fix this and
support those who want a static ddLogLevel.

On Tue, Nov 19, 2013 at 3:07 AM, adamgit notifications@github.com wrote:

To check I understand this correctly:

Right now, when I build the library, apps lose the ability to control
log-levels of Lumberjack, right?

They are stuck with whatever level was chosen by the person who compiled
the library?

...because Lumberjack is architected to only support a single app at a
time?

Adding an "svgLogLevel" wouldn't fix anything, because you'd have the
exact same problem: as soon as the library is compiled that variable no
longer exists.

(bear in mind: most libraries are shipped as static libs, pre-compiled.
Few people are compiling from source)


Reply to this email directly or view it on GitHubhttps://github.com/robbiehanson/CocoaLumberjack/pull/162#issuecomment-28781669
.

Member

rivera-ernesto commented Nov 20, 2013

@adamgit

Then you could use something instead of a simple variable to let users specify SVG's log level.

// Custom log macros
#define svgLogLevel     [SVGLog logLevel] // And +setLevel:
#define SVGLogError(frmt, ...)   LOG_OBJC_MAYBE(LOG_ASYNC_ERROR,   svgLogLevel, LOG_FLAG_ERROR,   0, frmt, ##__VA_ARGS__)
// ...

You can check here how a library defines its internal log macros and then allows the user to modify the levels here.

It uses a CocoaLumberjack-based log framework but you could do the same with only CocoaLumberjack.

@augustj

I think the problem is that ddLogLevel should only be used by the client app and we should add some documentation for third-party frameworks.

As of 1.6.3 this is still a problem if you're using CocoaPods. Using any pod that uses CocoaLumberjack will cause this error (in my case it was MagicalRecord).

@rivera-ernesto rivera-ernesto added a commit to rivera-ernesto/CocoaLumberjack that referenced this pull request Nov 21, 2013

@rivera-ernesto rivera-ernesto "Fix" conflicts with 3rd party libraries using CocoaLumberjack
As discussed in #162, there's no solution to sharing `ddLogLevel` between libraries and the client application:

* Apps/libraries may define `ddLogLevel` as a const, extern, int, a function returning an int, etc. Each one incompatible with the other.

* Even more, if apps/libraries used a compatible and shared `ddLogLevel` there would be no way to manage separately log levels from each one.

The solution is to define a `LOG_LEVEL_DEF` by default equal to `ddLogLevel` so apps can continue to work as usual.

3rd party libraries simple redefine `LOG_LEVEL_DEF` to something different:

```obj-c
// #undef LOG_LEVEL_DEF // Undefine first only if needed
#define LOG_LEVEL_DEF myLibLogLevel
```

3rd party frameworks **need** to be updated.
da051e4
Member

rivera-ernesto commented Nov 21, 2013

Fixed by #172.

@augustj augustj referenced this pull request in SVGKit/SVGKit Dec 6, 2013

Closed

Please Tag V1.1 for use w/ Cocoapods (if ready) #119

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment