Nope.
Add another variable if you want to be able to pass arguments to the script.
MSVC_SCRIPT_ARGS would be a reasonable name for such.

Actually keep this PR, but change to be a separate variable..

Actually keep this PR, but change to be a separate variable..

@bdbaddog Done per your comments.

@bdbaddog What do I need to do for this PR? The status (no comments, no merge in days) looks not good.

@bdbaddog What do I need to do for this PR? The status (no comments, no merge in days) looks not good.

looks not good?
You do understand this is a volunteer run project?

We do the best that we can.

As discussed in #4106, it might be better if the variable name for the string argument(s) when using MSVC_USE_SCRIPT is MSVC_USE_SCRIPT_ARGS (item 1 in #4106).

There is a second use case where a pass-through argument could be allowed when using the "normal" msvc detection that is likely a better use of the variable name MSVC_SCRIPT_ARGS (item 2 in #4106).

As discussed in #4106, it might be better if the variable name for the string argument(s) when using MSVC_USE_SCRIPT is MSVC_USE_SCRIPT_ARGS (item 1 in #4106).

There is a second use case where a pass-through argument could be allowed when using the "normal" msvc detection that is likely a better use of the variable name MSVC_SCRIPT_ARGS (item 2 in #4106).

Is it likely that you'd need to specify both? If not having two different variable names may only cause confusion..

Also does it make sense for any _SCRIPT_ARTS to be a subst()'able string?

Also does it make sense for any _SCRIPT_ARTS to be a subst()'able string?

I'm not sure.

MSVC_USE_SCRIPT_ARGS probably should accept a scalar string or a list of strings. A list of string arguments can be more palatable from a user perspective.

Also, it would likely be a good idea to verify the argument is in fact a string or that the flattened list contains all strings and otherwise raise a user argument exception immediately with a sensible error message. It is probably bad news to pass an incorrect type down through the script environment calls.

This is what I have been using in my implementation:

class MSVCScriptArgsError(VisualCException):
    pass
...

    if SCons.Util.is_String(use_script):
        use_script = use_script.strip()
        if not os.path.exists(use_script):
            raise MSVCScriptNotFound('Script specified by MSVC_USE_SCRIPT not found: "{}"'.format(use_script))
        use_script_args = env.get('MSVC_USE_SCRIPT_ARGS', None)
        if use_script_args is not None:
            use_script_args_orig = use_script_args
            use_script_args_list = SCons.Util.flatten(use_script_args)
            try:
                use_script_args = ' '.join(use_script_args_list)
            except TypeError as e:
                error_msg = 'Script arguments specified by MSVC_USE_SCRIPT_ARGS must be a string or a sequence of strings:\nvalue: {}\n{}'.format(use_script_args_orig, str(e))
                raise MSVCScriptArgsError(error_msg)
            use_script_args = use_script_args.strip()
            if not use_script_args:
                use_script_args = None
        debug('MSVC_USE_SCRIPT String %s %s', repr(use_script), repr(use_script_args))
        d = script_env(use_script, use_script_args)
...

Off-topic trivia:

# FYI: MSVC_VERSION is necessary if there are no detected msvc instances (otherwise never invoked).

# This "passes" on 32-bit windows and 64-bit windows
env32 = Environment(
    MSVC_USE_SCRIPT      = 'C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\Bin\\SetEnv.cmd',
    MSVC_USE_SCRIPT_ARGS = '/Release /x86 /2003'
)

# This "passes" on 32-bit windows and FAILS on 64-bit windows (no cl.exe on path)
env64 = Environment(
    MSVC_USE_SCRIPT      = 'C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\Bin\\SetEnv.cmd',
    MSVC_USE_SCRIPT_ARGS = '/Release /x64 /2003'
)

Any guesses?

Off-topic trivia:

# FYI: MSVC_VERSION is necessary if there are no detected msvc instances (otherwise never invoked).

# This "passes" on 32-bit windows and 64-bit windows
env32 = Environment(
    MSVC_USE_SCRIPT      = 'C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\Bin\\SetEnv.cmd',
    MSVC_USE_SCRIPT_ARGS = '/Release /x86 /2003'
)

# This "passes" on 32-bit windows and FAILS on 64-bit windows (no cl.exe on path)
env64 = Environment(
    MSVC_USE_SCRIPT      = 'C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\Bin\\SetEnv.cmd',
    MSVC_USE_SCRIPT_ARGS = '/Release /x64 /2003'
)

Any guesses?

Do you have a copy of the debug log for both and/or what happens if you just run that on the command line? (I don't have MSVC 6 anywhere at the moment)

Renamed variable per discussion in #4106 also the variable can now be a list or space separated.
Added test.

Also does it make sense for any _SCRIPT_ARTS to be a subst()'able string?

I'm not sure.

MSVC_USE_SCRIPT_ARGS probably should accept a scalar string or a list of strings. A list of string arguments can be more palatable from a user perspective.

Also, it would likely be a good idea to verify the argument is in fact a string or that the flattened list contains all strings and otherwise raise a user argument exception immediately with a sensible error message. It is probably bad news to pass an incorrect type down through the script environment calls.

This is what I have been using in my implementation:

class MSVCScriptArgsError(VisualCException):
    pass
...

    if SCons.Util.is_String(use_script):
        use_script = use_script.strip()
        if not os.path.exists(use_script):
            raise MSVCScriptNotFound('Script specified by MSVC_USE_SCRIPT not found: "{}"'.format(use_script))
        use_script_args = env.get('MSVC_USE_SCRIPT_ARGS', None)
        if use_script_args is not None:
            use_script_args_orig = use_script_args
            use_script_args_list = SCons.Util.flatten(use_script_args)
            try:
                use_script_args = ' '.join(use_script_args_list)
            except TypeError as e:
                error_msg = 'Script arguments specified by MSVC_USE_SCRIPT_ARGS must be a string or a sequence of strings:\nvalue: {}\n{}'.format(use_script_args_orig, str(e))
                raise MSVCScriptArgsError(error_msg)
            use_script_args = use_script_args.strip()
            if not use_script_args:
                use_script_args = None
        debug('MSVC_USE_SCRIPT String %s %s', repr(use_script), repr(use_script_args))
        d = script_env(use_script, use_script_args)
...

I'll agree, but I think out of scope of this particular PR?
In theory we'd fully validate the _ARGS as it's a knowable allowable set, but I think that as well as simpler validation can be left to another PR.

As is this PR is usable and will solve an existing issue for some users.

Do you have a copy of the debug log for both and/or what happens if you just run that on the command line? (I don't have MSVC 6 anywhere at the moment)

I already know the answer. This nicely encapsulates the issues with debugging this sort of problem.

The crux of the problem is the aggressively minimal SCons environment in which the msvc batch files are executed: when run directly from the command-line there are no errors/warnings. To add insult to injury, even when the MSCommon debugging is enabled, the "raw" stdout is not logged.

SCons does not include some of the windows system environment variables used by the setenv.cmd batch file.

SetEnv.cmd fragment:

IF "%TARGET_CPU%" =="x64" (
 IF "%PROCESSOR_ARCHITECTURE%" == "AMD64" ( 
   IF EXIST "%VCTools%\x64\cl.exe" (
   SET "VCTools=%VCTools%\x64"
   ) ELSE (
   SET VCTools=
   ECHO The x64 compilers are not currently installed.
   ECHO Please go to Add/Remove Programs to update your installation.
   ECHO .
   )
 ) ELSE IF "%PROCESSOR_ARCHITEW6432%"=="AMD64" ( 
     IF EXIST "%VCTools%\x64\cl.exe" (
     SET "VCTools=%VCTools%\x64"
     ) ELSE (
     SET VCTools=
     ECHO The x64 compilers are not currently installed.
     ECHO Please go to Add/Remove Programs to update your installation.
     ECHO .
     )
 ) ELSE ( 
     IF EXIST "%VCTools%\x86_x64\cl.exe" (
     SET "VCTools=%VCTools%\x86_x64;%VCTools%"
     ) ELSE (
     SET VCTools=
     ECHO The x64 compilers are not currently installed.
     ECHO Please go to Add/Remove Programs to update your installation.
     ECHO .
     )
   ) 

The PROCESSOR_ARCHITECTURE and PROCESSOR_ARCHITEW6432 environment variables are not defined. This causes the else clause to be executed. On 64-bit platforms, the x64 compiler is installed in the x64 folder. On 32-bit platforms, the 64-bit compiler is installed in the x86_x64 folder. Therefore, x86_x64 does not exist and the message is echoed to stdout.

The message "The x64 compilers are not currently installed" is in fact generated when executed from SCons.

If the output were captured it would look something like this (taken from my own tools):

        "STDOUT": [
            "",
            "",
            "The x64 compilers are not currently installed.",
            "Please go to Add/Remove Programs to update your installation.",
            ".",
            "Setting SDK environment relative to C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0. ",
            "Targeting Windows Server 2003 x64 RELEASE",
            "",
            "*** WARNING ***",
            "You are currently building on a Windows 9x based platform.  Most samples have ",
            "NMAKE create a destination directory for created objects and executables.  ",
            "There is a known issue with the OS where NMAKE fails to create this destination",
            "directory when the current directory is several directories deep.  To fix this ",
            "problem, you must create the destination directory by hand from the command ",
            "line before calling NMAKE. ",
            ""
            ""
        ],

The warning is innocuous and generated because the "OS" environment variable is not defined. The older files expect OS to be defined. This is why the msvc 6.0 paths are in quotes: the batch file thinks it is 95/98/ME since OS is not "Windows_NT"

In addition, the SDK setenv.cmd batch files expect delayed expansion to be enabled which is typically not the default in a windows shell.

The /x86 target environment is "incomplete" but works anyway. Here is the resulting environment for the x86 invocation:

{
    "ENV": {
        "PATH": [
            "C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\VC\\Bin",
            "C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\Bin",
            "\\Microsoft.NET\\Framework\\v2.0.50727",
            "!Path!",
            "C:\\Windows\\System32"
        ],
        "INCLUDE": [
            "C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\VC\\Include",
            "C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\VC\\Include\\Sys",
            "C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\Include",
            "C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\Include\\gl",
            "!Include!"
        ],
        "LIB": [
            "C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\VC\\Lib",
            "C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\Lib",
            "!Lib!"
        ],
        "LIBPATH": [],
        "VSCMD_ARG_app_plat": "",
        "VCINSTALLDIR": "C:\\Software\\MSVS-2005-80-Pro\\VC\\",
        "VCToolsInstallDir": ""
    },
    "TARGET_ARCH": null,
    "MSVC_USE_SCRIPT": "C:\\Program Files\\Microsoft SDKs\\Windows\\v6.0\\Bin\\SetEnv.cmd",
    "MSVC_USE_SCRIPT_ARGS": "/Release /x86 /2003",
    "MSVC_VERSION": "14.3"
}

Due to delayed expansion not being enabled, !PATH!, !Include!, and !Lib! are not expanded. Also, the Framework path is incomplete due to "windir" not being defined.

SDK 6.0 contains stand-alone msvc 8.0 compilers.

I have a mind-bending intermediate windows batch file that sets the environment as appropriate in a setlocal/local block, calls the SDK batch file, and "exports" the non-windows wow64/system variables back into what would be the msvc batch file environment.

If interested, I could attach.

@jcbrill would you mind moving the last few above to your issue #4106 ?
Seems a better place to have/keep info from this discussion?

@bdbaddog sure.

@yuzhichang - do the changes I've made work for your use model? If so I'll go ahead and merge.

@bdbaddog LGTM. Thanks for the review.