1227079 - run_command() is called for various Python scripts without sys.executable which causes failures on Windows like "not a valid Win32 application"

Status: RESOLVED INCOMPLETE

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Initially we found that problem while trying to get our Firefox UI tests running on Windows by using gittool.py as default for the checkout step.

Some research showed up that the problem we have here is manifested in the way how internal python scripts are getting called via run_command(). The command passed over only contains the py file but not the Python executable. Given that internally we don't run the command with shell=True (which indeed can be dangerous) the above mentioned failure will be thrown. So a solution would be to use sys.executable as real command and pass in the Python file as first argument.

http://stackoverflow.com/questions/25651990/oserror-winerror-193-1-is-not-a-valid-win32-application

This problem might not be visible on the RelEng build or test machines because they have those tools pre-installed and don't call them via the py script.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Fails with not a valid Windows application:
subprocess.check_call(['script.py'])

Works but seems to be dangerous to do(?):
subprocess.check_call(['script.py'], shell=True)

Works and seems to be safe:
subprocess.check_call([sys.executable, 'script.py'])

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Interestingly run_command() misses to use the shell argument for ProcessHandler while it is in place for subprocess.Popen:

https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/script.py#1102

We should get both in sync first so that we have the same running conditions.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1 (run_command)

Just an initial patch for being in sync with both the call to subprocess.Popen and ProcessHandler.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Jordan, it would be good to know what you think in how we can get this mentioned problem fixed. If its the shell way or via sys.executable. I haven't found something else.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Maybe we should only update the consumers of `run_command()` and letting those decide if one of the two options are necessary. Once a config file specifies it's own path to the tools, it would have to take care about itself.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Also sys.executable is kinda heavily used meanwhile across various configs like:
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/b2g/desktop_windows32.py#23

Comment 7

[Jordan Lund (:jlund)]

Comment on attachment 8690785 [details] [diff] [review]
Patch v1 (run_command)

Review of attachment 8690785 [details] [diff] [review]:
-----------------------------------------------------------------

I think it is better to be explicit about the interpreter you want to use per sub process call like you have seen we do in many windows configs. However, I can't say why we don't pass shell=True to mozprocess but we do when we directly call popen. Therefore this patch makes sense. Does it solve your issue or do you also need to pre append the interpreter to the ui test call you are making on windows?

Finally, since this is a mh wide change, we should be extra cautious here about any weird side effects it creates, granted it only matches what we do for the popen case. Have you tried this on try?

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jordan Lund (:jlund) from comment #7)
> I think it is better to be explicit about the interpreter you want to use
> per sub process call like you have seen we do in many windows configs.

Ok, so we should favor sys.executable in all those cases then. I think that we should change the default settings for gittool and others so we can ensure that the interpreter gets set.

> However, I can't say why we don't pass shell=True to mozprocess but we do
> when we directly call popen. Therefore this patch makes sense. Does it solve
> your issue or do you also need to pre append the interpreter to the ui test
> call you are making on windows?

It does not solve the problem. I think this patch only affects a very small set of code or nothing given that nearly all processes we start have at least 1 argument. So in 99.99% of all cases a list will be passed into run_command() which causes shell to be set to False. This patch is just here for parity.

> Finally, since this is a mh wide change, we should be extra cautious here
> about any weird side effects it creates, granted it only matches what we do
> for the popen case. Have you tried this on try?

Not yet given that I wanted to get your opinion on it first. But I have triggered a try now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1a0277f6648

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I was talking with Jordan on IRC about the remaining things to fix and we agreed to leave this out for now. It might be too invasive to do, and as long as we don't really need it, there is no harm to leave what we have right now. As long as you can setup the affected exes yourself in a config all will be fine. In case of firefox-ui-tests this landed via bug 1226769.

So I'm only going to land this patch if try is passing.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/861a0c06290d

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/861a0c06290d

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It needs to stay open for the remaining fixes.