Skip to content

Update hackage-security to GHC 8.8 #234

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

Closed
wants to merge 2 commits into from

Conversation

Avi-D-coder
Copy link

This goes on top of #232

@Avi-D-coder
Copy link
Author

Is MonadFail the right approach?

@@ -335,6 +338,8 @@ cachedVersion CachedInfo{..} remoteFile =
getCachedInfo ::
#if __GLASGOW_HASKELL__ < 800
(Applicative m, MonadIO m)
#elif __GLASGOW_HASKELL__ >= 807
(MonadFail ReadJSON_Keys_Layout, MonadIO m)
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed this seems rather odd. I haven't looked at the surrounding code but it does seem suspicious that you would need to add a constraint of this form. Are you sure this is necessary?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not clear how MonadFail can be replaced, it seems to come from getCachedInfo:347 and ReadJSON_Keys_Layout comes from readLocalFile : 367.

Should I try to replace MonadFail with a Throws _?

Copy link
Author

Choose a reason for hiding this comment

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

My mtl foo is not good enough to remove MonadFail ReadJSON_Keys_Layout.

@edsko I see you wrote the ReadJSON_Keys_Layout code, do you have any advice?

Copy link
Author

Choose a reason for hiding this comment

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

THe cloest I have gotten is MonadError DeserializationError IO, but this produces the warning:

Security/Client.hs:211:18: warning:
     Couldn't match type IOException with DeserializationError
        arising from a functional dependency between:
          constraint MonadError DeserializationError IO
            arising from the type signature for:
                           checkForUpdates :: forall (down :: * -> *).
                                              (MonadError DeserializationError IO,
                                               Throws VerificationError, Throws SomeRemoteError) =>
                                              Repository down -> Maybe UTCTime -> IO HasUpdates
          instance MonadError IOException IO at <no location info>
      Inaccessible code in
        a pattern with constructor: FGz :: Format FormatGz,
        in a case alternative
     In the pattern: FGz
      In the pattern: Some FGz
      In a case alternative:
          Some FGz -> verifyFileInfo' (Just newInfoTarGz) targetPath tempPath

@@ -68,6 +68,7 @@ instance MonadReader RepoLayout m => ToJSON m Snapshot where

instance ( MonadReader RepoLayout m
, MonadError DeserializationError m
, MonadFail m
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case I would probably try to use the MonadError constraint to instead report a more precise error. For instance, define locally:

let lookupFile path =
      withExceptT DeserializationErrorSchema 
      $ liftEither $ FileMap.lookupM snapshotMeta path

and use this in place of FIleMap.lookupM below.

Copy link
Author

@Avi-D-coder Avi-D-coder Nov 1, 2019

Choose a reason for hiding this comment

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

I was not able to make this work (Either does not implement MonadFail) so I just wrote:

lookupErr :: MonadError e m => (String -> e) -> FileMap -> TargetPath -> m FileInfo

@hvr
Copy link
Member

hvr commented Nov 2, 2019

@Avi-D-coder to be honest, I'm utterly confused by this PR; is this really against master? Also this seems to address an issue that has long been resolved by 471a7b8, no?

@@ -99,8 +99,8 @@ library
base16-bytestring >= 0.1.1 && < 0.2,
base64-bytestring >= 1.0 && < 1.1,
bytestring >= 0.9 && < 0.11,
Cabal >= 1.14 && < 1.26,
Copy link
Member

Choose a reason for hiding this comment

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

This line here makes me suspect this is against some fairly ancient state of the master branch

@bgamari
Copy link
Contributor

bgamari commented Nov 2, 2019

Hmm, indeed I am also a bit confused; master already appears to build with GHC 8.8.1. @Avi-D-coder, is there a particular error that you were seeing?

@Avi-D-coder
Copy link
Author

Avi-D-coder commented Nov 2, 2019

This was based on #232

@Avi-D-coder Avi-D-coder closed this Nov 2, 2019
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.

3 participants