Fix syntax for array access in cylc check-software

[kinow]

Not happening any time soon, as we don't have a dependency to OTHER tool in check-software, but only to PY tools for now.

But if we had one, then it would crash due to an array access moved to next line. The code is still perfectly fine, so in theory no syntax error. Except that our intention here is to access one of the elements of the tuple returned by process communicate() method.

Tested locally by adding

diff --git a/bin/cylc-check-software b/bin/cylc-check-software
index 4d8843d43..ff2bfb878 100755
--- a/bin/cylc-check-software
+++ b/bin/cylc-check-software
@@ -54,6 +54,8 @@ req_py_ver_range = ((3,6),)
 opt_spec = {
     'EmPy': [None, 'TEMPLATING', 'PY'],
     'sphinx': [(1, 5, 3), 'HTMLDOCS', 'PY'],
+    'graphviz': [None, 'TEMPLATING', 'OTHER',
+            (['dot'], '-V', r'graphviz version ([^\s]+)', 2)]
 }

And debugging in the IDE. Not applicable to the 7.8.x branches.

[matthewrmshin]

Not a problem of this change.

The outfile setting here is a bit awkward. The normal interface looks like out, err = proc.communicate(), so I guess we are looking for 0 or 1 for index here? So we are expecting outfile to be 1 (for STDOUT) or 2 (for STDERR)?

Just not very clear unless you know subprocess.Popen very well.

[hjoliver]

I agree that's a bit hard to understand. git blame says most recently touched by Martin's bandit changes, but the construct dates back to before that, so perhaps @sadielbartholomew can comment?

[hjoliver]

(But I guess the actual bug here was introduced by @MartinRyan (and missed by our reviews - sorry).

[kinow]

@matthewrmshin

Just not very clear unless you know subprocess.Popen very well.

+1, I re-read the code a few times to try to understand what it was doing. Then had to open the docstring of communicate

(But I guess the actual bug here was introduced by @MartinRyan (and missed by our reviews - sorry).

Yup. But we will miss more small issues like this. But as long as we can cover code like this with simple tests, we should be good 😬

Should I create a separate, low priority ticket, to review the way we are handling Popen, communicate, and accessing the array response in this method? Or would it be best to address that here now? I'd prefer to first fix this syntax issue, and later see how to simplify this code.

[hjoliver]

I'm pretty sure this is a one-off unusual use of Popen (plus @MartinRyan has recently wrapped Popen to assuage bandit, so there are few distinct calls now - all go via the wrapper). So yeah, just fix this syntax issue I'd say.

[sadielbartholomew]

the construct dates back to before that, so perhaps @sadielbartholomew can comment?

Sorry, I missed this namecheck until now.

I'll be honest, I do not have much more understanding of the motivator & context behind the outfile setting, as I based a lot of my code for check-software on that written by @oliver-sanders for rose check-software, & though I will have understood it superficially, I recall I struggled to comprehend what the numerical outfile-based index was doing too (but left it in because it worked 😁 ). Perhaps Oliver can comment further?

[hjoliver]

This is the correct fix, tested (by artificially restoring a graphviz dependency), no need (IMO) to be too picky about making the existing code clearer in this case (it's not that hard to figure out what it does).

[hjoliver]

(Back port not required - done as part of #3021)