[WIP] MSVC: detection fixes and changes

[jcbrill]

Changes:

• VS/VC roots are classified by their installed features (e.g, devenv.com, vcexpress.com, etc.). This provides for special-case verification of batch file arguments based on the internal classification.
• Consistent with earlier behavior, express versions are used for the non-express msvc version symbol for 14.1 and 8.0 when the express version is the only version installed.
• There is a strong possibility that 14.0Exp may not have been detected correctly. It appears that registry keys for earlier versions of msvc are not populated. Refined detection of 14.0Exp was added.
• Special case handling of VS2015 BuildTools was added. The msvc batch files restrict the arguments available as compared to full versions. The arguments may be accepted and ignored possibly resulting in build failures that are likely not easy to diagnose.
• Special case handling of VS2015 Express was added. The msvc batch files restrict the arguments available as compared to full versions. For example, store/UWP build are only available for x86 targets.
• Windows/Platform SDK installations of 7.1, 7.0, and 6.1 populate registry keys and installation folders that were detected by scons (versions 10.0 and 9.0). Unfortunately, the generated files are intended to be used via SetEnv.cmd and result in errors. The detection of sdk-only installations was added and the roots are ignored.
• The relative imports of the MSCommon module were changed to top-level absolute imports in a number of microsoft tools. Moving any of the tools to the site tools folder failed on import (i.e., the relative paths become invalid when moved).
• VS2005 to VS2015 batch file (vcvarsall.bat) dispatches to a dependent batch file when configuring the msvc environment. In certain installation scenarios, the dependent batch file (e.g., vcvars64.bat) may not exist. The existence of vcvarsall.bat, the dependent batch file, and the compiler executable are now verified.
• MSVC configuration data specific to versions VS2005 to VS2008 was added as the dependent batch files have different names than the batch files for VS2010 and later. VC++ For Python is handled as a special case as the dependent batch files: are not used and are in different locations.
• When VC++ For Python is installed using the ALLUSERS=1 command-line option, the registry keys written are under HKLM rather than HKCU. VC++ For Python installed for all users is now correctly detected.
• The existing detection configuration for vswhere and the registry was refactored to separate the two methods of detection.
• The detection of the msvc compiler executable has been modified and no longer considers the os environment. The detection of the msvc compiler executable was modified to provide more detailed warning messages.

Internal code changes:

• Rename VISUALSTUDIO_ to MSVS_PRODUCT and vs_def to vs_product_def in all files.
• Replace process_path with normalize_path with additional arguments in MSCommon.MSVC.Util.
• Add msvs channel, msvs component id, and msvs component definitions.
• Add Validate module to MSCommon.MSVC.
• Add AutoInitialize class to MSCommon for object initialization upon class definition completion.
• Add custom formatter to MSCommon debug logging to prepend the class name to the function name in the log record based on passed arguments.
• Rework MSCommon.Kind to return the msvs component, vs directory, vs executable, and vc feature map.
• Combine _VCVER_TO_PRODUCT_DIR vc version keys into vs product keys and use MSCommon.MSVC.Kind to determine the vs component type.
• Add file exists and normalized path caches to MSCommon.vc.
• Rework vswhere executable management and queries.
• Add an object hierarchy for vs/vc detection. Include preliminary data structure construction for msvs channels (preview, release, any) and component types (Enterprise, Professional, Community, BuildTools, Express, etc).
• Select an msvc instance rather than a vc dir and pass the instance to MSCommon.MSVC.ScriptArgument methods.
• Rework msvc action selection (use script, select, use settings, bypass).

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 the appropriate documentation

[jcbrill]

[STICKY] More detailed notes:

• During msvc detection of installed MSVC versions, vs/vc roots are classified by their installed features (e.g., devenv.com, vcexpress.exe, etc.). This provides for special-case verification of extended argument support based on the internal classification.

  With the implementation of the internal classification, argument validation can take into account installed features (or the lack thereof) of an msvc product that would not otherwise be known from the msvc version alone. For example, there are differences between the capabilities of VS2015 and VS2015 BuildTools installations despite sharing the same msvc version of 14.0. See below for details.

• Due to the manner in which registry keys are populated for VS2015 and earlier, an express version is typically detected as both the non-express version and the express version. For example, when VS2012 Express is installed, both 11.0 and 11.0Exp are detected as installed. Basically, an express version is being used for the non-express symbol when there are no other editions of the same version installed. However, this behavior did not exist for 8.0 and 14.1. The detection of 8.0 and 14.1 was modified to provide similar behavior as all other versions: if only the express version is detected the same installation is used for the non-express version symbol.

