AddonFinderProcess fixes

[andrewfg]

See #3904 (comment)

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

[andrewfg]

@holgerfriedrich / @mherwege for info..

[andrewfg]

@holgerfriedrich as mentioned elsewhere I have some doubts about whether your usage of args[0] to get the executable name is actually correct. I think that the executable name would be in "args[-1]" (if such a thing would exist)..

Therefore I think the first code block quoted below should probably be replaced by the second code block below. However I did not make that change (yet) because you may have a good explanation about why args[0] does actually make sense. => Please advise.

Doubtful Code

       protected ProcessInfo(ProcessHandle.Info info) {
           String cmd = info.command().orElse(null);
           if (cmd == null) {
               String[] args = info.arguments().orElse(new String[] {});
               // TODO (TBC) I think that the following use of args[0] may be wrong ??
               cmd = args.length > 0 ? args[0] : null;
           }
           command = cmd;
           commandLine = info.commandLine().orElse(null);
       }

Possible Replacement

        protected ProcessInfo(ProcessHandle.Info info) {
            String cmd = info.command().orElse(null);
            if (cmd == null && !Platform.isWindows()) {
                String[] args = info.commandLine().orElse("").split("\\s+");
                cmd = args.length > 0 ? args[0] : null;
            }
            command = cmd;
            commandLine = info.commandLine().orElse(null);
        }

---

EDIT: below is a log on Linux showing command, arguments and commandLine where it is evident that the executable name "python3" is NOT to be found in args[0] but rather IS to be found in the first token of commandLine.split("\\s+")..

command:null, arguments:[-u, /home/openhabian/grott/grott.py], commandLine:/usr/bin/python3 -u /home/openhabian/grott/grott.py

[holgerfriedrich]

• @andrewfg I agree, the current implementation only works for Windows. Don't know what I tested, but obviously args[0] contains the first parameter and not the command :-/

I checked both of my test setups:

• Windows only gives command to me. arguments and commandLine are always empty. command is empty for a few tasks as well.
• Linux: many entries with all 3 elements empty. command only populated for java task. arguments do not include the command. commandLine is available and contains the command as well. Spaces are not escaped, so splitting the commandLine may not work (edge case, spaces in directory names uncommon in Linux)

Regarding scripts: On Windows, we just get the Python executable, not the script which is called... It seems we cannot get the arguments or commandLine using this API. Or have I overlooked something again?

[andrewfg]

splitting the commandLine may not work (edge case, spaces in directory names uncommon in Linux)

Split will certainly NOT work on Windows since path names of the executable may include spaces. However Windows always has the command populated so split is never actually needed.

By contrast on Linux path names cannot contain spaces. So split always works.

Regarding scripts: On Windows, we just get the Python executable, not the script which is called

That seems indeed to be the case.

[andrewfg]

@holgerfriedrich -- I made some changes today. The match-property value support is as now follows. => Can you please confirm that this is Ok?

Windows

• commandLine property value not available -- so no support for (e.g. python) scripts
• command supported directly via ProcessHandle.info().command()

Other OS

• commandLine supported directly via ProcessHandle.info().commandLine()
• command supported via parsing ProcessHandle.info().commandLine().split()[0] -- this is a change from your previously wrong ProcessHandle.info().arguments()[0]

[andrewfg]

@holgerfriedrich apropos Windows there is a thread on StackOverflow (below) .. it contains a post that suggests a hack to get the command line .. but I am not sure if we should go that far. => WDYT?

https://stackoverflow.com/questions/46767418/how-to-get-commandline-arguments-of-process-in-java-9

---

EDIT: it seems the Windows 'wmic' console command is anyway deprecated, and will be removed from future Windows versions.

[holgerfriedrich]

@holgerfriedrich apropos Windows there is a thread on StackOverflow (below) .. it contains a post that suggests a hack to get the command line .. but I am not sure if we should go that far. => WDYT?

I think this is too much of a hack. Too much platform specific complexity, unless we have a clear need for that.
With the additional comment about future removal: My recommendation is not to implement this and accept that we cannot detect scripts on Windows.

