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

Pager help pages are broken on Windows #166

Closed
nojb opened this issue Nov 22, 2022 · 32 comments · Fixed by ocaml-dune/cmdliner#3
Closed

Pager help pages are broken on Windows #166

nojb opened this issue Nov 22, 2022 · 32 comments · Fixed by ocaml-dune/cmdliner#3
Labels

Comments

@nojb
Copy link

nojb commented Nov 22, 2022

Pager man pages (what you get when you pass --help by default) are broken on Windows. The first page of the help page renders correctly, but all terminal input is frozen. The only solution to get unstuck seems to be to kill groff and less (in my setup, these are Cygwin binaries).

Some more information:

The command line executed by cmdliner is of the form groff -m man -K utf8 -T utf8 < help.groff | less -R. It is executed using Sys.command, ie cmd.exe on Windows:

  • Executing the command-line -> hangs (this is the bug).
  • Adding -P-c to the groff command-line -> works
  • Redirecting the output of groff to a file $file and then invoking less -R < $file -> works

The issue seems to be a bad interaction between cmd.exe pipeing, Cygwin binaries and color escape codes. A possible solution is to do as in the last point above and avoid the pipe by redirecting the output of groff to an intermediate file, eg something along these lines (this patch fixed the problem in my system):

diff --git a/src/cmdliner_manpage.ml b/src/cmdliner_manpage.ml
index 46353d5..7ce4ae1 100644
--- a/src/cmdliner_manpage.ml
+++ b/src/cmdliner_manpage.ml
@@ -473,7 +473,9 @@ let pp_to_pager print ppf v =
           let groffer = groffer ^ opts in
           begin match pp_to_temp_file (print `Groff) v with
           | None -> None
-          | Some f -> Some (strf "%s < %s | %s" groffer f pager)
+          | Some f ->
+              let out = Filename.temp_file "groff" "out" in (* TODO: remove temp file *)
+              Some (strf "%s < %s > %s && %s < %s" groffer f out pager out)
           end
       in
       match cmd with
@nojb nojb changed the title pager help pages are broken on Windows Pager help pages are broken on Windows Nov 22, 2022
@nojb
Copy link
Author

nojb commented Nov 22, 2022

Incidentally, it seems that not all help pages are broken, only some. I am attaching the groff source of a hanging help page (produced using --help=groff):

help.txt

Executing groff -m man -K utf8 -T utf8 < help.txt | less -R (where groff and less are the Cygwin binaries) in cmd.exe freezes the terminal in my system.

@dbuenzli dbuenzli added the bug label Nov 22, 2022
@dbuenzli
Copy link
Owner

Did it used to work ? Is it maybe the new UTF-8 stuff of fab6ffa that trip things out ?

@nojb
Copy link
Author

nojb commented Nov 22, 2022

Did it used to work ? Is it maybe the new UTF-8 stuff of fab6ffa that trip things out ?

Yes, it used to work thanks to the second point above (passing -P-c to groff). The commit in question is 2100285.

@dbuenzli
Copy link
Owner

So the discussion about the removal of this was there.

@nojb
Copy link
Author

nojb commented Nov 22, 2022

So the discussion about the removal of this was there.

Thanks. I went over the discussion but I could not quite understand why -P-c was removed (there were many other flags begin discussed). cc @edwintorok @MisterDA do you have any insight?

@dbuenzli
Copy link
Owner

As mentioned in the release notes this was removed and instead less is invoked with -R.

I'm starting to think that the bug is rather in the interaction of less and cmd.exe.

@nojb
Copy link
Author

nojb commented Nov 22, 2022

I'm starting to think that the bug is rather in the interaction of less and cmd.exe.

However, Perhaps; however less -R < $file (where $file is the output of groff with the current flags) seems to work fine.

@MisterDA
Copy link
Contributor

Thanks for identifying this issue… I ran into it a while ago and I thought I was getting completely crazy because it would work sometimes and sometimes not. If less -R < $file works that's fine, I can test it too.

@dbuenzli
Copy link
Owner

However, Perhaps; however less -R < $file (where $file is the output of groff with the current flags) seems to work fine.

So that rather looks like one of these:

  1. A bug in groff when writing to stdout
  2. A bug in the | operator (who does that ?)

Is there maybe any line translation business occurring that should not be happening ?

@edwintorok
Copy link

I'm not too familiar with Windows, but from last time I used it there were several terminal implementations there, including powershell, windows terminal, the cygwin terminal (and msys terminal if that is different). Are all of them broken/frozen or just cmd.exe when you try the testcase from #166 (comment)?
Tools can also detect (at least on Linux) whether the output isatty or is a pipe and do things differently (e.g. turn off color output for a pipe unless instructed differently, etc.)

@nojb
Copy link
Author

nojb commented Nov 22, 2022

there were several terminal implementations

It doesn't look like it is an issue with the terminal emulator itself, but rather with the command interpreter (used by Sys.command), namely cmd.exe.

@edwintorok
Copy link

According to this https://ss64.com/nt/syntax-redirection.html pipes with external commands create their own instances of cmd.exe. If cmd.exe acts as both terminal emulator and shell I'm not sure how well that will work.
There are also some indications that various system settings might break stdin in cmd.exe https://bugs.python.org/issue16892.

@nojb
Copy link
Author

nojb commented Nov 23, 2022

According to this https://ss64.com/nt/syntax-redirection.html pipes with external commands create their own instances of cmd.exe. If cmd.exe acts as both terminal emulator and shell I'm not sure how well that will work. There are also some indications that various system settings might break stdin in cmd.exe https://bugs.python.org/issue16892.

Thanks, perhaps one (or more) of these issues may be playing a role.

Taking a step back, could someone spell what what are the advantages of not passing -P-c to the groff invocation? Incidentally, emitting ANSI color codes also broke the help pages for users setting PAGER=less (see #167).

@dbuenzli
Copy link
Owner

Ding. There's a fixed issue that is asking for a release. We also have another new annoyance here.

Can we try to fold fixes for these before a new release ?

@nojb
Copy link
Author

nojb commented Jan 13, 2023

I agree, this is blocking the use of the current cmdliner on Windows.

@MisterDA: do you know the answer to my question above?

Taking a step back, could someone spell what what are the advantages of not passing -P-c to the groff invocation?

@MisterDA
Copy link
Contributor

Typography

So, groff/grotty are able to change text attributes (bold, italic, colors). To the best of my knowledge, Cmdliner only outputs bold and underlined (or italic?).

Taking a step back, could someone spell what what are the advantages of not passing -P-c to the groff invocation?

In our case, groff calls grotty as its post-processor. The -P option of groff forwards its value to the post processor. We can either ask grotty to output SGR sequences or a so-called "old output format".

grotty(1)'s manpage is rather clear on what's needed to support SGR sequences (quoting):

By default, grotty emits SGR escape sequences [...]
Use the -c switch to revert to the old behaviour
For SGR support, it is necessary to use the -R option of less(1)

The conservative option is to keep using -P-c and disable SGR output.

After some more digging, it seems that this is the solution chosen and somewhat documented by Arch Linux and Debian. They disable SGR output from grotty and fake colors by tweaking the pager.

Color output in console § 2.6

There is a real color facility in grotty(1), but it is strongly discouraged for man pages. Here we fake a colored man by hacking two main pagers, less and most: we replace the sequences for bold, standout, and underline with spiced ones that contain color.

What we could also do is default to -P-c option if PAGER or MANPAGER are defined, but if they're not use SGR sequences and chose less -R. Defaulting to less -R is a good thing in all cases.

The more.com pager under Windows recognizes SGR sequences but not the "old output format" (-P-c option).

I think I misunderstood previously that groff has the capability of outputting colors, but the ones I was seeing were because of tweaks as the wiki describes, applied to the "old output format", not directly from the groff document--because Cmdliner doesn't produce colors. Sorry for the confusion. I guess for simple typographic purposes we can switch back to "old output format" and we don't need SGR codes (unless using Windows's more.com).
If we ever want to support hyperlinks macros it's possible that we'll need SGR for the OSC hyperlink sequence.

Windows hang

Regarding the issue at hand, the hang can be reproduced on Windows with any terminal. The invocation below, in a Cygwin bash shell (Cygwin 3.4.3, groff 1.22.4, less 590, Windows 22H2), hangs. The help.groff file.

cmd /C 'groff -m man -K utf8 -T utf8 <help.groff | less -R' # hangs
cmd /C 'groff -m man -K utf8 -T utf8 -P-c <help.groff | less' # hangs
cmd /C 'groff -m man -K utf8 -T utf8 -P-c <help.groff | less -R' # hangs

cmd /C 'groff -m man -K utf8 -T utf8 <help.groff | more.com' # doesn't hang
cmd /C 'groff -m man -K utf8 -T utf8 -P-c <help.groff | more.com' # doesn't hang, but no formatting

Using a intermediary file never hangs, but prints the name of the file in the pager.

cmd /C 'groff -m man -K utf8 -T utf8 <help.groff >help.tty && less -R help.tty'

I don't know what's happening. I can bisect next week if needed.

@dbuenzli
Copy link
Owner

Thanks @MisterDA for the summary, it would be great if you can investigate the Windows hangs. Also maybe someone like @dra27 has a suspicion (see "Windows hang" in the preceding message) ?

Defaulting to less -R is a good thing in all cases.

Note that in 1de3611 I tried to add a more brittle way to do that to solve #167 so it would be good to test the hanging invocations with the new invocations. Basically:

cmd /C 'groff -m man -K utf8 -T utf8 <help.groff >help.tty && LESS=FRX less help.tty'

@nojb
Copy link
Author

nojb commented Jan 18, 2023

Using a intermediary file never hangs, but prints the name of the file in the pager.

To me this seems the most robust way of fixing the issue. Isn't there a way to avoid showing the temporary file name in the output?

@nojb
Copy link
Author

nojb commented Jan 22, 2023

Using a intermediary file never hangs, but prints the name of the file in the pager.

To me this seems the most robust way of fixing the issue. Isn't there a way to avoid showing the temporary file name in the output?

Answering to myself: yes, there is an easy way; just replace less help.tty by less <help.tty.

cmd /C 'groff -m man -K utf8 -T utf8 <help.groff >help.tty && set LESS=FRX && less help.tty'

becomes

cmd /C 'groff -m man -K utf8 -T utf8 <help.groff >help.tty && set LESS=FRX && less <help.tty'

Except for the extra temporary file help.tty, everything else remains the same, including the fancy output codes. @dbuenzli: what do you think?

@dbuenzli
Copy link
Owner

dbuenzli commented Jan 22, 2023

We could do that but I have to say that it annoys me that we are not able to understand what is happening.

Reading around, isn't maybe the problem an operator precedence issue ? Does any of the following maybe work ?

cmd /C '(groff -m man -K utf8 -T utf8 -P-c <help.groff) | less'
cmd /C '(groff -m man -K utf8 -T utf8 -P-c <help.groff) | (set LESS=FRX && less)'
cmd /C 'set LESS=FRX && ((groff -m man -K utf8 -T utf8 -P-c <help.groff) | less)'

@nojb
Copy link
Author

nojb commented Jan 22, 2023

We could do that but I have to say that it annoys me that we are not able to understand what is happening.

Maybe the output of groff contains some characters which are interpreted by cmd.exe, see Pipes and CMD.exe in https://ss64.com/nt/syntax-redirection.html:

image

Does any of the following maybe work ?

They don't.

@dbuenzli
Copy link
Owner

https://ss64.com/nt/syntax-redirection.html:

I also read that part but eventually rather understood that the syntax on the left hand-side might need escaping.

Other bits on the internet actually seemed to indicate that cmd.exe was actually the way to go for piping binary since powershell seems to have created its own mess.

@nojb
Copy link
Author

nojb commented Jan 22, 2023

I also read that part but eventually rather understood that the syntax on the left hand-side might need escaping.

You are probably right. And in any case it doesn't explain the present issue because

  • cmd /C 'groff -m man -T utf8 -K utf8 < input.groff | less -R' hangs; but
  • cmd/C 'groff -m man -T utf8 -K utf8 < input.groff > input.out' followed by cmd /C 'cat input.out | less -R' works.

@nojb
Copy link
Author

nojb commented Jan 22, 2023

An interesting data point: the hanging only seems to happen when the help page is longer than a single screenful. If it fits in a single screen, it doesn't hang.

@dbuenzli
Copy link
Owner

Something like that seem to be mentioned at the end of this answer "EDIT: Asynchronously is not the full truth".

@dbuenzli
Copy link
Owner

The more I read on this the more I'm horrified. I think I'll admit defeat and use a temporary file.

@nojb
Copy link
Author

nojb commented Feb 20, 2023

The more I read on this the more I'm horrified. I think I'll admit defeat and use a temporary file.

I haven't been able to underestand anything further, so unfortunately the temporary file seems to be the only real workaround so far...

@nojb
Copy link
Author

nojb commented Apr 4, 2023

Absent a better solution, would it be possible to go with the temporary file?

@dbuenzli
Copy link
Owner

dbuenzli commented Apr 4, 2023

Certainly. Just don't have the time at the moment.

dbuenzli added a commit that referenced this issue Apr 9, 2023
@dbuenzli
Copy link
Owner

dbuenzli commented Apr 9, 2023

@nojb Could you please confirm 4158497 works.

@nojb
Copy link
Author

nojb commented Apr 10, 2023

@nojb Could you please confirm 4158497 works.

I think a && is missing. With this change, seems to work in my machine.

diff --git a/src/cmdliner_manpage.ml b/src/cmdliner_manpage.ml
index 89435ae..941220a 100644
--- a/src/cmdliner_manpage.ml
+++ b/src/cmdliner_manpage.ml
@@ -490,7 +490,7 @@ let pp_to_pager print ppf v =
               begin match tmp_file_for_pager () with
               | None -> None
               | Some tmp ->
-                  Some (strf "%s <%s >%s %s <%s" groffer f tmp pager tmp)
+                  Some (strf "%s <%s >%s && %s <%s" groffer f tmp pager tmp)
               end
           | Some f ->
               Some (strf "%s < %s | %s" groffer f pager)

@dbuenzli
Copy link
Owner

Ah yes of course.

emillon pushed a commit to ocaml-dune/cmdliner that referenced this issue Sep 25, 2023
emillon pushed a commit to ocaml-dune/cmdliner that referenced this issue Sep 25, 2023
emillon pushed a commit to ocaml-dune/cmdliner that referenced this issue Sep 25, 2023
Signed-off-by: Etienne Millon <me@emillon.org>
emillon pushed a commit to ocaml-dune/cmdliner that referenced this issue Sep 25, 2023
Signed-off-by: Etienne Millon <me@emillon.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants