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

Verify uses of exceptionFree in ExecuteSessionUpdate #252

Open
edsko opened this issue Sep 10, 2014 · 11 comments
Open

Verify uses of exceptionFree in ExecuteSessionUpdate #252

edsko opened this issue Sep 10, 2014 · 11 comments

Comments

@edsko
Copy link
Collaborator

edsko commented Sep 10, 2014

See #251 for details.

Some of these should be verified by me, others by @Mikolaj .

@edsko
Copy link
Collaborator Author

edsko commented Sep 10, 2014

Actually, buildExe and co also throw some exceptions, so they are definitely not exception free. This is probably not a good idea, because it exits the session update immediately. We may have to fix this (not high priority, however).

@Mikolaj
Copy link
Collaborator

Mikolaj commented Sep 10, 2014

I think buildExe should not throw exceptions. I even catch some and print them to the stderr log file. But it wouldn't hurt double checking, especially for the other tools (I've just caught and fixed that in haddock generation a couple days ago).

@Mikolaj
Copy link
Collaborator

Mikolaj commented Sep 12, 2014

Actually, I only catch IOExceptions in buildExe. So, if something really bad happens, an exception can get through. If needed, I can catch all exceptions and log them in the stderr log file.

@edsko
Copy link
Collaborator Author

edsko commented Sep 12, 2014

Aren't there some calls to "error" in there somewhere? Or "fail"? Or both?

@Mikolaj
Copy link
Collaborator

Mikolaj commented Sep 12, 2014

Countless (in the tools' codebase).

@edsko
Copy link
Collaborator Author

edsko commented Sep 12, 2014

No, that's not what I mean. In executeBuildExe itself:

        errors = case toLazyMaybe mcomputed of
          Nothing ->
            error "This session state does not admit artifact generation."
          Just Computed{computedErrors} -> toLazyList computedErrors

and

    when (not configGenerateModInfo) $
      -- TODO: replace the check with an inspection of state component (#87)
      fail "Features using cabal API require configGenerateModInfo, currently (#86)."

@Mikolaj
Copy link
Collaborator

Mikolaj commented Sep 12, 2014

I must have missed the context. I call many of the tools as libraries, so the exceptions from 'error' calls get through and not as ExitStatus exceptions (which I catch).

@edsko
Copy link
Collaborator Author

edsko commented Sep 12, 2014

I'm confused. I'm saying that executeBuildExe itself throws exceptions, independent of whatever library it is using.

@Mikolaj
Copy link
Collaborator

Mikolaj commented Sep 12, 2014

I guess my point was we may eliminate those, but we can't (easily) do that in libraries. That's why I didn't guess you mean our code exclusively.

@edsko
Copy link
Collaborator Author

edsko commented Sep 12, 2014

Well, we need to catch exceptions thrown by the library, although yes, of course, deeply nested calls to error in pure values somewhere could potentially be a problem -- although we don't actually get any pure values returned by these library calls, do we? So that's not something we should be worrying about?

It just seems rather counter-productive to try and catch exceptions thrown by the libraries, because we don't want to interrupt the execution of the session update, but then throw some exceptions of our own?

@Mikolaj
Copy link
Collaborator

Mikolaj commented Sep 13, 2014

Another one is here (called in executeBuildExe)

buildDeps mcomputed = do
  case toLazyMaybe mcomputed of
    Nothing -> fail "This session state does not admit artifact generation."
    ...

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

No branches or pull requests

2 participants