c-blake / cligen

Nim library to infer/generate command-line-interfaces / option / argument parsing; Docs at
https://c-blake.github.io/cligen/
ISC License
501 stars 23 forks source link

multiple test errors in MacOS #172

Closed slonik-az closed 3 years ago

slonik-az commented 3 years ago

Running ./test.sh results in multiple errors on MacOS, all of the similar nature. Typical error looks like this

85,87c85,86
< demo entry point with varied, meaningless parameters.  A Nim invocation might
< be: demo(alpha=2, @[ "hi", "ho" ]) corresponding to the command invocation "demo
< --alpha=2 hi ho" (assuming executable gets named demo).
---
> demo entry point with varied, meaningless parameters.  A Nim invocation might be: demo(alpha=2, @[ "hi", "ho" ])
> corresponding to the command invocation "demo --alpha=2 hi ho" (assuming executable gets named demo).

I suspect this is because value of COLUMNS in running tests is different from the reference output. In other words, export COLUMNS=80 does not have intended effect on MacOS. I tried using iTerm2, standard Terminal app, bash, zsh -- all the same. I think the test suit should be using a way of controlling output width independent of the environment variable settings.

c-blake commented 3 years ago

Well, yes. The test suite only produces a non-empty diff with 80 columns, but these are not real "errors". I've known that since I did it. It is more important to adapt correctly to the execution environment than to run tests with any number of columns. If the only argument to have $COLUMNS override what tty ioctls say is the test suite then this is not a good enough argument. The COLUMNS=80 in the script is really for running in a non-tty environment (such as the CI).

One possible way forward (and this could be either something you personally do or a way we adapt test.sh) is to use the stty command as in:

stty columns 80
./test.sh
stty columns $COLUMNS

That worked for me on Linux just now.

c-blake commented 3 years ago

Also, this in no way relates to MacOS. So, I am closing this as a not real issue, but we can continue to discuss if you'd like.

slonik-az commented 3 years ago

In any case, I would like to be able to run the tests without being bombarded by spurious errors that I have to separate from the real ones. Since it seems to be a real issue known to the maintainer and affecting several platforms (at least MacOS and Linux) it would be great to fortify test.sh accordingly.

c-blake commented 3 years ago

I think you can. I gave you a command-line to run it and get clean output. (Ok, you may have to join lines and add semi-colons). Maybe you can test that on MacOS for us? I believe the stty command is fairly portable, but maybe not.

c-blake commented 3 years ago

By the way, I adapted cligen/sysUt.toItr along the lines of your forum post. So, if you already have cligen as a dependency then you can just import cligen/sysUt to get that.

slonik-az commented 3 years ago

The proposed solution

stty columns 80
./test.sh
stty columns $COLUMNS

does not work on MacOS 10.14.6 (Mojave). The diff is the same as before. Apparently, cligen tests still think that there are 114 columns in the tty.

slonik-az commented 3 years ago

@c-blake : Since the workaround does not work on MacOS I would appreciate if you could re-open the issue. I can serve as a guinea pig for possible solution on MacOS.

c-blake commented 3 years ago

You should be able to just manually resize your terminal to be 80 columns wide (or keep such a terminal around) to run test.sh. 80 columns is very standard. It's what nimpretty defaults to/the Nim stdlib style guide uses, for example.

Even if it were "failing" (which I disagree with), that script is only for cligen developers/the CI, not general cligen users. I can put a big comment at the top of the file saying "only expect an empty diff if you run in 80 columns" or something if you want.

Also..I just fired up a virtual machine and got it to work fine with stty columns 80; ./test.sh on OSX with kernel 17.0.0 from Aug 24, 2017 and an old Nim-0.20.2 I have installed on that VM. I doubt things like terminal handling have changed on OSX in many years. I am just using the regular Terminal app and Zsh. { Because of the fresh checkout of cligen and Nim integer hash changes from 0.20.2 there is a very small output relating to the way $ renders a HashSet. This is an "expected non-empty diff". }

One thing that does fail, though, is that the stty columns 80 seems to send a SIGWINCH to Zsh making it reset COLUMNS to also be 80. So, to save & restore you would need to use a different environment variable. E.g.:

x=$COLUMNS; stty columns 80; ./test.sh; stty columns $x

This works for me on Linux, OSX, and FreeBSD. I'm not sure why this isn't working for you.

If we cannot get an stty that works reliably on OSX, Linux & *BSD then that kind of must-run-in-an-80-column-terminal run-time requirement on cligen-developers may be the most portable thing we can do which entails exactly zero changes.

An alternative to that is to give up on "The Terminal Is The Authority On How Wide It Is" model that stdlib terminal.terminalWidth uses. That strikes me as probably being less reliable for cl-users running --help and that strikes me as a poor trade off to make since there are at least notionally very few cligen-devs compared to users of any-cligen-utility.

c-blake commented 3 years ago

I guess we could also add some new CLIGEN_WRAP_WIDTH variable, but only to make test.sh produce empty output on non-80 terminals does not actually strike me as "enough reason" to justify that new override.

c-blake commented 3 years ago

@slonik-az - If you first do stty columns 80 and then stty -a does not report 80 columns in its output then I think this is an OS bug or Terminal bug and the workaround is making your terminal 80 columns wide, and probably issuing a bug report to the OS/Terminal upstream. There are also workarounds like running under a GNU screen or termux pseudo-terminal that can set the terminal width to 80.

slonik-az commented 3 years ago

Just ran it on a single line. Got the following using nim-1.4.0

$ stty columns 80; ./test.sh >& test.out5
$ wc test.out5
  209  1975 13460 test.out5

Then

