Skip to content

Add cabal flag and CPP option to use Loc from monad-logger. #1076

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

Merged
merged 5 commits into from
Aug 12, 2020
Merged

Add cabal flag and CPP option to use Loc from monad-logger. #1076

merged 5 commits into from
Aug 12, 2020

Conversation

dmjio
Copy link
Contributor

@dmjio dmjio commented Apr 19, 2020

If template haskell is disabled in monad-logger, this needs to
be propagated to persistent. Otherwise, type errors ensue.

Database/Persist/Sql/Raw.hs:43:18: error:
    • Couldn't match type ‘Control.Monad.Logger.Loc’
                     with ‘Language.Haskell.TH.Syntax.Loc’
      NB: ‘Language.Haskell.TH.Syntax.Loc’
            is defined in ‘Language.Haskell.TH.Syntax’
                in package ‘template-haskell-2.14.0.0’
          ‘Control.Monad.Logger.Loc’
            is defined in ‘Control.Monad.Logger’
                in package ‘monad-logger-0.3.30’
      Expected type: Control.Monad.Logger.Loc
                     -> Control.Monad.Logger.LogSource
                     -> Control.Monad.Logger.LogLevel
                     -> fast-logger-2.4.15:System.Log.FastLogger.LogStr.LogStr
                     -> IO ()
        Actual type: LogFunc
    • In the second argument of ‘runLoggingT’, namely
        ‘(connLogFunc conn)’
      In a stmt of a 'do' block:
        runLoggingT
          (logDebugNS (pack "SQL") $ T.append sql $ pack $ "; " ++ show vals)
          (connLogFunc conn)
      In the expression:
        do runLoggingT
             (logDebugNS (pack "SQL") $ T.append sql $ pack $ "; " ++ show vals)
             (connLogFunc conn)
           getStmtConn conn sql
   |
43 |                 (connLogFunc conn)
   |                  ^^^^^^^^^^^^^^^^

Database/Persist/Sql/Raw.hs:65:10: error:
    • Couldn't match type ‘Control.Monad.Logger.Loc’
                     with ‘Language.Haskell.TH.Syntax.Loc’
      NB: ‘Language.Haskell.TH.Syntax.Loc’
            is defined in ‘Language.Haskell.TH.Syntax’
                in package ‘template-haskell-2.14.0.0’
          ‘Control.Monad.Logger.Loc’
            is defined in ‘Control.Monad.Logger’
                in package ‘monad-logger-0.3.30’
      Expected type: Control.Monad.Logger.Loc
                     -> Control.Monad.Logger.LogSource
                     -> Control.Monad.Logger.LogLevel
                     -> fast-logger-2.4.15:System.Log.FastLogger.LogStr.LogStr
                     -> IO ()
        Actual type: LogFunc
    • In the second argument of ‘runLoggingT’, namely
        ‘(connLogFunc conn)’
      In a stmt of a 'do' block:
        runLoggingT
          (logDebugNS (pack "SQL") $ T.append sql $ pack $ "; " ++ show vals)
          (connLogFunc conn)
      In the expression:
        do conn <- projectBackend `liftM` ask
           runLoggingT
             (logDebugNS (pack "SQL") $ T.append sql $ pack $ "; " ++ show vals)
             (connLogFunc conn)
           stmt <- getStmt sql
           res <- liftIO $ stmtExecute stmt vals
           ....
   |
65 |         (connLogFunc conn)
   |          ^^^^^^^^^^^^^^^^

Note: this change will not affect any code by default, only when the flag is disabled.

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@MaxGabriel
Copy link
Member

I'm not sure if I'm reading this right, but it kind of looks like Loc is always exported from monad-logger (the fake one if you have template haskell turned off, the TH one if you have it turned one). Could we instead always import Loc from monad-logger, and then we'd just get the correct version depending on what flag you have on monad-logger?

@MaxGabriel
Copy link
Member

If I'm correct about that, it should probably be documented in monad-logger that that's the intended approach

@dmjio
Copy link
Contributor Author

dmjio commented Aug 10, 2020

I'm trying to understand exactly why I did this. It does seem that monad-logger exports Loc from either template-haskell or its own defined Loc here. So it seems as if the changes I proposed would be redundant. If that were the case I would not expect the error message above to occur.

I honestly think Loc has the correct type, but for some reason GHC is not allowing the type being exported to be used since it comes from a different package. It would seem that nobody has tried to disable template_haskell in monad-logger and then use persistent with it, but I could be wrong here.

Here is a minimal reproduction (nix-build default.nix) of what I'm experiencing using the latest persistent-2.10.5.2 and monad-logger-0.3.34

# default.nix
{ pkgs ? import <nixpkgs> {} }: # nix rev 96069f7d890b90cbf4e8b4b53e15b036210ac146
with pkgs.haskell.lib;
let
  monad-logger = disableCabalFlag pkgs.haskellPackages.monad-logger "template_haskell";
  persistent = pkgs.haskellPackages.persistent.override { inherit monad-logger; };
in
  persistent

(shows above error)

@MaxGabriel
Copy link
Member

Oh, what I think the issue is is that Persistent does not import Loc from monad-logger, it always imports it from template haskell:

import Language.Haskell.TH.Syntax (Loc)

Would you be able to try Persistent always importing like this:

import Control.Monad.Logger (LogSource, LogLevel, Loc)

and seeing if that compiles for you?

dmjio added 4 commits August 11, 2020 13:26
If template haskell is disabled in monad-logger, this needs to
be propagated to persistent. Otherwise, type errors ensue.
Revert monad-logger-th changes.
@dmjio
Copy link
Contributor Author

dmjio commented Aug 11, 2020

Nice, worked for both template_haskell enabled and disabled on monad-logger. I reverted the flag stuff, and we can squash the other commits to clean up the history.

@MaxGabriel
Copy link
Member

Nice, do you mind adding a note to the changelog file about this?

@dmjio
Copy link
Contributor Author

dmjio commented Aug 12, 2020

Done.

@MaxGabriel MaxGabriel merged commit 31e25c4 into yesodweb:master Aug 12, 2020
@MaxGabriel
Copy link
Member

Thanks, merged! I also added documentation about this to monad-logger

@dmjio
Copy link
Contributor Author

dmjio commented Aug 12, 2020

Awesome, thanks a ton 🍻

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.

2 participants