MSVC: detection fixes #4534
MSVC: detection fixes #4534
Conversation
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 og 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 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.
…c path from json output. Test failures when the vc path from json is normalized. Test failure for AddOption help output.
…where command-line argument
…turning None. Change processing of user-specified vswhere path. TODO: * revisit MSVC.Util.process_path due to realpath behavior for some windows specifications (drive specifications, etc).
Changes: * Added MSVC detection priority. * Added VS2015 edition limitations. * Updated SDK batch file known issues. * Added footnotes for MSVC batch file argument limitations. * Added footnote for Windows SDK version numbers (Windows 10 and Windows 11)
Manually resolved conflicts in CHANGES.txt and SCons/Tool/MSCommon/vc.py.
…ical errors to prepare for merge with latest.
Manually resolve conflicts: * CHANGES.txt * RELEASE.txt * SCons/Tool/MSCommon/vc.py
Add vswhere roots for: * winget * scoop
Manually resolve conflicts: CHANGES.txt RELEASE.txt SCons/Tool/MSCommon/vc.py
Changes: * Bug fix for not found toolset version in vcTests.py * Re-work sdk list tests in vcTests.py when run on machine that may not have sdks installed (no msvc or older msvc). Potential future problem lurking as the gap between supported sdk versions grows with future releases. * Update the tolerance for precompiled header "fast enough" in msvc.py. This test, more than others, has a tendency to fail when run in a virtual machine on Windows. * Explicity run "foo.exe" instead of "foo" in vs-14.3-exec.py. A recent change in VS2022 appears to create a "foo" folder which causes the test to fail.
…tructures when vswhere finds new msvc roots.
Manually resolve conflicts: * SCons/Tool/msvc.xml
…thon). Changes: * An express installation of the IDE binary is used when no other IDE edition is detected. * A full development edition (e.g., Professional) of VS2008 is elected before a Visual C++ For Python edition. * Update detection order in README.rst and remove hard tabs. * Minor fixes lingering from original VSWHERE implementation where all call chains were not updated when the environment argument was added.
vswhere changes: * Split the current vswhere paths list into two lists: * the visual studio installer locations, * the package manager locations. * Add additional user vswhere priority groups: * before the vs installer locations [high priority], * after the vs installer locations [default priority], and * after the package manager locations [low priority]. * Adjust tests for vswhere path list changes accordingly. * Add vswhere location functions to: * register user vswhere executable paths by priority, * get the currently configured vswhere executable location, * freeze the vswhere executable location used for detection. * Assign the environment VSWHERE construction variable in Tool/MSCommon/vc.py. * Remove the assignment of the environment VSWHERE construction variable in Tool/msvc.py * Update the test suite function signature for finding the msvc product directory with vswhere. * Freeze the vswhere executable in the following functions before the cache is evaluated which will raise an exception if the vswhere executable has changed since the initial detection (if necessary): * MSCommon.vc.get_installed_visual_studios, * MSCommon.vc.find_vc_pdir, * MSCommon.vc.msvc_setup_env, * MSCommon.vs.get_installed_visual_studios
Manually resolved: * CHANGES.txt * RELEASE.txt
Manually resolve conflicts: * RELEASE.txt
Okay, will have a look. |
Wow... that (and the referenced template) are pretty messy.... |
|
Finally got a chance to check, this does work for me with a 17.10 install. |
A PR (#4552 ) was issued to improve this - feel free to have another loo |
# Manually resolved conflicts: # RELEASE.txt
|
|
||
| # productdir kind | ||
|
|
||
| VCVER_KIND_UNKNOWN = 0 # undefined |
There was a problem hiding this comment.
You could use a python enum here.
There was a problem hiding this comment.
An enum could certainly be used.
Any chance this could be deferred to a post-merge cleanup/additions?
| @@ -574,57 +613,68 @@ def _msvc_version_toolset_vcvars(msvc, vc_dir, toolset_version): | |||
|
|
|||
| def _msvc_script_argument_toolset_constraints(msvc, toolset_version): | |||
|
|
|||
| if msvc.vs_def.vc_buildtools_def.vc_version_numeric < VS2017.vc_buildtools_def.vc_version_numeric: | |||
| if msvc.vs_def.vc_buildtools_def.msvc_version_numeric < VS2017.vc_buildtools_def.msvc_version_numeric: | |||
There was a problem hiding this comment.
You have several instances of this condition, might be clearer to just have a simple function to do this?
There was a problem hiding this comment.
At present, I'm not sure it would clearer to move to a function. For some, it may be harder to comprehend what exactly is being compared.
While long-ish dereference chains might not be everyone's cup of tea, as written, they are simple one-line numeric comparisons. The source of the resolved values are immediately apparent by inspection.
What is getting increasingly complicated to comprehend is which of the many msvc version numbers are being used.
In local prototypes, judiciously adding properties to some of the data structures could be an effective method of visually reducing the dereference chains. The idea is that each property might resolve one or more dereferences. Basically, making a derived/embedded field appear as if it is a local attribute.
Something like:
msvc.vs_def.vc_buildtools_def.msvc_version_numeric < 2017.vc_buildtools_def.msvc_version_numeric
becomes:
msvc.vs_def.msvc_version_numeric < 2017.msvc_version_numeric
Unfortunately, this causes a non-trivial increase in the number of lines of code.
If directed, moving some the comparisons to functions could be straightforward. I'm not wild about it though.
| @@ -51,7 +51,7 @@ | |||
| # print k, v | |||
| #MergeMSVSBatFile(env, 9.0) | |||
| #print(env['ENV']['PATH']) | |||
| print(query_versions()) | |||
| print(query_versions(env=None)) | |||
There was a problem hiding this comment.
Doesn't the env default to none here? Better to test without specifying then I think users won't typically specify env=None..
There was a problem hiding this comment.
Doesn't the env default to none here?
It does now, it did not take any arguments before.
I'm happy to remove if you think it best. But...
This is an artifact of one of issues with the user specification of the VSWHERE construction variable.
When the VSWHERE construction variable was implemented, not all of the call chain "roots" where the VSWHERE construction variable could be populated were addressed. This was not obvious due to the environment argument being optional in many function implementations. Basically, all of the call chains needed to accept an optional environment argument if one was not already defined.
Mini-rant: the optional environment argument makes the vc code a lot harder to comprehend because it is not readily apparent what might be retrieved from the environment, if it exists, in the bowels of the call chains.
The function query_versions did not accept any arguments but could cause resolution of the vswhere location in the vc code. Because query_versions did not accept an argument, it was not possible to pass an environment that might contain the construction variable VSWHERE.
The query_versions function call in the test was changed to take an argument explicitly assigning None to the environment as a visual reminder that an optional environment argument is now accepted. When doing source tree diffs and searches, it was easier to determine if all of the call chains were inspected.
In the future, a test could be added that accepted a VSWHERE executable location which might be something that a user would want to specify.
|
Ideally this would have been a number of smaller PRs. 35 files and > 1200 lines changed makes it challenging to review carefully. If possible some docstrings on functions would be helpful for the review process, and also in the future should you not be available to fix a bug. When you get a chance to address the notes from this pass, we should be able to merge, and then will most likely release a new SCons version. Thanks for all your work here! |
I understand and agree in principle.
As mentioned in the top post above (#4534 (comment)), the vast majority of functional changes are limited to 5 source files one of which is new. Changes to msvc functionality are typically in Full local testing of msvc changes is time consuming which usually involves running the approximately 80 msvc/msvs tests on two physical computers (amd64 and arm64) and in 6-10 virtual machines followed by running the full test suite on at least one physical computer. AFAIK, the test suite is run against every currently supported version of MSVC for both 32-bit and 64-bit hosts across all virtual machines. Local tests are usually run using the oldest version of python supported (i.e., 3.6). Even then, some errors are caught by the configuration of the SCons test environments and the whole cycle starts anew. The changes in this PR originated prior to the last SCons release. It has been my experience that there is a flurry of activity prior to each release that can trigger time consuming updates for any and all branches that do not make it into the release. With the exception of supporting v14.40 toolsets, this code has been frozen for quite some time and no new development has been initiated locally as it likely will be dependent on the outcome of this PR.
Following adoption of this PR and/or post-release, I would like to do some post-merge maintenance which may involve some code movement and internal variable renaming. If possible could the following be deferred until at least after this PR is merged?
I believe all notes have been addressed in some detail with the exception of use of an enum. |
Yes. That would yield a much more manageable and simpler PR to merge than updating this one. |
|
I guess a question here is whether there's a need to mention (or even just for us to know) that there are code paths that cause the |
|
There would be a problem if something triggered an msvc detection before a user environment with a VSWHERE location specification that is different than the default vswhere location. An exception would be raised. The "new" vswhere executable code is written in such a way that a function could be added to the Util module to register a vswhere executable location with a priority (e.g., before the default vs location, after the default vs location but before the package manager locations, or after the package manager locations). This would allow user-defined vswhere executable paths to be registered from a custom site folder or sconstruct/sconscript files without requiring an environment for tools specification and/or a VSWHERE construction variable. Specification via VSWHERE in an environment would still always be the "highest" priority. The suggestion to add a wrapper function to the Util module is so that the API documentation includes the function. I could be wrong, but everything under the Tools folder is ignored when generating the API documents. Just a thought. |
That's correct. Only the top level (basically the module init file) is processed, none of the tools themselves. It's that way in the Sphinx build... because it was that way with the earlier doc generation system, where the tool went unmaintained, and we had to drop it, therefore I assumed it was intentional. Not hard to add, but I'm not sure if this is valuable or not... msvc is massive, and everything else is largely undocumented (oversimplification, but moderately accurate). |
|
In My guess is that is due to the dreaded precompiled headers is not fast enough test. In this PR, I increased the multiplier from 0.90 to 1.00 (i.e., precompiled headers should be the same as without or faster). I'm not sure how accurate this test is anymore. For simple scenarios it may be that the precompiled header overhead is more than the gains achieved. However, on production projects there could be sizable performance gains for large libraries. This is the one test that occasionally fails in virtual machines because who knows what Windows is doing. A rambling way to say:
|
|
I'm not fond of the speed tests - you've nailed the reasons. The ninja build one sometimes also fails, but usually doesn't - which is really irritating. A small test isn't very indicative anyway; a CI environment is whatever it is - the host serving that image this time could be really busy, or not very busy. When I push the requested doc massage, I could drop that test at the same time and see what happens. @bdbaddog ? |
Yes. |
|
Indeed... it's still considered a speedup, but one with costs: manual work has to be done to set it up; it's no longer the "recommended" approach, as I've noted somewhere else - that's from |
|
The proposed changes for the request about the |
Closes #17254 Summary of the issue: We are currently on SCons version 4.5.2, which was released in March of 2023. The latest version of SCons, version 4.8.1, was released in September 2024. Description of user facing changes: None. Description of development approach * Updated the required version of SCons from 4.5.2 to 4.8.1 in `requirements.txt`. * Changed the order of arguments to `SCons.Tool.MSCommon.vc.find_vc_pdir` to that expected by SCons 4.8. * Previously, the order of arguments was `environment, version`. As of SCons 4.8, it is `version, environment`. * Changed in SCons/scons#4534 Testing strategy: Built the project locally and on Appveyor. Known issues with pull request: None.
This PR fixes issues surrounding MSVC detection and user specification of the vswhere executable via the construction variable VSWHERE.
This PR is similar to the MSVC detection changes originally proposed in #4409. In hindsight, it was a mistake in #4409 to move beyond the initial detection changes and attempt to incorporate preview versions.
It is my intention to abandon #4409 as it is unlikely to be adopted in any reasonable time frame due to the quantity of uncommitted local changes and the number of updates to the main branch while the branch was under development.
For purposes of exposition, the changes have been partitioned into two logical categories: detection and vswhere.
Changes:
detection:
For msvc version specifications without an 'Exp' suffix, an express installation is used when no other edition is detected for the msvc version.
Similarly, an express installation of the IDE binary is used when no other IDE edition is detected.
VS2015 Express (14.0Exp) does not support the sdk version argument. VS2015 Express does not support the store argument for target architectures other than x86.
Script argument validation now takes into account these restrictions.
VS2015 BuildTools (14.0) does not support the sdk version argument and does not support the store argument.
Script argument validation now takes into account these restrictions.
The Windows SDK for Windows 7 and .NET Framework 4" (SDK 7.1) populates the registry keys in a manner in which the msvc detection would report that VS2010 (10.0) is installed when only the SDK was installed.
The installed files are intended to be used via the sdk batch file setenv.cmd. The installed msvc batch files will fail. The msvc detection logic now ignores SDK-only VS2010 installations.
Similar protection is implemented for the sdk-only installs that populate the installation folder and registry keys for VS2008 (9.0), if necessary.
For VS2005 (8.0) to VS2015 (14.0), vsvarsall.bat is employed to dispatch to a dependent batch file when configuring the msvc environment.
Previously, only the existence of the compiler executable was verified. In certain installations, the dependent batch file (e.g., vcvars64.bat) may not exist while the compiler executable does exist resulting in build failures. The existence of vcvarsall.bat, the dependent batch file, and the compiler executable are now validated.
MSVC configuration data specific to versions VS2005 (8.0) to VS2008 (9.0) was added as the dependent batch files have different names than the batch file names used for VS2010 (10.0) and later.
The VS2008 (9.0) Visual C++ For Python installation is handled as a special case as the dependent batch files are: (a) not used and (b) in different locations.
When VS2008 (9.0) Visual C++ For Python is installed using the ALLUSERS=1 option (i.e., msiexec /i VCForPython27.msi ALLUSERS=1), the registry keys 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.
For VS2008, a full development edition (e.g., Professional) is now selected before a Visual C++ For Python edition.
Prior to this change, Visual C++ For Python was selected before a full development edition when both editions are installed.
The registry detection of VS2015 (14.0), and earlier, is now cached at runtime and is only evaluated once for each msvc version.
The detection order and ranking of msvc instances for VS 2017 and later is now deterministic.
Visual Studio 2022 v143 BuildTools now supports msvc toolset versions from 14.30 to 14.4X.
Fixes Issue Microsoft toolset 14.40 does not work with SCons 4.7 and earlier #4543.
vswhere:
The vswhere executable is frozen upon initial detection.
Specifying a different vswhere executable via the construction variable VSWHERE after the initial detection now results in an exception.
Multiple bugs in the implementation of specifying a vswhere executable via the construction variable VSWHERE have been fixed.
Previously, when a user specified vswhere executable detects new msvc installations after the initial detection, the internal msvc installation cache, default msvc version, and msvs installation cache based on the initial detection are invalid. For example, when no vswhere executable is found for the initial detection and then later an environment is constructed with a user specified vswhere executable that detects new msvc installations.
The vswhere detection of VS2017 (14.1), and later, is now cached at runtime and is only evaluated once using a single vswhere invocation for all msvc versions.
Previously, the vswhere executable was invoked for each supported msvc version.
The vswhere executable locations for the WinGet and Scoop package managers were added to the default vswhere executable search list after the Chocolatey installation location.
Edits:
Contributor Checklist:
CHANGES.txt(and read theREADME.rst)