Skip to content
This repository

MagicalRecord doesn't want to use CocoaLumberjack logging #598

Closed
Zyphrax opened this Issue · 27 comments

8 participants

Yvo van Beek Fabio Pelosin Joseph Heenan Saul Mora Eloy Durán Stephen Vanterpool Michael Bachand Marcin Krzyzanowski
Yvo van Beek

I posted this issue at MagicalRecord (magicalpanda/MagicalRecord#277) but I'm starting to think this is a CocoaPods issue.

When I combine the MagicalRecord pod with the CocoaLumberjack pod, MagicalRecord should start using CocoaLumberjack for logging. However this doesn't seem to work, and MagicalRecord will fall back to using NSLog.

MagicalRecord is detecting CocoaLumberjack in MagicalRecord.h:

#if MR_ENABLE_ACTIVE_RECORD_LOGGING != 0
      // First, check if we can use Cocoalumberjack for logging
    #ifdef LOG_VERBOSE
        extern int ddLogLevel;
        #define MRLog(...)  DDLogVerbose(__VA_ARGS__)
    #else
        #define MRLog(...) NSLog(@"a %s(%p) %@", __PRETTY_FUNCTION__, self, [NSString stringWithFormat:__VA_ARGS__])
    #endif
#else
    #define MRLog(...) ((void)0)
#endif

However the MagicalRecord podspec causes this to be added to Pods-prefix.ch:

#ifdef __OBJC__
#define MR_SHORTHAND
#import "CoreData+MagicalRecord.h"
#endif

The Pods-prefix.ch doesn't have an include for DDLog.h, so LOG_VERBOSE isn't defined and MagicalRecord will default back to NSLog.

My AppName-Prefix.ch looks like this:

// Pod: Lumberjack logging
#import <CocoaLumberjack/DDLog.h>

// Pod: Magical Record
#define MR_ENABLE_ACTIVE_RECORD_LOGGING 1
#define MR_SHORTHAND 1
#import <MagicalRecord/CoreData+MagicalRecord.h>

How do the Pods-prefix.ch and AppName-Prefix.ch influence each other?
How can we get MagicalRecord and CocoaLumberjack to work together?

My workaround for now is to manually add import "DDLog.h" above the MagicalRecord include in Pods-prefix.ch. Of course I prefer not to edit the Pods project :)

Fabio Pelosin

@Zyphrax please join the discussion in #539.

Joseph Heenan

I've run into this same issue too - #539 seems to be closed now, is just that the MagicalRecord pod now needs to be updated as per the comments on #539?

Yvo van Beek

I've created a post_install script for now (for CocoaPods 0.17):

# Modify Pods-prefix.pch, so that MagicalRecord will use CocoaLumberjack
post_install do | installer |
    file = File.open("#{installer.sandbox_root}/Pods-prefix.pch", "r+")
    lines = file.readlines
    file.rewind
    lines.each do | line |
        if line.include? "CoreData+MagicalRecord.h"
            file.puts "#import \"DDLog.h\""
        end
        file.puts line
    end
end

I don't have a lot of experience with Ruby, suggestions for doing this more efficiently are welcome :)
Perhaps it would be better if the MagicalRecord pod could somehow detect the CocoaLumberjack pod.

Yvo van Beek

@alloy / @irrationalfab
Do you guys know a better way to solve this?

Basically when you use the MagicalRecord pod and CocoaLumberjack together, CocoaLumberjack (DDLog.h) should be included above MagicalRecord in the PCH.

Fabio Pelosin
Owner

@Zyphrax a first step in that direction is discussed in issue #833. That would be still a manual approach though. There is also an experimental feature currently shipping in CocoaPods which would allow automatic integration. If you open a installation generated with CocoaPods > 0.17. You'll notice that in the Pods project there is a file called Pods-header.h. The contents of this file should be similar to:

// WARNING: This feature of CocoaPods is present for discussion purposes and might be discontinued or changed in future
#define __COCOA_PODS

#define __POD_MagicalRecord
#define __POD_CocoaLumberjack

As this file is being imported in the prefix header a Pod like magical record could do something like the following in its source:

#ifdef __POD_CocoaLumberjack
  #import "DDLog.h"
#end

And the automatic integration would kick in. However this feature is experimental mostly because I haven't received enough feedback about it and I don't know if this is the best approach.

Yvo van Beek

@irrationalfab the Pods-header.h would give a lot of flexibility in putting together your own Pods-Prefix.pch.

In this case it would be unfortunate that everyone who combines MagicalRecord and CocoaLumberjack will have to change their Pods-header.h to make this work. I think it should be the responsibility of the MagicalRecord pod to detect the CocoaLumberjack pod.

