Closed gorbunov closed 4 years ago
OMG, thank you! Or I'll try to fix it 🙈
@gorbunov Hopefully, it is fixed in https://github.com/seregazhuk/php-watcher/pull/44 Can you check it, please? I cannot reproduce the bug 😞
@seregazhuk better, but still colored and with other bytes in the logs output:
Ping @alecrabbit. Under the hood I use alecrabbit/php-cli-snake. Do you have any ideas? How this can be fixed 🤔
@gorbunov do you have a supersmall PoC that we can run outself to debug this?
@seregazhuk I will look into this today
@gorbunov can you give more info about your use case?
at a first glimpse it's docker logs related "issue"
it seems like some of ansi codes are ignored in logs
and the best option would be to disable any ansi output, like with --no-ansi
UPD. option --no-ansi
should disable any color output and spinner
@gorbunov could you please provide a container for me to reproduce this issue and maybe I'll find other solution
@alecrabbit Sorry for the delay, I managed to narrow it down a bit:
On the current php-watcher version (0.4.3), and with simple test case:
docker run ogorbunov/test:php-watcher
container sources
docker logs
from zsh console, it works fine, I dont see any color sequences or spinner in logs, except that I lose blinking cursor in console after output.
Also: thanks you guys, this actually helps A LOT. =)
@gorbunov thanks for info
so, my recomendation for this particular use case is to have --disable-spinner
option or/and --no-ansi
option(has to disable spinner as well)
@seregazhuk if you'll need any new functionality in alecrabbit/php-cli-snake please create a feature request issue there, I'm willing to help 😉
@alecrabbit is there any way to detect it programmatically: whether ansi colors are supported or not? 🤔 I don't want to make this behavior with disabling spinner explicit via the option. It will be cool if the tool itself could understand whether the spinner can be rendered correctly or not.
I have found such a snippet:
if(!posix_isatty(STDOUT)) {
// no ansi support
}
@seregazhuk I've experimented with autodetect with php-console-spinner and it works in most cases (I can't test it on Windows) I have a class for that Stream::class
public static function hasColorSupport($stream = null): bool
{
$stream = self::refineStream($stream);
if ('Hyper' === getenv(ENV_TERM_PROGRAM)) {
return true;
}
if (onWindows()) {
return static::checkWindowsColorSupport($stream);
}
if (\function_exists('stream_isatty')) {
return @stream_isatty($stream);
}
if (\function_exists('posix_isatty')) {
return @posix_isatty($stream);
}
return static::checkStream($stream);
}
It needs to be tested more and its package needs to be cleaned up
so, I suppose we need to make a desicion what route to take here
UPD. Actually you have symfony/console
as a dependency - I think there is an autodetect function. I'll check it tomorrow
I'd vote for cli option as well. I feel it's like complex edge case here — i believe, phpstorm tries to use system console (zsh on newer osx, bash on older, cmd/ps on win, dash/bash on linux), so bug is not in console color or control sequences support itself, just in this complex tty piping between docker containers logging, phpstorm, actual terminal, etc.
@seregazhuk symfony/console
has its own Stream::class
I've deliberately removed autodetect dependency so lib users could decide enable spinner or not to make it more platform independent, e.g. on windows I recommend disable spinner by default.
UPD. not all terminal emulators have full ansi support by default, I mean most of them do but not all. I suppose it'll change in future, but for now we have to deal with it.
@seregazhuk I've decided to add disable()
method for spinner alecrabbit/php-cli-snake, will be available since ^0.4
, it disables any output anywhere. But still you have to do detection.
@alecrabbit Thank you! I think the best approach will be the following:
--no-spinner
(or something like that) and try to detect ansi support. Right? Am I missing something? Ping @gorbunov @alecrabbit
@alecrabbit I think there is no need in disable()
method 🤔 Once the watcher is running it will be impossible to toggle the spinner. So, I'll detect it on the start, if it is disabled I just don't render it and that's it.
I think there is no need in
disable()
method
I've already added it, so it stays :smile:
@gorbunov fixed in v0.5.1
Thank you @gorbunov and @alecrabbit for your help! 👍 💪
Would be nice to make option to run this without spinner, as docker console looks really nice, but become unhelpful )