Fix logic for running cabal directly.

[mdorman]

To see the failure, start a fresh copy of emacs, load a .cabal file, M-x
haskell-process-cabal-build; while the logic in haskell-process-do-cabal
suggests that that should not require an interactive process, it
complains that there is no interactive process.

I suspect this has been broken for a while (perhaps always). The
failure stems from the fact that (haskell-interactive-process) throws an
uncaught error if there is no interacive process, which aborts the
function. If that is wrapped to mask the error, (process-status child)
also throws an uncaught error, which aborts the function.

Thus the direct cabal invocation code would never be reached.

This is the least intrusive fix I can see---wrapping both calls
with (ignore-errors), and not even checking (process-status) if we don't
have a process. In manual testing it behaves as expected, running
cabal build.

A more extensive fix---which would rewrite the conditional logic to wrap
all the dangerous bits in a single (ignore-errors)---would almost
certainly swap positions of the indirect cabal invocation and the direct
cabal invocation, so there would be much more churn. Still, it might
produce a nicer overall result, if anyone wants to see that.

[gracjan]

I do not think I am able to follow... anybody else understands if this is good pr or bad pr?

@mdorman: Are you able to create an automated test in our testsuite to catch this issue? That would help a lot.

[mdorman]

@gracjan I assume the test environment doesn't have cabal installed?

[gracjan]

No, it does not.

[mdorman]

I think this test demonstrates the problem, as well as demonstrating that my fix resolves it.

[fice-t]

Hmm, I think ignore-errors should be used sparingly. If you just want to wrap it around haskell-interactive-process why not instead change the error it throws into a warning/message and have that function return nil instead of erroring?

[mdorman]

@fice-t my change isn't only wrapping haskell-interactive-process, it's also wrapping process-status, which we don't have the luxury of changing.

Still, while I don't think your suggestion is unreasonable from an engineering standpoint, it would cause a lot of churn for very little real value: look at all the callers of haskell-interactive-process---none of them check the return value, presumably because they expect it to throw an error if there's nothing present.

So I'm not inclined to go down that road.

The road I would consider if people preferred it, would be to restructure haskell-process-do-cabal to have all of its machinations to get the process information wrapped in a single ignore-errors call---so you end up with something like:

(let ((process (ignore-errors (...))))
(if process
(use-the-interactive-process)
(run-cabal-directly))

But as I alluded to in my initial message, that ends up restructuring that function significantly---and that seemed a bit aggressive to do for my first ever PR to the project.

[fice-t]

Ah, I see. Looks like it'd be a bunch of work to fix that.

In that case, does there need to be an ignore-errors around process-status if you're checking for the nil process beforehand?

[mdorman]

@fice-t I don't have enough perspective to say; it's not clear to me that haskell-process-process can never return something that would cause process-status to throw an error?

[gracjan]

Nice test, this is about what we want, to see that haskell-process-do-cabal does something.

There is a nice way to intercept deep calls with el-mock. We use it for example here:

haskell-mode/tests/haskell-process-tests.el

Line 31 in fe2cce7

(mocklet (((haskell-session-name "dummy-session") => "dumses1"))

Use mocklet to make sure execution path you want it really taken. Thanks!

[mdorman]

So I removed the probably-superfluous ignore-errors that @fice-t pointed out. If at some point it truly proves necessary, we can deal with it then.

I also rewrote the test to verify that shell-command is getting called with exactly the expected command line.

[gracjan]

Thanks!