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

Consider calls to buildExe and co after RPC exception #253

Closed
edsko opened this issue Sep 10, 2014 · 3 comments
Closed

Consider calls to buildExe and co after RPC exception #253

edsko opened this issue Sep 10, 2014 · 3 comments

Comments

@edsko
Copy link
Collaborator

edsko commented Sep 10, 2014

The calls to buildExe and co happen at the end of the execution of a session update. Before #251 (comment) this meant that they would not be called if a remote exception happened (that is, if the ghc server died for some reason). Now that we've changed how we do the RPC calls, however, this is no longer true, and buildExe and co will still be called.

This makes sense, and is consistent: we want the local updates to happen independent of the remote updates. However, we need to verify that buildExe and co don't depend on the remote state; and in fact, it seems that they do. buildExe for instance checks Computed, which would have been filled with some dummy values if a remote exception in fact did happen. We need to think about this and perhaps modify the execution of the session update to take this into account (and have buildExe report an appropriate error, possibly) -- and if we do, add a test for it.

@edsko edsko added this to the Version 0.9 milestone Sep 10, 2014
@Mikolaj
Copy link
Collaborator

Mikolaj commented Sep 13, 2014

The checks of session state that buildExe does are listed in
#252 (comment)
and
#252 (comment)

So we'd need to mark the state somehow to reflect there was an error and so Computed is dummy. Previously Computed was set to Nothing in such cases or its computedErrors contained at least one error (not a warning). Is it changed? If not, buildExe should be fine. I'd just check the other cabal-invoked tools.

@edsko
Copy link
Collaborator Author

edsko commented Sep 13, 2014

@Mikolaj I've now changed it so that the dummy value for Computed will include one source error ("GHC server died (dummy error)"). If you think that is sufficient we can close this issue.

edsko added a commit that referenced this issue Sep 13, 2014
@Mikolaj
Copy link
Collaborator

Mikolaj commented Sep 13, 2014

Thank you, that is enough for buildExe. For consistency and to avoid user surprise, I've added similar tests for errors to buildDoc and executeBuildLicenses, because otherwise they'd generate empty documentation/licenses (in case of RPC exception) or a subset (with source errors, misleading). I didn't modify buildDotCabal, because it was specified as a pure function and so I can't just set ExitStatus to Failure when Computed contains an error, even if it comes from a complete external disaster, not just a type error.

@Mikolaj Mikolaj closed this as completed Sep 13, 2014
Mikolaj added a commit that referenced this issue Sep 13, 2014
and only refuse building it in case of "GHC server died (dummy error)".
We are in no position to decide if partial documentation should be built
or not. Let the user of our API decide --- he has access to all relevant
information, in particular, to errors.
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