Perhaps another way to solve this would be to do the following:

  • Pod PCH directives (prefix_header_contents) are added to Pods-Prefix.pch in the order that pods are defined in the Podfile
  • Next to prefix_header_contents there will be an extra directive "prefix_header_contents2" (we have to figure out a proper name for this). The contents of this directive will be added to the Pods-Prefix.pch after all the Pods are processed

So for example:

  • Pod B - prefix_header_contents: "B", prefix_headers_contents2: "Y"
  • Pod A - prefix_header_contents: "A", prefix_headers_contents2: "X"
  • Pod C - prefix_header_contents: "C"

Would result in:

  • B
  • A
  • C
  • Y
  • X

This way MagicalRecord could set up the logging in prefix_headers_contents2 and properly detect CocoaLumberjack.

Fabio Pelosin
Owner

In this case it would be unfortunate that everyone who combines MagicalRecord and CocoaLumberjack will have to change their Pods-header.h to make this work. I think it should be the responsibility of the MagicalRecord pod to detect the CocoaLumberjack pod.

To be clear, the Pods header is generated automatically and the above example is intended to be included in the source code of MagicalRecord. Therefore, I'm not following your point that everyone will have to change their Pods-header.h.

Perhaps another way to solve this would be to do the following:
This way MagicalRecord could set up the logging in prefix_headers_contents2 and properly detect CocoaLumberjack.

In my opinion, Pods should not use the prefix header for more than the strictly necessary. I don't see why the contents of the prefix_headers_contents2 can't go in the MagicalRecord source.

Regarding the order aware logic, you are not the first one to propose it. Personally, I don't like the idea. I think that keeping into account the order of declaration of the Pods, is brittle, unexpected to the users, could lead to difficulties in a hierarchy of target definitions and would be complex to maintain. Currently the order of declaration can influence the resolution process but this is a band-aid because our resolver is still naive.

Yvo van Beek

@irrationalfab Ah sorry, I completely misunderstood the purpose of Pods-header.h. The Pods-header would work fine if it is always included at the top of the Pods-Prefix.pch and MagicalRecord would indeed change MagicalRecord.h. I do wonder if MagicalRecord and other libraries are willing to add lines like these to their code (which is completely irrelevant for people who don't use CocoaPods).

I think we need some kind of way to define the order of the imports in de Pods-Prefix.pch. The order of the pods in de PodFile might not be the best way, I support your argument on that. Perhaps some kind of template file, where you can say: these are my imports, import CocoaLumberjack here, import MagicalRecord there. Basically what I thought Pods-header.h was for :)

Fabio Pelosin
Owner

I do wonder if MagicalRecord and other libraries are willing to add lines like these to their code (which is completely irrelevant for people who don't use CocoaPods).

This would be up to the library maintainer, akin to the inclusion of the podspec in t the root of the repo. If they want to provide automatic auto detection they can. Also the nature of the macro makes the code harmless for non CP environments and generally would provide useful functionality which the lib can expose with another macro.

Perhaps some kind of template file, where you can say: these are my imports, import CocoaLumberjack here, import MagicalRecord there. Basically what I thought Pods-header.h was for :)

If I understand you correctly this is #833 is about. CocoaPods would create a header for each target and not overwrite it during install. This file would be imported in the prefix. In this way the user has still a chance to manually affect the build environment. Same for the Xcconfigs.

Fabio Pelosin
Owner

