open-rpa / node-red-contrib-tagui

Mozilla Public License 2.0
8 stars 4 forks source link

Better error handling #8

Closed rafael-pinheiro closed 2 years ago

rafael-pinheiro commented 2 years ago

I installed node-red in a virtual machine and tried running the example. The node gave me a completed status but I could not see the image output.

By running tagui via the CLI I could see the error (Chrome was missing) and it was easy to fix.

I feel like the plugin could use some better error handling.

skadefro commented 2 years ago

Hey, sorry to hear you had problems. but the first line in the readme says "Requires chrome to already be installed." The node allows you to "dump" all output, by disabling the silent option and attaching a debug node to the middle output connector.

But @kensoh what do you think. Should the nodered node check for prerequests like chrome or can you catch that in tagui and throw an error based on that ? I don't know how tagui reacts when chrome is missing ( I should proberly test that )

rafael-pinheiro commented 2 years ago

Thanks for the fast reply :)

I think I tried to check it on the middle connector but did not see any output. I'll try to reproduce it later when I have some time to confirm that. I have used it before so I was aware that chrome needs to be installed, wasn't a big issue. But it was a bit misleading to get a success status on node-red ui.

skadefro commented 2 years ago

Hmm, re-read your message. Maybe i should clarify ... the node will look at exit codes and output from tagui, but by default it tells tagui to run silently .. so either tagui is not reporting the error in silent mode ( a line starting with ERROR) , and/or it should exit with a non zero exit code to symbolise there is an issue with chrome missing.

kensoh commented 2 years ago

Hi Allan,

Yes, TagUI should give below error message and exit code 1 if Chrome command cannot be found -

ERROR - cannot find "google-chrome" command on your Linux to launch web browser
update chrome_command setting in tagui/src/tagui and make sure symlink is created

For silent mode, this message should still show because it is before actual execution, so the silence mode does not apply. The specific way it tests for availability of Chrome is to use the type command. If valid it will return the path to the command.

type "google-chrome"
skadefro commented 2 years ago

I just tested on a docker image without chrome, and this is what i get image I don't have easy access to debug if this is also what nodjs get's from the stdout/stderr and if i can "catch" that ...

skadefro commented 2 years ago

i managed to test it now ... stdout and stderr gets no messages when above error happens, and the exitcode is 0 Not sure how to handle that then, unless I check for chrome my self ( would a simple path search be enough for that ? ) Feels like I'm missing something obvious, the stdout or stderr SHOULD contain something, i see the error is a normal output message about missing php ... I wonder why what is not getting picked up ... hmm ...

kensoh commented 2 years ago

Hi Allan, thanks for sharing these details! Above looks like a different error that has nothing to do with Chrome being missing. It is related to PHP missing. Can you check why php command is missing? I would imagine and have validated before that your Docker image already has PHP installed as part of the Docker file definition.

kensoh commented 2 years ago

Adding on, if php command is available, php -v will be a valid command and return the PHP version.

skadefro commented 2 years ago

Yes, this is a completly different error, but still relevant since it produces the same symptons. The node cannot detect the issue due to the process returning error code 0 and for some reason nodejs does not get the output ( I still think this is a bug in my code or nodejs ... ). I will add a check for php, create an image with only php and continue the testing. ( I don't have time today but will have a look at it in the weekend )

skadefro commented 2 years ago

I ended up looking a little anyway. I just added a check for php and was about to submit it when it dawned on me. There is something inconsistent here. TagUI works perectly fine on my windows box, but I do not have php installed, yet it fails on linux. And sure enough when i look in my C:\Users\%username%\tagui\src\php there is php. Why is that missing on linux ? And do i need to check for php being pre-installed on both linux and macos, or only linux ?

kensoh commented 2 years ago

What you observed is correct and by design. For Windows, it never comes installed with PHP, thus since the start I always included the PHP binaries so that users do not need to install on their own.

For Mac, it always comes shipped with PHP, until the recent Monterey release, which users will have to manually install on their own using 2 lines in their terminal - https://github.com/kelaberetiv/TagUI/issues/1137#issuecomment-989465983

For Linux, many distributions come installed with PHP by default, but Ubuntu does not, thus an Ubuntu image or environment requires installing PHP, if it is not already installed as a requirement for other apps.

I think on Linux that can be automated. For macOS, the 2 lines to install requires user to key in their user password, thus it can't be automated.

skadefro commented 2 years ago

Thanks for the clarification @kensoh ... I now check for php on non windows installations. The node also have more logic for catching errors, and it now properly catches when php or chrome is missing

kensoh commented 2 years ago

Thanks a lot @skadefro, the open-source community is blessed to have you sharing your talents!