[andrewfg]

@holgerfriedrich just so you know: the change that I made from args[0] to commandLine[0] means that now on Linux systems those bindings that already have discovery info in their addons.xml will now be properly suggested (they were not suggested previously) e.g. MQTT ..

[holgerfriedrich]

Not sure if the implementation of ProcessHandle.allProcesses() is JDK specific, so when searching for the command on other OS, maybe check if command is populated and only if not start splitting commandLine.

Linux paths can contain spaces as well, I just tried out and got a commandLine as follows: /home/openhabian/test dir/bash. Space is in the directory name, and not escaped in the return value. But I think it is the same as above for Windows: edge case, no need to implement a solution for that. Directory names with spaces are very uncommon.

[holgerfriedrich]

@holgerfriedrich just so you know: the change that I made from args[0] to commandLine[0] means that now on Linux systems those bindings that already have discovery info in their addons.xml will now be properly suggested (they were not suggested previously) e.g. MQTT ..

👍 Maybe we can ask to get this cherry picked to the 4.1 branch.

I have not yet looked at the code, maybe I can do later today.

[andrewfg]

check if command is populated and only if not start splitting

Yes. That is the case.

got a commandLine as follows: /home/openhabian/test dir/bash

Ok. I think I may have a solution for that. We could split the commandLine at the position where the string in arg[0] is located. Or on space if args are null..

[andrewfg]

I have not yet looked at the code, maybe I can do later today.

See my latest commit. Please read the javadoc..

[mherwege]

Is the path in the command relevant? I would argue it is not because you do not want to match on something that can be different by installation. If not, you could split on file separator character first before separating on whitespace. That would eliminate the paths with whitespace problem.
The file separator character can easily be retrieved:

String fileSeparator = FileSystems.getDefault().getSeparator();

[andrewfg]

the path in the command relevant?

Who knows? Depending on the addon, it may indeed be so. So I will try to keep it.

Also splitting on slash characters would IMHO make the parsing yet more complex.

[mherwege]

Also splitting on slash characters would IMHO make the parsing yet more complex.

I think you should be able to do it with one regex (not tested):

String fileSeparator = FileSystems.getDefault().getSeparator();
String regex = "^(([^\\" + fileSeparator + "]*\\" + fileSeparator + ")+)?[^\\" + fileSeparator + "\\s+]*"
Pattern cmdPattern = Pattern.compile(regex);
Matcher matcher = cmdPattern.matcher(commandLine)
String command = matcher.find() ? matcher.group() : null;

Of course the pattern can be compiled in advance.

[andrewfg]

I think you should be able to do it with one regex (not tested)

@mherwege I did test it .. and unfortunately it doesn't work.

Anyway, I just pushed another commit that I think resolves the issue in faster resp. better way.

[mherwege]

LGTM

[andrewfg]

@mherwege I think for safety sake I need to check that lastIndexOf(args[i]) does not return -1 .. it might be the case that args has a String that is somehow not in the commandLine..

[mherwege]

@mherwege I think for safety sake I need to check that lastIndexOf(args[i]) does not return -1 .. it might be the case that args has a String that is somehow not in the commandLine..

Would be unexpected, but indeed, defensive programming.

[holgerfriedrich]

LGTM

[holgerfriedrich]

@andrewfg I tried parts of the code with Linux and Windows.
It seems that Set is no longer removing the duplicates.
Could you pls double check?

For me, Windows returned dozens of (null, null), and also other duplicate objects, in spite of the conversion .collect(Collectors.toUnmodifiableSet());.

May be caused by our helper lass which does not properly implement comparison.
Using a record is one way to fix this.

[mherwege]

LGTM

[andrewfg]

@mherwege / @holgerfriedrich UPDATE: OK all is good. (My test environment was wrong..)

[holgerfriedrich]

LGTM.
I just copied the record into my plugin for a quick test and tried it on Linux and Windows, works fine.

[wborn]

Thanks for the fixes!