• There is a strong possibility that VS2015 Express was not being detected as 14.0Exp at all. From limited test environments, VS2015 Express does not appear to populate the VCExpress registry key as was done by earlier express versions of Visual Studio. The VS2015 Express version can be detected via other MSVS registry keys and the SxS VC key which requires the installation classification disambiguation.

• The msvc batch files for the VS2015 BuildTools installation restricts the feature set that is available with a "full" (e.g., Community) installation. The VS2015 BuildTools vcvarsall.bat batch file may dispatch to a special build tools batch file under certain conditions. The special build tools batch file appears to: (a) accept and silently ignore the store/uwp argument and (b) ignore the sdk version argument. The argument restrictions are now detected during validation based on the internal classification.

  Note: due to the default folder layout, a default installation of the VS2015 BuildTools on a 32-bit host appears to work as a full installation version.

• The msvc batch files for the VS2015 Express installation restricts the feature set that is available with a "full" (e.g., Community) installation. The VS2015 Express target batch files for amd64 and arm (i.e., vcvarsx64_amd64.bat and vcvarsx86_arm.bat) do not support the store/uwp argument as the store library paths are (intentionally) incorrect. The store/uwp argument is supported for x86 targets. The sdk version argument is not supported.

• Msvc version 10.0 was detected when only the "Windows SDK for Windows 7 and .NET Framework 4" (SDK 7.1) is installed due to the manner in which the registry keys are populated. Given a default installation, the msvc batch files will fail. An sdk-only installation appears to be correctly detected by inspecting both the SxS VS and VC registry keys. An sdk-only installation is now ignored. Similar protection is implemented for for VS2008 (9.0) which may be created by multiple windows/platform SDKs.

• The MSCommon module import was changed from a relative import to a top-level/absolute import in the following Microsoft tools: midl, mslib, mslink, mssdk, msvc, msvs.

  Moving any of the tools using relative imports to the site tools folder would fail on import (i.e., the relative import paths become invalid when moved).

• For VS2005 to VS2015, vsvarsall.bat is employed to dispatch to a dependent batch file when configuring the msvc environment. Previously, only the compiler executable was verified. In certain installation scenarios, the dependent batch file (e.g., vcvars64.bat) may not exist while the compiler executable does exist. The existence of vcvarsall.bat, the dependent batch file, and the compiler executable are now validated.

  MSVC configuration data specific to versions VS2005 to VS2008 was added as the dependent batch files have different names than the batch file names used for VS2010 and later.

  The VC++ For Python installation is handled as a special case as the dependent batch files are: (a) not used and (b) in different locations.

• When Visual C++ For Python is installed using the ALLUSERS=1 option (i.e., msiexec /i VCForPython27.msi ALLUSERS=1), the registry keys written are written to HKEY_LOCAL_MACHINE rather than HKEY_CURRENT_USER. An entry was added to query the Visual C++ For Python keys in HKLM following the HKCU query, if necessary.

• The existing product detection configuration and implementation for vswhere and the registry was refactored to separate the two methods of detection.

  The registry detection is cached at runtime and only evaluated for each msvc version once.

  The vswhere detection will only run each vswhere executable once. The detected msvc instances for all vswhere executables are merged (i.e., the detected msvc instances are the union of all vswhere executables).

  Allowing the vswhere executable to be specified via the environment is at odds with portions of the current implementation. For instance, the installed vcs list is constructed once and cached typically during the default environment initialization. If a vswhere executable is registered that has not been seen before, the json output is processed and merged with the existing detected msvc instances. Dependent cache data structures area cleared only if new msvc installations are discovered.

• The detection loops were modified to examine the existence of the msvc product directory inside the query loops rather than outside the query loops. When a queried product directory exists the loop is exited. This removes the need to raise an internal exception to indicate a missing configuration. The rationale being that a valid product directory might be found by continuing to evaluate the results of any remaining queries.

