Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix syntax for array access in cylc check-software #3032

Merged
merged 1 commit into from
Mar 28, 2019
Merged

Fix syntax for array access in cylc check-software #3032

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions bin/cylc-check-software
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,11 @@ def cmd_find_ver(module, min_ver, cmd_base, ver_opt, ver_extr, outfile=1,
res = [NOTFOUND_MSG, False]
else:
try:
output = procopen([cmd, ver_opt], stdoutpipe=True,
stdin=open(os.devnull),
stderrpipe=True).communicate()
[outfile - 1].decode().strip()
output = procopen(
[cmd, ver_opt], stdoutpipe=True,
stdin=open(os.devnull),
stderrpipe=True).communicate()[outfile - 1].decode()\
.strip()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@sadielbartholomew sadielbartholomew Mar 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

version = re.search(ver_extr, output).groups()[0]
try_next_cmd = False
if min_ver is None:
Expand Down