@casademora, @tonyarnold It would be great if you could share your opinion about this feature. In detail it would be helpful to know:

  • Whether you would implement such a functionality in MagicalRecord.
  • Whether you think that the implementation can be improved (even if you wouldn't implement it).
  • Any alternative implementation which you would suggest for an automatic of optional components like CocoaLumberjack (the less specific to CocoaPods the better).
Fabio Pelosin
Owner

The gist is described in this comment.

Saul Mora
Eloy Durán
Owner
alloy commented

@casademora Yeah, but the point is that with CP the source is not imported into the user’s project, so unless you hack the source, you can’t set that before compilation. This ticket is about changing the way MR detects the default. So instead of:

#if MR_ENABLE_ACTIVE_RECORD_LOGGING != 0
      // First, check if we can use Cocoalumberjack for logging
    #ifdef LOG_VERBOSE
        extern int ddLogLevel;
        #define MRLog(...)  DDLogVerbose(__VA_ARGS__)
    #else
        // [SNIP]
    #endif
#endif

It would be:

#if MR_ENABLE_ACTIVE_RECORD_LOGGING != 0
      // First, check if we can use Cocoalumberjack for logging
    #ifdef LOG_VERBOSE || __POD_CocoaLumberjack
        #import "DDLog.h"
        extern int ddLogLevel;
        #define MRLog(...)  DDLogVerbose(__VA_ARGS__)
    #else
        // [SNIP]
    #endif
#endif
Saul Mora
Eloy Durán
Owner
alloy commented

@casademora Awesome. This was untested code btw, so maybe it’s best if we create a PR for MR that we have verified to work as intended.

Fabio Pelosin
Owner

@casademora Thanks!
@Zyphrax Given that you already have a good test project could you fork MR and apply @alloy's patch and test that it actually works as expected? The support from the CP side is already there. We just need to graduate the feature.

Eloy Durán
Owner
alloy commented

And then there’s the pod version issue, which, after much discussion, we settled on solving like this: https://gist.github.com/alloy/5346435.

Basically, if you follow semver, we will define version macros, otherwise we don’t.

Fabio Pelosin irrationalfab referenced this issue in AFNetworking/AFNetworking
Closed

Add support for COCOAPODS macro #905

Yvo van Beek

Great!

@irrationalfab Sure I'll try this out later this week

@casademora or @tonyarnold Could you accept my existing MR pull request (magicalpanda/MagicalRecord#432)? MR currently has a method that performs two fetches instead of one, because its calling the wrong method. After that I can create a fresh fork of MR.

Yvo van Beek

Sorry about the delay.
I've tried @alloy 's patch but it had a small syntax issue. I got it working with the following code:

#if MR_ENABLE_ACTIVE_RECORD_LOGGING != 0
      // First, check if we can use Cocoalumberjack for logging
    #if defined(LOG_VERBOSE) || defined(__POD_CocoaLumberjack)
        #import "DDLog.h"
        extern int ddLogLevel;
        #define MRLog(...)  DDLogVerbose(__VA_ARGS__)
    #else
        // [SNIP]
#endif

That works great. I'll submit the change to the development branch of MagicalRecord.
@casademora @irrationalfab fyi

Yvo van Beek Zyphrax referenced this issue in magicalpanda/MagicalRecord
Closed

Added CocoaLumberjack detection through CocoaPods macro #463

Stephen Vanterpool

I'm working on merging it into MR but i can't get it working locally. Where should __POD_CocoaLumberjack be defined?

Yvo van Beek

They've changed the definition format in the latest version of CocoaPods. The updated code is:

#if MR_ENABLE_ACTIVE_RECORD_LOGGING != 0
    // First, check if we can use Cocoalumberjack for logging
    #if defined(LOG_VERBOSE) || defined(COCOAPODS_POD_AVAILABLE_CocoaLumberjack)
        #import "DDLog.h"
        extern int ddLogLevel;
        #define MRLog(...)  DDLogVerbose(__VA_ARGS__)
    #else
        // [SNIP]
#endif

So compared to the original MR code, the following has changed:

  • ifdef LOG_VERBOSE became if defined LOGVERBOSE || COCOAPODS_POD_AVAILABLE etc.
  • DDLog.h is imported above the ddLogLevel line

CocoaPods pod definitions are listed in Pods-environment.h in the Pods project.

Stephen Vanterpool

Ok. I'm almost there. I have it compiling, and I use Lumberjack all over this project I'm testing it with (and have various ddLogLevels set), however MRLog still isn't working. Still digging.

Michael Bachand

As of the time of writing, it looks like this is still not fixed in the latest MagicalRecord release (2.2), although the change does appear to be in master.

For an example of what I believe is an improved solution over what is in MagicalRecord master, check out this commit on our MagicalRecord fork, which causes MagicalRecord to integrate with CocoaLumberjack and also adds log levels.

Saul Mora
Saul Mora

I've recently created a 3.0 version branch of magical record that should have better support for logging in general. If you're up for it, give it a try and let me known how the logging works for you. It should be a bit more automatic depending on if you have a debug build or not. There are now 5 log levels, verbose, info, warn, error and fatal. You can see the levels and the method to enable them in the MagicalRecord+Settings.h file.

Marcin Krzyzanowski

I know it's closed but I found this issue when trying to figure out how to add #define to all my Pods, and this is what I came with: https://gist.github.com/krzak/7690635

Modyfying Pods-environment.h did the trick. I'm not sure if this is the best, but works for me.

post_install do | installer |
  open("#{installer.sandbox_root}/Pods-environment.h","a") do |file|
    file.puts <<EOF
// Disable logs
#ifndef DEBUG
  #define NSLog(...)
#endif
EOF
  end
end
Fabio Pelosin

@krzak Interesting technique, note that this could break in the future if we implement #1396.

Raphael Schaad raphaelschaad referenced this issue in Flipboard/FLAnimatedImage
Open

Log using CocoaLumberjack, if available #1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.