Inhibit errors from terminal-size

[bradrn]

Closes #4901.

[dbaynard]

Code looks ok, I'm getting somebody more familiar with semantics to take a look, too.

[bradrn]

@dbaynard Have you had any progress on this PR yet?

[dbaynard]

No — thanks for the reminder! I've just checked through, myself (including your terminal-size PR) and so I think the only thing remaining — which I'm seeking to confirm now — is the addition of the silently dependency. Which should be fine… hopefully this can be merged soon. Thanks for your patience, too!

[dbaynard]

@bradrn Have you seen hspec/silently#7? Seems like this may introduce that issue, to stack.

[lehins]

@bradrn I'd recommend just getting the fixed version of the windows module from your PR https://github.com/bradrn/terminal-size/blob/591033712003b58f8c61dba0fc60e95439df9c9b/src/System/Console/Terminal/Windows.hs copied directly into stack with some CPP on windows. Until and if the PR biegunka/terminal-size#13 gets merged (I wouldn't get hopes up)

[bradrn]

@dbaynard Thanks for responding! I had a look at the silently issue you linked, but I don’t see how it relates to this issue.

(Also, I’m curious to know: why does the addition of a dependency require approval?)

@lehins I’d prefer to avoid copying the code over — that’s a massive amount of code duplication between stack and terminal-size, and I’d prefer to have all the code in terminal-size in case the maintainer makes any further improvements. (Although I have to agree with you that this seems unlikely at this point.)

[lehins]

@bradrn hSilence [stderr] will capture all output going to stderr for all threads. Do you think it is a problem now? ;) I think @dbaynard meant to link to this issue: hspec/silently#6

I had a look at the silently issue you linked, but I don’t see how it relates to this issue.

Totally agree with you, I'd like to avoid it as well whenever possible. Only reason I suggested such a work around is because terminal-size doesn't seem to be getting much action, so getting your PR merged might take some time.

I’d prefer to avoid copying the code over ...

[lehins]

I’m curious to know: why does the addition of a dependency require approval?

@bradrn By adding a dependency you are bringing it's whole code base with it in one PR. Even if you use one function from that package, that function might have problems. Unless it is a vetted package we'd need to do a thorough review, not only of the function that you use, but the package as whole. Just think about how many people use stack and what sort of impact could be if that dependency had serious bugs or even worse security problems.

[bradrn]

@bradrn hSilence [stderr] will capture all output going to stderr for all threads. Do you think it is a problem now? ;) I think @dbaynard meant to link to this issue: hspec/silently#6

That certainly is a big issue! I don’t think this is nearly as good of a solution now…

@bradrn By adding a dependency you are bringing it's whole code base with it in one PR. Even if you use one function from that package, that function might have problems. Unless it is a vetted package we'd need to do a thorough review, not only of the function that you use, but the package as whole. Just think about how many people use stack and what sort of impact could be if that dependency had serious bugs or even worse security problems.

That does make sense.

[bradrn]

Looks like progress on this PR has slowed down a bit, but after thinking about it I think I have a solution. I was beginning to think that @lehins was right in saying that the code from terminal-size should be copied into Stack, but I think I have a better idea: commercial-haskell (or some other trusted party) could fork biegunka/terminal-size, apply my PR, then add an extra-dep on that fork rather than the main terminal-size. (Or my fork could be used, but I doubt I would qualify as a ‘trusted party’.) This has the advantage of reducing code duplication, as well as ensuring that any future changes to terminal-size can still be incorporated into stack via the fork.

[lehins]

... [commercialhaskell] could fork biegunka/terminal-size, apply my PR, then add an extra-dep on that fork ...

@bradrn Unfortunately that solution has a problem. In order to be able to upload stack on hackage the new terminal-size fork has to also be uploaded to hackage (with some other name), which means commercialhaskell has to maintain yet another package. This solution has too high of an overhead, comparing to just copying one function.

[bradrn]

In order to be able to upload stack on hackage

I didn’t realise that Stack was on Hackage! There goes another attempted solution…

This solution has too high of an overhead, comparing to just copying one function.