$ head -7 test.out5
85,87c85,86
< demo entry point with varied, meaningless parameters.  A Nim invocation might
< be: demo(alpha=2, @[ "hi", "ho" ]) corresponding to the command invocation "demo
< --alpha=2 hi ho" (assuming executable gets named demo).
---
> demo entry point with varied, meaningless parameters.  A Nim invocation might be: demo(alpha=2, @[ "hi", "ho" ])
> corresponding to the command invocation "demo --alpha=2 hi ho" (assuming executable gets named demo).

As expected I am getting

$ stty columns 80; stty -a
speed 38400 baud; rows 34; columns 80;
[...snip..]

I am flabbergasted what is going on.

slonik-az commented 3 years ago

Finally I figured it out. Redirecting the output as in stty columns 80; ./test.sh >& test.out makes columns setting ineffective. However, running it without redirection

$ stty columns 80; ./test.sh

results in no output and generates test/out file identical to test/ref.

c-blake commented 3 years ago

Thanks for confirming that the command-lines I gave here and in my new test.sh comment seem to work in your environment. There are, of course, the 2 other workarounds (manual re-size and running under a pseudo-terminal).

That said, I agree that fd=[12] re-direction should work. The re-direct works for me in my admittedly old OSX environment. This is very unlikely to be a cligen issue since cligen just calls Nim stdlib terminal.terminalWidth. Just looking at the when Unix branch of that code suggests that redirects should work. stdin/fd=0 is the first one tested, and that is not even re-directed. So, the very first ioctl should succeed (and it does for me). Even if stdin were re-directed, the Nim stdlib terminalWidth opens the controlling terminal (ctty) device.

I am not sure what is happening in your very specific environment, but it sounds like whatever it is would also block "automatic hardening". I.e., it would fail if we hard-coded the x=$COLUMNS; stty columns; stuff; stty columns $x into test.sh script. Another reason to not do this "in script" but make the (very few) external users run do the stty themselves in a wrapper script or on the command-line is to force visibility of the (persistent) modification to terminal settings. If we did it "in script" and something failed to end the script early or bypass any trap handlers (like a kill -9 from somewhere else) then we would be risking leaving the terminal "misconfigured" (set as 80 columns) which could cause confusion..in Zsh/less/any terminal oriented programs. { You can try stty 5 to see what I mean, but be warned - it will mess up your terminal until you can manually resize it. } Thing can always go wrong, of course, but forcing visibility means such (few) users maybe have more hope of repairing settings.

Since you sounded pretty deep in the debugging frustrations and since I don't like to abandon people in that state, if you still have any energy to investigate then I would suggest compiling the below 2-line width.nim test program and then running it under sudo dtruss ./width. (Under Linux it would be strace, BSD, ktrace):

import terminal # save in width.nim
stderr.write terminalWidthIoctl([0,1,2]), "\n"

Only the last few lines really matter. I would be super surprised if this tiny test program does not succeed/fail in the same ways as test.sh on your system. If it fails in the same way, this might be a well-defined issue for the Nim stdlib. What I see on my (working) set up is:

sudo dtruss ./width
...
ioctl(0x0, 0x40087468, 0x7FFEE456AB80) = 0 0
getrlimit()
dtrace: error on enabled probe ...(syscall::write_no_cancel:return ...).

Then I also did it with sudo dtruss ./width >/tmp/out 2>/tmp/err getting an empty out and the err with all the same output but missing the dtrace error messages. So, I guess dtruss only writes those messages when non-redirected. Suppressing error msgs when output is re-directed seems a bit sadistic. Gack! Maybe that is fixed by your version of OSX.

Anyway, it sounds like you need to run it with re-directs to induce the error at all, since it was working for you un-redirected, but failing redirected. Those error messages are just about the write calls emitting of the "80\n" string anyway, and you can see what they do by not running it under dtruss at all.

It looks like dtruss does not (did not at my version) know how to "decode" the ioctl protocol the way strace can. The call succeeds with return != -1, though. So, it should be the only ioctl near the very end since the Nim code stops as soon as any ioctl succeeds with != -1. At most, some libc thing may do an ioctl on fd 2 before using stderr, but it should certainly skip the ioctl on fd 1 if it "is working". Mine only does fd0, but mine also works as expected.

Your failing-case dtruss output ought to be different in some detail from what I describe here. It is not easy for me to guess what that difference might be, since I cannot reproduce your failure. Some detail/environmental perturbation (OSX, backend C compiler or libc, probably not Nim version) is likely the ultimate cause. In any case, from the Nim code and shell invocation, only the stdin/fd0 ioctl should happen, and it should fill in the at 0x7FF.. address C struct with a width of 80. If it doesn't then it probably would also fail with some 7 line C program that does the ioctl()s. If that fails, you may have a bug to report to Apple. ;-)

I do not personally use OSX much at all/hardly ever (hence why my test env is so old). So, this is probably near the limit of debugging help I can provide. It is conceivable, though unlikely, that what Nim calls IOctl_WinSize defined in Nim stdlib's lib/posix/termios.nim has become incompatible with OSX in the past 3 years. It may also matter if you are on a PowerPC Mac or Intel Mac due to PPC being "big endian". None of these are likely and the Nim C shares endianness expectations with the local OS, but maybe something, somewhere is being too clever and byte swapping. I am just brainstorming.

I realize that you could also just be happy that you have some way to run test.sh without noisy output, un-redirected. In that case you can ignore all my attempts to help you track down what is weird in your OSX environment. Also worthy of note is that you could generalize this idea to have a "sim80.sh" shell script wrapper:

#!/bin/sh
x=$COLUMNS # This is sim80.sh
stty columns 80
"$@"
ex=$?
stty columns $x
exit $ex

Then you could probably put that in your $PATH and run sim80.sh ./test.sh.