Verify that a user-specified script via MSVC_USE_SCRIPT exists by jcbrill · Pull Request #4063 · SCons/scons

jcbrill · 2021-11-18T21:35:18Z

This PR eliminates one cause of subprocess/_subproc failure: an unchecked user script file name being passed to subprocess that may not exist.

The issues raised in #4058 are still possible in the face of _subproc/subprocess failures (i.e., this fixes the most likely symptom).

Behavior changed:

Verify that a user-specified script via MSVC_USE_SCRIPT exists and raise an exception if the script does not exist.

Contributor Checklist:

I have created a new test or updated the unit tests to cover the new/changed functionality.
I have updated CHANGES.txt (and read the README.rst)
I have updated RELEASE.txt
I have updated the appropriate documentation

… the user script is not found.

…ues and raise appropriate exception. Build call string rather than inline string in two separate invocations of _subproc.

…plicitly requesting _subproc to raise exceptions bypassing dummyPopen objects.

mwichmann · 2021-11-18T23:42:38Z

-                                      stderr=subprocess.PIPE)
+                                      stderr=subprocess.PIPE,
+                                      error='raise')
+    except OSError as e:


what's the intent here? catch a few specific exceptions and re-raise them, then catch essentially everything else and re-raise it too - and a bare raise would be plenty in any case.

Simply an attempt at raising the same kind of exception with a reduced traceback to provide the most information possible.

As an example,

OSError: [WinError 2] The system cannot find the file specified:

versus

Exception: [WinError 2] The system cannot find the file specified:

Is there any utility in distinguishing between a ValueError (invalid subprocess arguments) and other exceptions?

I can certainly remove all but the last clause if that is desired. The three Exception types listed are the most likely exceptions raised by subprocess.

Results from a "file-not-found" error in subprocess are shown below.
Raising a MSVCScriptNotFound exception was manually commented out to allow subprocess to raise the error..

Both a bare raise:

try: call ... except: raise

and a direct call without a try/except

call ...

result in:

FileNotFoundError: [WinError 2] The system cannot find the file specified: File "S:\SCons\Test-Arg\SConstruct", line 29: env = Environment(MSVC_USE_SCRIPT=r'C:\test_batch-file_does-not_exist.bat') File "S:\SCons\scons\scripts\..\SCons\Environment.py", line 1010: apply_tools(self, tools, toolpath) File "S:\SCons\scons\scripts\..\SCons\Environment.py", line 116: _ = env.Tool(tool) File "S:\SCons\scons\scripts\..\SCons\Environment.py", line 1879: tool(self) File "S:\SCons\scons\scripts\..\SCons\Tool\__init__.py", line 273: self.generate(env, *args, **kw) File "S:\SCons\scons\scripts\..\SCons\Tool\default.py", line 41: SCons.Tool.Tool(t)(env) File "S:\SCons\scons\scripts\..\SCons\Tool\__init__.py", line 273: self.generate(env, *args, **kw) File "S:\SCons\scons\scripts\..\SCons\Tool\mslink.py", line 310: msvc_setup_env_once(env) File "S:\SCons\scons\scripts\..\SCons\Tool\MSCommon\vc.py", line 805: msvc_setup_env(env) File "S:\SCons\scons\scripts\..\SCons\Tool\MSCommon\vc.py", line 949: d = script_env(use_script) File "S:\SCons\scons\scripts\..\SCons\Tool\MSCommon\vc.py", line 747: stdout = common.get_output(script, args) File "S:\SCons\scons\scripts\..\SCons\Tool\MSCommon\common.py", line 274: error='raise') File "S:\SCons\scons\scripts\..\SCons\Action.py", line 797: pobj = subprocess.Popen(cmd, **kw) File "subprocess.py", line 729: File "subprocess.py", line 1017:

where:

try: call ... except Exception as e: raise Exception(str(e))

is only about 6 lines shorter in this case:

Exception: [WinError 2] The system cannot find the file specified: File "S:\SCons\Test-Arg\SConstruct", line 29: env = Environment(MSVC_USE_SCRIPT=r'C:\test_batch-file_does-not_exist.bat') File "S:\SCons\scons\scripts\..\SCons\Environment.py", line 1010: apply_tools(self, tools, toolpath) File "S:\SCons\scons\scripts\..\SCons\Environment.py", line 116: _ = env.Tool(tool) File "S:\SCons\scons\scripts\..\SCons\Environment.py", line 1879: tool(self) File "S:\SCons\scons\scripts\..\SCons\Tool\__init__.py", line 273: self.generate(env, *args, **kw) File "S:\SCons\scons\scripts\..\SCons\Tool\default.py", line 41: SCons.Tool.Tool(t)(env) File "S:\SCons\scons\scripts\..\SCons\Tool\__init__.py", line 273: self.generate(env, *args, **kw) File "S:\SCons\scons\scripts\..\SCons\Tool\mslink.py", line 310: msvc_setup_env_once(env) File "S:\SCons\scons\scripts\..\SCons\Tool\MSCommon\vc.py", line 805: msvc_setup_env(env) File "S:\SCons\scons\scripts\..\SCons\Tool\MSCommon\vc.py", line 949: d = script_env(use_script) File "S:\SCons\scons\scripts\..\SCons\Tool\MSCommon\vc.py", line 747: stdout = common.get_output(script, args) File "S:\SCons\scons\scripts\..\SCons\Tool\MSCommon\common.py", line 276: raise Exception(str(e))

Other than the length of the traceback, is there a compelling reason to even have the try/except block?

I'm happy to make whatever changes that are deemed best in this case.

After sleeping on it and a few more tests this morning, I propose the following change which uses a bare raise and also writes a message to the debug output and to stderr:

try: popen = SCons.Action._subproc(env, call_str, stdin='devnull', stdout=subprocess.PIPE, stderr=subprocess.PIPE, error='raise') except: errmsg = 'Subprocess failed calling command: {}'.format(call_str) debug(errmsg) print(errmsg, file=sys.stderr) raise

Debug output to stdout (SCONS_MSCOMMON_DEBUG=-) yields:

... use_script 1 'C:\\test_batch-file_does-not_exist.bat' PATH: C:\Windows\System32;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0 Calling '"C:\test_batch-file_does-not_exist.bat"' Subprocess failed calling command: "C:\test_batch-file_does-not_exist.bat" & set

Stderr yields:

Subprocess failed calling command: "C:\test_batch-file_does-not_exist.bat" & set FileNotFoundError: [WinError 2] The system cannot find the file specified: File "S:\SCons\Test-Arg\SConstruct", line 29: env = Environment(MSVC_USE_SCRIPT=r'C:\test_batch-file_does-not_exist.bat') File "S:\SCons\scons\scripts\..\SCons\Environment.py", line 1010: apply_tools(self, tools, toolpath) File "S:\SCons\scons\scripts\..\SCons\Environment.py", line 116: _ = env.Tool(tool) File "S:\SCons\scons\scripts\..\SCons\Environment.py", line 1879: tool(self) File "S:\SCons\scons\scripts\..\SCons\Tool\__init__.py", line 273: self.generate(env, *args, **kw) File "S:\SCons\scons\scripts\..\SCons\Tool\default.py", line 41: SCons.Tool.Tool(t)(env) File "S:\SCons\scons\scripts\..\SCons\Tool\__init__.py", line 273: self.generate(env, *args, **kw) File "S:\SCons\scons\scripts\..\SCons\Tool\mslink.py", line 310: msvc_setup_env_once(env) File "S:\SCons\scons\scripts\..\SCons\Tool\MSCommon\vc.py", line 805: msvc_setup_env(env) File "S:\SCons\scons\scripts\..\SCons\Tool\MSCommon\vc.py", line 949: d = script_env(use_script) File "S:\SCons\scons\scripts\..\SCons\Tool\MSCommon\vc.py", line 747: stdout = common.get_output(script, args) File "S:\SCons\scons\scripts\..\SCons\Tool\MSCommon\common.py", line 278: error='raise') File "S:\SCons\scons\scripts\..\SCons\Action.py", line 797: pobj = subprocess.Popen(cmd, **kw) File "subprocess.py", line 729: File "subprocess.py", line 1017:

