[executeCommandLine] should return STDERR if STDOUT is empty

[andrewfg]

NOTE: this relates to Issue #2033 (comment)

If ErrorCode == 0 then executeCommandLineAndWaitResponse() would always return the response from STDOUT.

However on some Unix commands (e.g. wget, curl) if ErrorCode == 0 there is no response on STDOUT, but the commands return valid information on STDERR (even though there was not an error).

In this PR, if ErrorCode == 0, the return value of executeCommandLineAndWaitResponse() is !"".equals(STDOUT) ? STDOUT : STDERR

Signed-off-by: Andrew Fiddian-Green [email protected]

[andrewfg]

@kaikreuzer @cpmeister

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/oh3-migrated-rule-executecommandline-command-fails/114027/10

[cpmeister]

LGTM

[NorbertHD]

@andrewfg at https://community.openhab.org/t/oh3-migrated-rule-executecommandline-command-fails/114027/5 you wrote that on OH2 you got the output from wget. I have looked at the code before #1700 but there is no access to getErrorStream(), so how could the stderr output of wget be read?

About your PR, I would prefere to set the ProcessBuilder redirectErrorStream property to true. Then stdout and stderr outputs are merged in the correct chronology.

[andrewfg]

how could the stderr output of wget be read?

I have no idea. All I can say is that on OH2 it returns the message, and on OH3 it does not. Maybe it is related to the change of Java version, or something like that??

would prefere to set the ProcessBuilder redirectErrorStream property to true

Ok. Currently I have no idea how to do that though. But I will look into it tomorrow. However it might therefore require reverting the prior commit related to STDERR and STDOUT when ResultCode != 0. Since if both streams are merged in STDOUT then there's no need to look at STDERR anymore at all. Or??

[NorbertHD]

I have no idea.

I was hoping that someone with more java (ProcessBuilder) knowledge than me knows the answer.

Ok. Currently I have no idea how to do that though

You have to remove the errorFuture code and change

Process process = processTemp = new ProcessBuilder(commandLine).start();

to

Process process = processTemp = new ProcessBuilder(commandLine).redirectErrorStream(true).start();

then there's no need to look at STDERR anymore at all?

Yes

[andrewfg]

@NorbertHD many thanks for the tips. I have made the changes that you suggest.
Note: it works perfectly now in my environment. And the JUnit tests all passed.

@cpmeister it is ready to go (again) so can you please have a look at it (again) :)

[cpmeister]

LGTM

[andrewfg]

I think it would be useful if I add an extra JUnit test to confirm the STDERR redirection.

[cweitkamp]

Unit tests are always appreciated. Can you do that quickly? Otherwise a follow-up PR may be okay too.

I would like to merge this into https://github.com/openhab/openhab-core/tree/3.0.x before building a 3.0.1 patch release for this weekend.

[andrewfg]

I will try it today.

[andrewfg]

add an extra JUnit test to confirm the STDERR redirection

@cweitkamp done :)

[kaikreuzer]

I would like to merge this into https://github.com/openhab/openhab-core/tree/3.0.x before building a 3.0.1 patch release for this weekend.

@cweitkamp Afaik, this is an improvement, not a bug fix.
The regression in comparison to 2.5 was fixed by #2104 and is already cherry-picked to 3.0.1.

[andrewfg]

yhis is an improvement, not a bug fix.

@kaikreuzer hmm, not really. The prior PR only partly solved the regression from v2.5 (the case where errorCode != 0), but it did not resolve the case where errorCode == 0 and (yet) there was still data on STDERR. But I think we are talking of nuances here..

[kaikreuzer]

Are you really sure that STDERR was considered in 2.5? I wouldn't understand why this would have been the case - from the code it looks as if clearly only STDOUT was returned.

[kaikreuzer]

Ok, diving a bit deeper, it seems you are right - the variable name in 2.5 was chosen misleadingly. The PumpStreamHandler indeed combines the output of STDOUT and STDERR into a single stream.

[andrewfg]

indeed combines the output of STDOUT and STDERR into a single stream.

Yes. And the test case is the 'wget' command which just returns an empty string in STDOUT, and returns the full connection status trace with its HTTP response headers in STDERR. The 'curl' command is similar..

[kaikreuzer]

I would like to merge this into https://github.com/openhab/openhab-core/tree/3.0.x before building a 3.0.1 patch release for this weekend.

Go ahead with this then @cweitkamp - I leave it to you :-)

[kaikreuzer]

Seems you are busy, so I'll take care of it.

[cweitkamp]

Yes, I was. Thanks for taking care.