But it isn’t just one function! It’s two functions (for Posix and Windows), plus all the functions they use, plus all the functions they use, plus all the imports, plus all the CPP for choosing between OSs

…which is only ~150 lines, now that I check. Maybe this is more feasible than I thought!

[lehins]

It’s two functions (for Posix and Windows)

I thought the issue was on Windows only? If that is still the case than all that would be necessary is to copy the System.Console.Terminal.Windows module over to src/windows/ folder in stack, rename it to System.Console.Terminal.Size, fix it and add it to package.yaml. While posix version could still be used from terminal-size package, granted it is till correct, by adding another module to src/unix: System.Console.Terminal.Size that simply re-exports System.Console.Terminal.

[bradrn]

I thought the issue was on Windows only?

This is a bit complex:

1. As far as I know, this particular issue has been reproduced on Windows only. However, I can see it happening on Posix as well, in cases where ioctl errors out. It would be more accurate to say that the proposed fix is Windows-only.
2. Looking more broadly, the terminal-size dependency was only introduced in Terminal width detection isn't done on Windows #3588, to fix a Windows-specific issue. So it may be possible to simply include the terminal-size source code for Windows only and revert to the previous Stack-specific terminal detection code. But the previous detection code actually seems pretty similar to the code in terminal-size, so in terms of the amount of code required, there isn’t actually much difference between including terminal-size for Windows only and using the previous Stack code vs including terminal-size for both OSs.

[svenpanne]

[...] 2. Looking more broadly, the terminal-size dependency was only introduced in #3588, to fix a Windows-specific issue. So it may be possible to simply include the terminal-size source code for Windows only and revert to the previous Stack-specific terminal detection code. [...]

A friendly ping here, because this issue is effectively a show stopper on Windows... 😞 The terminal-size package seems to be dead (no release in 4 years, several open issues/PRs), so simply absorbing its tiny amount of code into stack seems to be the way to go.

Can this be done soon plus a new stack release (soon, too)? Currently the combination of stack plus Emacs is effectively broken on Windows, including one of the few sane ways to use Haskell in an IDE (Intero), which is quite sad.

[bradrn]

@svenpanne Note that this is solvable for now with my version of Stack, if you’re willing to compile it from source; I’ve been using it very successfully with Intero. But I agree that this is a big problem which does need to be fixed.

[svenpanne]

After some head-scratching and digging out my old MS-DOS books, I came up with the following workaround: Just put a file stty.bat somewhere in your Windows PATH with the following contents:

@IF "%1"=="size" exit 255

When terminal-size calls stty size, this causes a failure, which is what we want, causing Nothing to be returned for the terminal size. Any other stty call is a no-op, avoiding e.g. invalid ioctl calls. Cruel hack, but at least Intero is up and running again. Nevertheless, stack needs a real fix soon...

[bradrn]

@svenpanne That’s a very elegant solution! I tried something similar before, but I didn’t think of making my own stty. And this approach could even be bundled into the Stack installer!

I’m still wondering though: Does that solve the problems with Emacs? I seem to remember there being some cases where Emacs doesn’t use the same PATH as Windows. (Or I could be wrong, but it’s best to check anyway!)

[svenpanne]

Yes, the stty.bat hack works with Emacs, that was my main motivation. Note that I use spacemacs, not plain Emacs, the former uses a .spacemacs.env configuration file for setting up environment variables. Nevertheless, I see no reason why this hack shouldn't work with a plain Emacs.

Nevertheless, stack should not rely on stty or be flexible regarding errors from it (not there/failed ioctl/...).

[tuomohopia]

What's the status of this PR for merge? I keep hitting stty: 'standard input': Inappropriate ioctl for device on haskell-ide-engine on vscode/win10 and I think this may fix that.

[bradrn]

@tuomohopia It would be nice to know. The problem is really that all the approaches we’ve come up with so far have had big problems; I would list them all, but it’s easier to just to refer to the discussion above.

[svenpanne]

Well, actually it's quite easy to fix the problem. Let's analyze things:

First of all, using stack within Emacs under Windows is fundamentally broken at the moment, so #4901 should really be marked as high priority somehow. The underlying reason for this is that 9b34190 introduced a dependency to the buggy and unmaintained package terminal-size (a bad idea in retrospect). The Windows part of terminal-size incorrectly assumes that it can call stty size via a shell and get something sensible back when GetConsoleScreenBufferInfo failed, see https://github.com/biegunka/terminal-size/blob/d47596c281390f091eb97e3cd40ee374c7f29314/src/System/Console/Terminal/Windows.hs#L41-L57. The fix for this in terminal-size is quite easy and consists of 2 parts:

• Use proc "stty" ["size"] instead of shell "stty size", so we get an Exception when the executable is not found. Currently we just get a written complaint from the command interpreter (bad!).
• catch the IOError in that case and return Nothing.

This is the real fix, this pull request is just sweeps the problem under the carpet, and my alternative of putting a one-line batch script in the PATH (see #4968 (comment)) is not much better, but it "works" at least without changing stack.

Taking over maintainership of terminal-size and applying the fix there will probably take ages, so IMHO the right way to proceed for stack is to absorb terminal-size into its own source tree.

[bradrn]

• Use proc "stty" ["size"] instead of shell "stty size", so we get an Exception when the executable is not found. Currently we just get a written complaint from the command interpreter (bad!).

• catch the IOError in that case and return Nothing.

This is the real fix, this pull request is just sweeps the problem under the carpet

How does using proc as opposed to shell help?

But I do agree that my solution just ‘sweeps the problem under the carpet’, as you put it; I’m not too familiar with this area, and that was the best solution I could figure out with my limited knowledge.

Taking over maintainership of terminal-size and applying the fix there will probably take ages, so IMHO the right way to proceed for stack is to absorb terminal-size into its own source tree.

I’m starting to agree that this is the best way to go here, despite the code duplication. At this stage I can’t think of any other solution.

[svenpanne]

[...] How does using proc as opposed to shell help? [...]

It's because of the difference between "finding a command command interpreter" (with shell, works even if the shell doesn't find stty) and "finding an executable" (with proc).

[jneira]

After some head-scratching and digging out my old MS-DOS books, I came up with the following workaround: Just put a file stty.bat somewhere in your Windows PATH with the following contents:

@IF "%1"=="size" exit 255

Another workaround, if you have msys installed (and stack installs always one in windows systems under local-programs-path / stack path --programs) is to use the included stty.exe in %MSYS_ROOT%\usr\bin%

EDIT: see #4968 (comment)

[svenpanne]

Another workaround, if you have msys installed (and stack installs always one in windows systems under local-programs-path / stack path --programs) is to use the included stty.exe in %MSYS_ROOT%\usr\bin%

Alas, this is not a workaround because stty fails when used within an IDE, see e.g. #4901 (comment). You have to use the dummy-stty from my reply...

[bradrn]

It's because of the difference between "finding a command command interpreter" (with shell, works even if the shell doesn't find stty) and "finding an executable" (with proc).

That makes sense @svenpanne — thanks for explaining! Pity the documentation doesn’t make that clear…

[snoyberg]

I'm reviewing this PR now. I'm fine with absorbing this code. Does someone want to update this PR to do that? I'm not in favor of using silently as the current code does.

[snoyberg]

I do not want to go in the direction of silently.

[bradrn]

@snoyberg I totally agree with you! Since I created this PR, I have learnt that this is a dangerous solution, mostly due to hspec/silently#6 (mentioned earlier in the comments). The problem is that we haven’t yet found any really satisfactory solution to this — the best idea we have so far is #4968 (comment), which suggests modifying terminal-size and absorbing that code into stack.

[snoyberg]

Closing in favor of #5183. That one is blocked on a CI issue, but should be mergeable soon.

[bradrn]

On a quick glance, it looks like that PR just inlines some of the code from terminal-size into stack and corrects it by redirecting sterr into a pipe — is that impression correct? Anyway, that solution looks better than the ones suggested here, so thanks for finally fixing this issue!