As this is a user-facing error, writing the call string to stderr prior to the exception could prove useful for diagnostic purposes.

@mwichmann Would this be a better approach?

Have to think about it. In this case we could instead raise an SCons error, which will reduce the call stack. Although maybe a little too terse?

except FileNotFoundError as e: raise SCons.Errors.UserError(e) from None

Yes, it's a UserError only if we can prove the SConscript requested a non-existent file, not if something went wrong on the system. It was just an illustration I could get to trigger to make sure I didn't have the syntax wrong :)

A bit off topic: an enhancement that I was planning on proposing once this was resolved is the addition of a user specified script argument: MSVC_USE_SCRIPT_ARG to be used in conjunction with MSVC_USE_SCRIPT. This would allow any flags recognized by the msvc batch files to be passed along with the script name.

For example, this would allow the following which currently can't be achieved directly in a SConstruct/SConscript file without resorting to a user-defined batch file (VS2022 Preview, amd64 target, toolset 14.2, sdk 10.0.19041.0):

use_version = '14.3' use_script = 'c:\\software\\msvs143bt-pre\\VC\\Auxiliary\\Build\\vcvarsall.bat' use_script_arg = 'amd64 --vcvars_ver=14.2 10.0.19041.0' env = Environment(MSVC_VERSION = use_version, MSVC_USE_SCRIPT = use_script, MSVC_USE_SCRIPT_ARG = use_script_arg)

If something like this were added, there would be no good way to validate the user provided script argument other than call subprocess. In this case it could be harder to distinguish between a user error and a system error.

Unfortunately, current msvc logic will not pick up batch file errors correctly (unless cl.exe is not added to the path). There is room for improvement in this area.

>>> "room for improvement" in msvc_tool True

:-)

Further off-topic: If a user script argument were allowed and an overloaded version when MSVC_USE_SCRIPT=None allowing a dictionary to be passed, it is possible in user space to unify/resolve most of the outstanding msvc issues/requests (preview, toolsets, components, etc).

Working SConstruct fragments via script argument enhancements:

from vsdetect import scons_msvc_byinstance as scons_msvc # SCons runs batch file: MSVC_USE_SCRIPT (string) and MSVC_USE_SCRIPT_ARG (string) env = Environment(**scons_msvc(MSVC_VERSION='14.3', MSVC_PREVIEW=True, MSVC_COMPONENT='BuildTools', MSVC_TOOLSET='14.2', TARGET_ARCH='amd64', MSVC_ADD_ARG='10.0.19041.0', run_script=False)) # user runs batch file: MSVC_USE_SCRIPT (None) and MSVC_USE_SCRIPT_ARG (dictionary) env = Environment(**scons_msvc(MSVC_VERSION='14.3', MSVC_PREVIEW=True, MSVC_COMPONENT='BuildTools', MSVC_TOOLSET='14.2', TARGET_ARCH='amd64', MSVC_ADD_ARG='10.0.19041.0', run_script=True)) # lookup by c runtime env = Environment(**scons_msvc(MSVC_RUNTIME='140', MSVC_PREVIEW=True, MSVC_COMPONENT='BuildTools', MSVC_TOOLSET='14.2', TARGET_ARCH='amd64', MSVC_ADD_ARG='10.0.19041.0', run_script=True)) # lookup by toolset env = Environment(**scons_msvc(MSVC_TOOLSET='14.2', MSVC_PREVIEW=True, MSVC_COMPONENT='BuildTools', TARGET_ARCH='amd64', MSVC_ADD_ARG='10.0.19041.0', run_script=True))

This would also allow user-side caching if the overloaded dictionary support were added.

@jcbrill - can you file an issue about additional arguments to vcvarsall.bat, I have some additional thoughts which would be good to capture in such an issue rather than here.

mwichmann · 2021-11-19T19:40:57Z

I'm getting a little confused here... there's already a try block in Action.py:_subproc, and passing raise to that call means it will catch an exception and reraise it. Maybe the additional processing should live there, and not with the caller (vc.py in this case)? The more modern interfaces in subprocess (like run, which is replacing check_call) will raise a CalledProcessError now, should we do the same in _subproc?

        # from subprocess.run
        if check and retcode:
            raise CalledProcessError(retcode, process.args,
                                     output=stdout, stderr=stderr)