• The detection of the msvc compiler executable has been modified. The compiler detection no longer considers the host environment path. In addition, existence of the msvc compiler executable is checked in the detection dictionary and the scons ENV path before the detection dictionary is merged into the scons ENV.

  Note; the detection of the msvc compiler in the vc module always checks for the full name (i.e., cl.exe). The search in the Util module and the CC construction variable use only the base name cl which employs the windows PATHEXT environment variable. It is possible that the detected compiler executable in the vc module and the cl found on the command-line are not the same binary.

  Different warnings are generated when the msvc compiler is not detected in the detection dictionary based on whether or not an msvc compiler was detected in the scons ENV path (i.e., already exists in the user's ENV path prior to detection).

• The environment argument (i.e., env) for the msvc product detection routines was moved to the end of the argument list from the front of the argument list in some cases.

• The msvc and msvs tests were updated accordingly.

Suggested changes:

• Prefer a "full" version of VS2008 over the VC++ For Python version of VS2008. Currently, a detected VC++ For Python installation is used when a full version of VS2008 is installed. This would be consistent with other MSVC versions where a development version is preferred over a build tools version.

Test Environments

Environ			ARM	AMD	DEV	DEV	BT	BT	EXP	EXP	SDK	SDK
		ID	1	2	3	4	5	6	7	8	9	10
		Host	PC	PC	VM	VM	VM	VM	VM	VM	VM	VM
		Arch	64	64	64	32	64	32	64	32	64	32
		Windows	11	10	11	10	11	10	11	10	11	10
MSVS	Kind	MSVC
2022	Com	14.3	*	*	*
2022	BT	14.3	*	*			*	*
2019	Com	14.2		*	*	*
2019	BT	14.2	*	*			*	*
2017	Com	14.1		*	*	*
2017	BT	14.1	*	*			*	*
2017	Exp	14.1Exp		*					*	*
2015	Com	14.0		*	*	*
2015	BT	14.0					*	*
2015	Cmd	14.0	*
2015	Exp	14.0Exp							*	*
2013	Com	12.0		*	*	*
2013	Exp	12.0Exp							*	*
2012	Pro	11.0			*	*
2012	Exp	11.0Exp		*					*	*
2010	Pro	10.0			*	*
2010	Exp	10.0Exp		*					*	*
2008	Pro	9.0		*	*	*
2008	Py	9.0		*			*1	*1
2008	Exp	9.0Exp							*	*
2005	Pro	8.0		*	*	*
2005	Exp	8.0Exp							*	*
2003	Pro	7.1		*	*	*
2002	Pro	7.0		*	*	*
6.0	Pro	6.0		*	*	*
WinSDK	Kind	MSVC
7.1	SDK	10.0		*	*	*	*	*	*	*	*	*
7.0	SDK	9.0		*	*	*	*	*	*	*	*	*
6.1	SDK	9.0		*	*	*	*	*	*	*	*	*
6.0	SDK	8.0		*	*	*	*	*	*	*	*	*
2003R2	SDK	8.0		*	*	*	*	*	*	*	*	*
2003SP1	SDK	8.0		*	*	*	*	*	*	*	*	*
2003	SDK	7.1

Footnotes:

• 1: VC++ For Python installer command-line: msiexec /i VCForPython27.msi ALLUSERS=1

Kind description:

• Com: Community
• BT: BuildTools
• Exp: Express
• Cmd: CommandLine (e.g., v140 tools installed via VS2017-VS2022)
• Pro: Professional
• Py: VC++ For Python
• SDK: Windows/Platform SDK (some versions include stand-alone compilers)

Express Detection Changes

Results from VMs with only express versions installed.

VCVer	Installed	Main	4409
14.1			*
14.1Exp	*	*	*
14.0		*	*
14.0Exp	*		*
12.0		*	*
12.0Exp	*	*	*
11.0		*	*
11.0Exp	*	*	*
10.0		*	*
10.0Exp	*	*	*
9.0		*	*
9.0Exp	*	*	*
8.0			*
8.0Exp	*	*	*

Stress Tests

Environments build count and msvc cache entry count by test environment (i.e., combinations of msvc batch file arguments).

Id	Builds	Cache
1	768	768
2	1744	1697
3	952	952
4	291	291
5	891	891
6	467	467
7	132	67
8	132	67
9	0	0
10	0	0

[jcbrill]

@mwichmann @bdbaddog I need some guidance. Any takers?

Providing the ability to specify the vswhere path on the command-line would allow the user-defined path specification to be used when constructing the default environment tools (i.e., the initial detection). This also allows a SetOption from an sconstruct and/or site initialization file to set the value of the option rather than on the command-line proper.

Specifying the vswhere path via an Environment likely is not available when the default environment tools are initialized. Since the installed vcs list is cached, if new roots are discovered by the user-defined vswhere path (which is presumably why it is being specified), the installed vcs list needs to be cleared and recomputed. The proposed branch does this.

Specifying the vswhere path via the environment makes the implementation more complicated that it would be otherwise. If the vswhere path specification via the environment were deprecated and eventually eliminated, detection of msvc instances can be done once and never again as all requisite information is available at the time of initial detection.

I believe there are at least two methods for implementing the vswhere path command-line option.

Any and all advice is welcome.

Method A - Built-in option in SCons.Script.SConsOption.py

1. SCons.Script.SConsOptions.Parser:

   op.add_option('--vswhere-path',
                 nargs=1, type="string",
                 dest="vswhere_path", default=None,
                 action="store",
                 help="Path for vswhere.exe",
                 metavar="EXEPATH")
   

2. SCons.SCript.SConsOptions.SConsValues

   settable = [
       ...
       'vswhere_path',
       ...
   ]
   

Method 2 -- AddOption in SCons.Tool.MSCommon.vc.py

1. SCons.Tool.MSCommon.vc.py:

   SCons.Script.AddOption(
       '--vswhere-path',
       dest='vswhere_path',
       type="string",
       nargs=1,
       action="store",
       metavar='EXEPATH',
       default=None,
       help='Path for vswhere.exe',
   )
   

2. SCons.SCript.SConsOptions.SConsValues

   settable = [
       ...
        # TODO: Remove these once we update the AddOption() API to allow setting
        #       added flag as settable.
        # Requested settable flag in : https://github.com/SCons/scons/issues/3983
        # from Tool.MSCommon.vc
       'vswhere_path',
       ...
   ]
   

3. test\Help.py

   One of the help test fails due to an additional local option which is not expected:

   <   --debugging  Compile with debugging symbols
   ---
   >   --debugging             Compile with debugging symbols
   >   --vswhere-path=EXEPATH  Path for vswhere.exe
   

   The test has to updated for the additional argument on platforms that load the msvc tools (win32 only?).

   Should other, possibly platform-specific, local options be added this could make configuring the expected local options text more complicated.

[mwichmann]

Sigh... I've just been hacking on a complaint about paths, which shows up here: #4410. This still isn't completely fleshed out, but stuff that came tumbling out when I sat down to actually write it. There's an item there about user-supplied paths...

And, yes, there's already an issue complaining about the problems of tool modules that optionally add cli arguments: #3455.

And, yes, the tests are painfully fragile when they expect specific outputs, but that output is variable based on external factors. Usually it means writing an even more complex regex than the one that was in place before.

So all this is known stuff, and I have no answers...

[jcbrill]

I realize you have a lot on your plate and am not trying to pile on.

#4410 is well written and is kind of depressing. #3455 (and the referenced issue) is more or less what I'm struggling with here.
Thanks for the references.

As a middle ground, could internal modules supporting tools (e.g., the MSCommon.vc module supports msvc, mslib, mslink) be added as "built-in"options conditionally based on the platform in script options in the near term?

For example, the "--vswhere-path" option would only be added to the parser definitions on win32 platforms.

The downside is that the help message would vary by platform which may not be the worst thing in the world as the local options would only be added on win32 platforms anyway. There are likely more downsides of which I am blissfully unaware.

The upside is that the options are not "local options" so they would be displayed with -H for the scons script and do not require a user Help invocation in the sconstruct/site initialization nor would the test suite have to be modified.

[bdbaddog]

@jcbrill - any --vswhere would need be a separate PR. I'd advocate for --vswhere and not --vswhere-path. If there's not already a GH issue for this problem, please create one and add your proposed solutions there? We can discuss there?

[bdbaddog]

You'll need to add blurb to CHANGES.txt and RELEASE.txt.

[bdbaddog]

why make this change? (and all the similar changes)

[jcbrill]

The MSCommon module import was changed from a relative import to a top-level/absolute import in the following Microsoft tools: midl, mslib, mslink, mssdk, msvc, msvs.

Moving any of the tools using relative imports to the site tools folder would fail on import (i.e., the relative import paths become invalid when moved).

Should just work moving to scons-site folder. Was doing some testing and the imports failed.

[bdbaddog]

So you'd want moving some part of the msvc logic to site-scons, and not all of it to work?

[jcbrill]

Yes. If one wanted to copy and customize a microsoft tool like mslib.py locally. After copying mslib.py to the tools folder in the site-scons folder and running scons, the copied tool import would fail as the relative imports are no longer valid.

The MSCommon module was the only module in the microsoft tools that was using a relative import. All others were absolute.

This is not something that I'm passionate about.

[bdbaddog]

ahh ok.
I think I misread this previously. So this isn't about moving part of a tool to site-tools, but rather a whole tool, which in this case depends on common logic in SCons.Tool

O.k. Then yes these changes make total sense. Good catch.

[jcbrill]

Any --vswhere would need be a separate PR. I'd advocate for --vswhere and not --vswhere-path. If there's not already a GH issue for this problem, please create one and add your proposed solutions there? We can discuss there?

I thought I read somewhere that an environment variable was added based on the name. There already exists a VSWHERE construction variable. Thought there might be a conflict.

Happy to move to another PR, it was just easier in this one since the handling of vswhere executables has been modified anyway and there is a function to evaluate if a command-line argument was set (which could just be no-op stub returning None). No harm, no foul.

[bdbaddog]

Any --vswhere would need be a separate PR. I'd advocate for --vswhere and not --vswhere-path. If there's not already a GH issue for this problem, please create one and add your proposed solutions there? We can discuss there?

I thought I read somewhere that an environment variable was added based on the name. There already exists a VSWHERE construction variable. Thought there might be a conflict.

Happy to move to another PR, it was just easier in this one since the handling of vswhere executables has been modified anyway and there is a function to evaluate if a command-line argument was set (which could just be no-op stub returning None). No harm, no foul.

In general. The smaller more focus each PR has, the easier it is for me to review and merge. The larger it gets, the review time will increase almost exponentially..

[jcbrill]

In both the existing code and the proposed code, the subprocess call for vswhere has the check=True option set. A CalledProcessError is not caught and likely would result in script exit with that exception. OSError exceptions are caught and ignored. I think that it might be a good idea to do the same for the 'CalledProcessError` exception: catch and ignore rather than raise.

Anyone else OK with that?

It might matter if there is an issue with vs installed vswhere.exe which would likely cause the script to fail during default msvc tools initialization. In this case, the user-specified vswhere via the Environment may not get a chance to run which kind of defeats the purpose of being able to set it in the environment (something I would like to see removed).

[bdbaddog]

In both the existing code and the proposed code, the subprocess call for vswhere has the check=True option set. A CalledProcessError is not caught and likely would result in script exit with that exception. OSError exceptions are caught and ignored. I think that it might be a good idea to do the same for the 'CalledProcessError` exception: catch and ignore rather than raise.

Anyone else OK with that?

It might matter if there is an issue with vs installed vswhere.exe which would likely cause the script to fail during default msvc tools initialization. In this case, the user-specified vswhere via the Environment may not get a chance to run which kind of defeats the purpose of being able to set it in the environment (something I would like to see removed).

What would cause this type of failure? Bad command line args to vswhere?

[jcbrill]

In both the existing code and the proposed code, the subprocess call for vswhere has the check=True option set. A CalledProcessError is not caught and likely would result in script exit with that exception. OSError exceptions are caught and ignored. I think that it might be a good idea to do the same for the 'CalledProcessError` exception: catch and ignore rather than raise.
Anyone else OK with that?
It might matter if there is an issue with vs installed vswhere.exe which would likely cause the script to fail during default msvc tools initialization. In this case, the user-specified vswhere via the Environment may not get a chance to run which kind of defeats the purpose of being able to set it in the environment (something I would like to see removed).

What would cause this type of failure? Bad command line args to vswhere?

My understanding is that when check=True if the executed command returns a non-zero exit code(i.e., the return code of the process is what is being checked) a CalledProcessError is raised containing the exit code. Basically if vswhere.exe itself has a non-normal exit.

I suppose it is possible that bad arguments could cause vswhere.exe to exit with a non-zero code. I have not tried it but it should not be too difficult to test. There might be any number of OS events that could cause vswhere.exe to fail internally.

If subprocess.run is passed invalid arguments a ValueError should be raised.

[bdbaddog]

I'd rather you didn't add any additional features/logic to this PR. It's already very large and will take quite some time to properly review.

[jcbrill]

I'd rather you didn't add any additional features/logic to this PR. It's already very large and will take quite some time to properly review.

That's fine. There are no more features planned.

Are you ok with catching and ignoring CalledProcessError? Like OSError, the error string is logged to the debug file if enabled.

Basically, from:

        except OSError as e:
            errmsg = str(e)
            debug("%s: %s", type(e).__name__, errmsg)
            break

to:

        except (OSError, subprocess.CalledProcessError) as e:
            errmsg = str(e)
            debug("%s: %s", type(e).__name__, errmsg)
            break

[bdbaddog]

I'd rather you didn't add any additional features/logic to this PR. It's already very large and will take quite some time to properly review.

That's fine. There are no more features planned.

Are you ok with catching and ignoring CalledProcessError? Like OSError, the error string is logged to the debug file if enabled.

Basically, from:

        except OSError as e:
            errmsg = str(e)
            debug("%s: %s", type(e).__name__, errmsg)
            break

to:

        except (OSError, subprocess.CalledProcessError) as e:
            errmsg = str(e)
            debug("%s: %s", type(e).__name__, errmsg)
            break

That'd be a logic change. See previous statement.. ;)

[jcbrill]

There's a reason there is WIP in the title ;)

[jcbrill]

[STICKY] Notes to self for post-review cleanup:

• MSCommon.vc.py, lines 59-60: delete comment and blank line

  # import SCons.Script
  
  

• MSCommon.MSVC.Kind.py, line 79: delete erroneous comment for field

      'kind',  # relpath from pdir to vsroot
  

• MSCommon.MSVC.Kind.py, line 473: delete unnecessary assignment
  (needed before vc_dir was added to cache key: likely copied not moved)

          vc_dir = os.path.normcase(os.path.normpath(pdir))
  

• MSCommon.vc.py, line 1102: revisit subprocess arguments and exception handling (see comment below: [WIP] MSVC: detection fixes and changes #4409 (comment))

  
       try:
           cp = subprocess.run(vswhere_cmd, stdout=PIPE, stderr=PIPE, check=True)
       except OSError as e:
           errmsg = str(e)
           debug("%s: %s", type(e).__name__, errmsg)
           break
       except Exception as e:
           errmsg = str(e)
           debug("%s: %s", type(e).__name__, errmsg)
           raise
  

[mwichmann]

In both the existing code and the proposed code, the subprocess call for vswhere has the check=True option set.

That could be an error (on my part). That flag was added after examining linter complaints:

W1510: Using subprocess.run without explicitly set `check` is not recommended. (subprocess-run-check)

It's quieter now, but... that meant we changed from the default (False) to True. If it stays true, then the code needs to catch the exception, but I'd probably just change it to false.

[jcbrill]

Revised SCons.Tool.MSCommon.README.rst document may provide additional context when reviewing implementation changes.

[jcbrill]

There's a VM with just 2022 Preview in it; and there's a setup with many things installed including the preview. Is either one of those better to use? Or both?

It shouldn't matter as long as the command line options are enabled and Preview is passed on the command line:

set SCONS_ENABLE_MSVC_OPTIONS=True
python ./jbrill-msvc-fixes/scripts/scons.py ... --msvs-channel=Preview

Whatever is easiest for you.

My primary tests have been on a development box with many installed visual studios and VS2022 with both release (community and build tools) versions and both preview (community and build tools) versions installed.

[mwichmann]

There's a VM with just 2022 Preview in it; and there's a setup with many things installed including the preview. Is either one of those better to use? Or both?

It shouldn't matter as long as the command line options are enabled and Preview is passed on the command line:

set SCONS_ENABLE_MSVC_OPTIONS=True
python ./jbrill-msvc-fixes/scripts/scons.py ... --msvs-channel=Preview

SCons Error: no such option: --msvs-channel

this is with what gh has named jbrill-msvc-fixes checked out.

Edit: sorry... operator error, on the VM in question, what I always have set to open up a cmd shell in this case did a powershell. Retrying.

[mwichmann]

And, it works to build. Sorry about the false alarm, guess my setup wasn't consistent and I got lazy.

That VM has no other install, and the build fails without that argument with the usual

'cl' is not recognized as an internal or external command, 
operable program or batch file.

So that's good!

[jcbrill]

@mwichmann Thank you. The help is appreciated.

[mwichmann]

So just to put this out there... this mechanism adds an optional command-line argument to pass arguments to the msvc subsystem. It looks, however, quite different from how another optional feature, ninja, is enabled. To be enable the chance to use ninja, you use a commandline option (--experimental), while this uses an environment variable to enable. They're not the same kind of feature, admittedly, but there might be a desire to have more user interface consistency.

Also, I was trying to figure out if I could enable this without putting on the commandline. You can't run the testsuite with non-standard arguments directly, you'd have to modify every single launch. However, SCONSFLAGS should be usable for avoiding having to put the argument on the commandline. I can't make it work, however (in very limited testing - and just for a simple hello, world build, not for the testsuite). Curious if you see the same problem.

[bdbaddog]

So just to put this out there... this mechanism adds an optional command-line argument to pass arguments to the msvc subsystem. It looks, however, quite different from how another optional feature, ninja, is enabled. To be enable the chance to use ninja, you use a commandline option (--experimental), while this uses an environment variable to enable. They're not the same kind of feature, admittedly, but there might be a desire to have more user interface consistency.

Also, I was trying to figure out if I could enable this without putting on the commandline. You can't run the testsuite with non-standard arguments directly, you'd have to modify every single launch. However, SCONSFLAGS should be usable for avoiding having to put the argument on the commandline. I can't make it work, however (in very limited testing - and just for a simple hello, world build, not for the testsuite). Curious if you see the same problem.

Yes. Please use --experimental for this. Adding another env var should be the very last option used.

[jcbrill]

Banged out in a hurry, apologies for grammatical errors. If I didn't answer a question, post a gentle reminder.

To be enable the chance to use ninja, you use a commandline option (--experimental), while this uses an environment variable to enable.

The environment variable is temporary and is only used for my own development testing.

Unlike, the ninja and install sandbox arguments, it is preferable to define the command-line arguments at the file level rather than via a constructed environment.

The only reason that it wasn't added without the environment variable is that it causes one of the unit tests to fail. The command-line options end up being local options and the test (I can't recall which one) is matching the options that it added in the test file and is not expecting any other local arguments so the expected matched text fails.

The ability to set the channel and the component via the environment. was implemented, tested, and then removed. There is a non-trivial amount of validation that needs to happen so it was easier to take it out for now.

The development strategy thus far has been to make sure that without any additional information, the new code behaves identically to the old code.

Also, I was trying to figure out if I could enable this without putting on the commandline.

For a single run, there is an exposed method that can used to set the default channel. It needs to be called before first use in the msvc code.

The msvs_set_channel_default function available via from SCons.Tool.MSCommon import msvs_set_channel_default allows the default msvs channel to be set:

def msvs_set_channel_default(msvs_channel) -> bool:
    """Set the default msvs channel.
    Args:
        msvs_channel: str
            string representing the msvs channel
            Case-insensitive values are:
                Release, Rel, Preview, Pre, Any, *
    Returns:
        bool: True if the default msvs channel was accepted.
              False if the default msvs channel was not accepted.
    """
    rval = _VSChannel.set_default_channel(msvs_channel, 'msvs_set_channel_default')
    return rval

The implementation will use the command-line argument first, followed by the value set via the function above, or the initialized value (Release) if neither the command-line argument or the method call were defined.

This could be done in a site initialization file or an sconstruct file (subject to the fact that it need to be set before first retrieval, otherwise a warning is issued):

from SCons.Tool.MSCommon import msvs_set_channel_default
msvs_set_channel_default('Preview')

At present, without the command-line option or the method call mentioned above, the code acts as the current master (preview versions are ignored).

The reason is that there are issues that still need to be resolved surrounding:

• the vs.py code (vs instances, the installed vs list cached on the first call)
• taking components into account, the msvc installation selected may not be the same as the msvs selected
• the default msvc version in vc.py (different versions based on release/preview installations)
• msvc exists in vc.py (all installations)
• cached values in the vc.py (installed vc list)
• the default msvc version should not necessarily be forced when using auto-detection but it may have to be set if no versions are found.
• there are likely more issues lurking

[jcbrill]

@mwichmann If you want to run the test suite with only preview versions, you can change a hard-coded initialization value in SCons/Tool/MSCommon/MSVC/VSDetect.py

https://github.com/jcbrill/scons/blob/98dd971c4c8bab34af306b442e838e633b9dd13f/SCons/Tool/MSCommon/MSVC/VSDetect.py#L442-L467

Change cls.vs_channel_initial from:

 @classmethod
 def _initial(cls) -> None:
     cls.vs_channel_initial = Config.MSVS_CHANNEL_RELEASE

to:

 @classmethod
 def _initial(cls) -> None:
     cls.vs_channel_initial = Config.MSVS_CHANNEL_PREVIEW

[jcbrill]

However, SCONSFLAGS should be usable for avoiding having to put the argument on the commandline. I can't make it work, however (in very limited testing - and just for a simple hello, world build, not for the testsuite). Curious if you see the same problem.

Unfortunately, it's probably never going to work without modifications.

The issue appears to be that the command-line options are registered in the the MSCommon/Tool/MSVC/Options.py module. Any test that does not load the vc.py (MSCommon?) module is going to fail with a no such option: --msvs-channel error.

The options would have to be defined in SCons/Script/ScriptOptions.py to be considered "first-class" options always known to the parser.

[mwichmann]

Hmmm, might need to record that tidbit somewhere. The whole way the options are handled (other than the so-called "built-in options", which are mostly fine) gives lots of headaches. I didn't realize that SCONSFLAGS didn't work. It should, as those are added very early, before even the first effort at parsing:

def _exec_main(parser, values) -> None:
    sconsflags = os.environ.get('SCONSFLAGS', '')                  
    all_args = sconsflags.split() + sys.argv[1:]
    options, args = parser.parse_args(all_args, values)

[jcbrill]

SCONSFLAGS is doing what it is supposed to do.

The problem is that there are a lot of tests that don't necessarily load the module which adds the local options.

11/79 (13.92%) C:\Data\_env-368-64-scons\Scripts\python.exe test\MSVC\batch.py
FAILED test of S:\SCons\jbrill-msvc-channel\scripts\scons.py
	at line 673 of S:\SCons\jbrill-msvc-channel\testing\framework\TestCommon.py (_complete)
	from line 776 of S:\SCons\jbrill-msvc-channel\testing\framework\TestCommon.py (run)
	from line 458 of S:\SCons\jbrill-msvc-channel\testing\framework\TestSCons.py (run)
	from line 98 of test\MSVC\batch.py
S:\SCons\jbrill-msvc-channel\scripts\scons.py returned 2
STDOUT =========================================================================
scons: Reading SConscript files ...
scons: done reading SConscript files.

STDERR =========================================================================
usage: scons [OPTIONS] [VARIABLES] [TARGETS]

SCons Error: no such option: --msvs-channel

EDIT: THIS POST IS LIKELY INCORRECT PENDING FURTHER INVESTIGATION.

[jcbrill]

I'm not sure the previous post is correct.

[mwichmann]

Okay, I get it, now. Yes, there will be issues with this. In fact, many of the tests specifically don't want to load the module, as they intend to test paths through setting and retrieving construction variables. So stuffing an argument into SCONSFLAGS that won't always be recognized is clearly a recipe for failure. I'll withdraw my idea that this should work :-)

[jcbrill]

I expected this to work:

set SCONSFLAGS=--site-dir=S:\SCons\site-preview

Where S:\SCons\site-preview\site-init.py contains:

from SCons.Tool import MSCommon
MSCommon.msvs_set_channel_default('preview')

Of the 79 MSVS+MSVC tests, the following tests fail, I'm not sure why yet and can't look at until later tonight:

Failed the following 7 tests:
	test\MSVC\msvc_cache_force_defaults.py
	test\MSVC\MSVC_SDK_VERSION.py
	test\MSVC\MSVC_SPECTRE_LIBS.py
	test\MSVC\MSVC_TOOLSET_VERSION.py
	test\MSVC\MSVC_USE_SCRIPT_ARGS.py
	test\MSVC\MSVC_UWP_APP.py
	test\MSVC\no_msvc.py

[jcbrill]

@mwichmann A minor tweak was pushed that allows running the test suite with the default msvs channel being set to Preview.

TEMPORARY FIX: Instead of using the windows environment variable to enable command-line options, a windows environment variable was added that defines the default channel if the command-line option is not defined.

Sample usage:

rem msvc options variable not needed
set SCONS_ENABLE_MSVC_OPTIONS=

rem define default msvs channel
set SCONS_MSVS_CHANNEL_DEFAULT=Preview

<run test suite>

It is not all rainbows and unicorns though: at a minimum, the SCons\Tool\msvsTests.py test fails.

This is not particularly surprising as the code has been modified working from the inside out and only the release versions are strictly compatible with all test files. As mentioned above, there are known issues that have not been addressed yet. At this point in the development cycle, I typically only run the MSVS+MSVC tests (approximately 79 tests).

Feel free to take it for a spin around the block.

[jcbrill]

@mwichmann All tests appear to pass using preview-only versions with the latest commit (and there was much rejoicing).

Previous temporary environment variables were removed and replaced with a single environment variable that is similar to SCONSFLAGS. See below for more details.

To run the test suite with preview-only versions:

rem Define msvc options command-line arguments:
set SCONS_MSVC_OPTIONS=--msvs-channel=Preview

<run test suite>

To run the test suite normally:

rem Ensure undefined (or set to Release):
set SCONS_MSVC_OPTIONS=

<run test suite>

To run a single script with preview-only versions:

rem Overrides SCONS_MSVC_OPTIONS (if defined) when on command-line
python ./jbrill-msvc-fixes/scripts/scons.py ... --msvs-channel=Preview

To run a single script normally:

rem Ensure undefined (or set to Release):
set SCONS_MSVC_OPTIONS=

python ./jbrill-msvc-fixes/scripts/scons.py ...

Note: MSCommon.Tool.vs.py might misbehave and need a time-out.

Also, I was trying to figure out if I could enable this without putting on the commandline. You can't run the testsuite with non-standard arguments directly, you'd have to modify every single launch. However, SCONSFLAGS should be usable for avoiding having to put the argument on the commandline. I can't make it work, however (in very limited testing - and just for a simple hello, world build, not for the testsuite). Curious if you see the same problem.

Windows temporary environment variables SCONS_ENABLE_MSVC_OPTIONS and SCONS_MSVS_CHANNEL_DEFAULT have been removed and replaced with a new windows environment variable SCONS_MSVC_OPTIONS.

Environment variable SCONS_MSVC_OPTIONS works in spirit similar to SCONSFLAGS in that the msvc local command-line options can be defined via an environment variable rather than specified on the command-line directly.

The msvc local command-line options are always added when the SCons.Tool.MSCommon.MSVC.Options module is imported.

This caused an issue with test/Help.py which tests adding a local option and matching the expected output. test/Help.py was modified to detect if the msvc options module was imported and adjusts the expected output accordingly. Note: not all tests will load the msvc modules.

The current commit was tested locally as follows:

• all tests in WSL2/ubuntu
• all tests in Windows 10/python 3.6/amd64
• all tests in Windows 10/python 3.6/amd64/preview only
• limited tests in Windows 11/python 3.6/ARM64
• limited tests in Windows 11/python 3.6/ARM64/preview only
• limited tests in all 64/32-bit virtual machines as described above
• limited tests in all 64/32-bit virtual machines as described above/preview only

A number of internal sematic issues are still outstanding.