jcbrill · 2021-11-19T21:04:35Z

Passing error='raise' to _subproc was added to bypass creation and return of the dummyPopen object which causes the msvc code path to fail with cascading errors.

It would seem better to have some of the error handling in _subproc and do as little as possible in get_output.

There are a number of calls to _subproc from other modules though so I would be a bit wary of unintended consequences for other code paths.

From the 3.10 subprocess documentation:

Exceptions raised in the child process, before the new program has started to execute, will be re-raised in the parent.

The most common exception raised is OSError. This occurs, for example, when trying to execute a non-existent file. Applications should prepare for OSError exceptions. Note that, when shell=True, OSError will be raised by the child only if the selected shell itself was not found. To determine if the shell failed to find the requested application, it is necessary to check the return code or output from the subprocess.

A ValueError will be raised if Popen is called with invalid arguments.

check_call() and check_output() will raise CalledProcessError if the called process returns a non-zero return code.

All of the functions and methods that accept a timeout parameter, such as call() and Popen.communicate() will raise TimeoutExpired if the timeout expires before the process exits.

Exceptions defined in this module all inherit from SubprocessError.

From my reading of the documentation, CalledProcessError is one of at least three base exception types that can be raised by subprocess.

For better or worse, that was why the code as submitted was catching/re-raising OSError, ValueError, and SubprocessError and then any other exception (Exception) solely to preserve the base exception type and minimize the traceback.

I'm leaning towards believing that the value added of that approach is minimal at best compared to catching an exception, writing any debug messages, writing any diagnostic messages to stderr, and then re-raising whatever was caught in get_output.

bdbaddog · 2021-11-21T21:15:20Z

+  From Joseph Brill:
+    - Verify that a user specified msvc script (via MSVC_USE_SCRIPT) exists and raise an exception
+      when the user specified msvc script does not exist.
+    - Explicitly request _subproc to raise exceptions rather than returning dummyPopen objects when


Change to SCons.Action._subproc() please.

also please add blurb for both of these to RELEASE.txt (in the appropriate sections)

bdbaddog · 2021-11-21T21:26:16Z

Can you break this into two PR's one to check if the specified script exists, and one to deal with the exception issue? Maybe drop the exception handling out of this one and file a separate for the rest.

The checking for specified script is a no-brainer to merge, rather not hold that up while pondering the impact of the other changes.

In general single item PR's are preferred.

jcbrill · 2021-11-21T22:01:23Z

Can you break this into two PR's one to check if the specified script exists, and one to deal with the exception issue? Maybe drop the exception handling out of this one and file a separate for the rest.

The checking for specified script is a no-brainer to merge, rather not hold that up while pondering the impact of the other changes.

Easy enough.

In general single item PR's are preferred.

Understood.

also please add blurb for both of these to RELEASE.txt (in the appropriate sections)

@bdbaddog The existing RELEASE.txt (differences since 4.2) or should there be a "new" empty one (differences since 4.3)?

bdbaddog · 2021-11-22T01:29:54Z

Can you add a test and and update to RELEASE.txt ?

jcbrill · 2021-11-22T14:23:19Z

Can you break this into two PR's one to check if the specified script exists, and one to deal with the exception issue? Maybe drop the exception handling out of this one and file a separate for the rest.

Will issue other PR after this PR is resolved.

@jcbrill - can you file an issue about additional arguments to vcvarsall.bat, I have some additional thoughts which would be good to capture in such an issue rather than here.

Will do.

Can you add a test and and update to RELEASE.txt ?

Release updated. Perhaps adding RELEASE.txt to the contributor checklist template would be helpful as well:

I have updated RELEASE.txt

I will try to add a test.

Python unit tests in general, and scons tests in particular, are completely foreign to me. Any hints or pointing me in the direction of a known existing test might accelerate the process. Teach a man to fish...

bdbaddog · 2021-11-23T17:56:30Z

@jcbrill - Hmm I'm surprised we don't have any tests for MSVC_USE_SCRIPT.

Here's the wiki page on testing.
https://github.com/SCons/scons/wiki/TestingMethodology

Also you can take a look at test/MSVC/TARGET_ARCH.py or other test in that dir.
Maybe copy that and use as a base.

Ideally you'd specify a non-existing MSVC_USE_SCRIPT and then run scons and check that stderr matches what you expect. and that SCons exits with error code.

…ed via MSVC_USE_SCRIPT. Update CHANGES.txt and RELEASE.txt and adjust whitespace.

Manually resolve conflicts in CHANGES.txt and RELEASE.txt

jcbrill · 2021-11-26T15:14:58Z

Test for user-specified script not found exception added.

Whitespace updates in both CHANGES.txt and RELEASE.txt for consistency with earlier/existing style/versions. If not desired, original style can be restored.

bdbaddog · 2021-11-26T21:01:25Z

@@ -0,0 +1,53 @@
+#!/usr/bin/env python


Can you update the header copyright blurb with the same content from templates/Tests.py

That has the updated copyright blurb.

I'll fix and push.

jcbrill added 6 commits November 14, 2021 06:49

Verify user script (MSVC_USE_SCRIPT) exists and raise an exception if…

a360f8d

… the user script is not found.

Rename FileNotFound to MSVCScriptNotFound and improve exception text.

11d0a50

Let _subproc raise exception to bypass dummyPopen cascading error iss…

b25731e

…ues and raise appropriate exception. Build call string rather than inline string in two separate invocations of _subproc.

Merge branch 'master' into jbrill-msvc-usescript

f147fdf

Merge branch 'master' into jbrill-msvc-usescript

c9813a1

Update CHANGES.txt for verifying MSVC_USE_SCRIPT script exists and ex…

8fab39e

…plicitly requesting _subproc to raise exceptions bypassing dummyPopen objects.

jcbrill mentioned this pull request Nov 18, 2021

dummyPopen object returned due to subprocess error causes unrelated exception in msvc/windows #4058

Closed

mwichmann reviewed Nov 18, 2021

View reviewed changes

bdbaddog reviewed Nov 21, 2021

View reviewed changes

Remove _subproc changes.

4ecf30f

jcbrill changed the title ~~Bypass dummyPopen object return from subprocess in msvc/windows~~ Verify that a user-specified script via MSVC_USE_SCRIPT exists Nov 21, 2021

bdbaddog and others added 3 commits November 21, 2021 19:48

Merge branch 'master' into jbrill-msvc-usescript

9b55ce8

Add improvement text and update versions numbers in RELEASE.txt

af3c540

Update description.

7c439c3

mwichmann added the MSVC Microsoft Visual C++ Support label Nov 23, 2021

mwichmann added this to the 4.4 milestone Nov 23, 2021

jcbrill added 2 commits November 26, 2021 09:59

Add test to verify script not found exception for msvc script specifi…

8eaf16b

…ed via MSVC_USE_SCRIPT. Update CHANGES.txt and RELEASE.txt and adjust whitespace.

Merge branch 'master' into jbrill-msvc-usescript

09c7515

Manually resolve conflicts in CHANGES.txt and RELEASE.txt

bdbaddog reviewed Nov 26, 2021

View reviewed changes

update copyright blurb to current version

6e6cc40

bdbaddog merged commit c15eee2 into SCons:master Nov 26, 2021

jcbrill deleted the jbrill-msvc-usescript branch November 26, 2021 21:15

bdbaddog modified the milestones: 4.4, NextRelease Dec 1, 2021

jcbrill mentioned this pull request Feb 26, 2022

MSVC script enhancements including user-defined arguments #4106

Closed

Uh oh!

Conversation

jcbrill commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contributor Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwichmann commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcbrill commented Nov 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdbaddog commented Nov 21, 2021

Uh oh!

jcbrill commented Nov 21, 2021

Uh oh!

bdbaddog commented Nov 22, 2021

Uh oh!

jcbrill commented Nov 22, 2021

Uh oh!

bdbaddog commented Nov 23, 2021

Uh oh!

jcbrill commented Nov 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jcbrill commented Nov 18, 2021 •

edited

Loading

mwichmann commented Nov 19, 2021 •

edited

Loading