MSVC suppress warning for no installed Visual Studio instances for default tools configuration #4132
Conversation
…etup_env with undefined MSVC_VERSION (default tools).
|
Looks good. Also.. how to test this? |
Making the warning an exception solves the duplicate message problem :) Fail early and often. I'm only half kidding. I prefer an exception to a build that fails because cl.exe is not found. I'm probably in the minority though. Famous last words: "how hard could it be?". I shall investigate.
I'm assuming you mean adding a unit test. In the discussion for #2813, I used a vm without MSVS and mingw. It might be possible to "borrow" the fake registry and fake vswhere fixtures/code so that no MSVS instances are detected. I think at that point all that is needed is an environment that initializes. That step is beyond my current abilities. Might need to poke around. |
|
@jcbrill - indeed if you request a specific version and it's not there, throwing an exception and exiting is WAY better than "can't find cl.exe"... It might be reasonable to just make a unit test (such that vswhere and registry are never hit) for the test. |
|
If you mocked get_default_version() and just called msvc_setup_env().. it's probably doable? |
|
I'm in the midst of stuff and can't check - we should definitely have a way to mock the registry-fishing stuff, I kinda thought I'd seen such in some test but maybe it was some other context. |
Yup. There's mocked registry tests for sure. |
|
@bdbaddog I will look either later tonight or first thing in morning, but I THINK there may be multiple warnings due to the warning being issued inside the loop checking for host/target combinations. Each host/target fails because it does not exist and then just falls off the end of the loop end effectively returns an empty dictionary. If that is what is happending, a simple flag/count could be added to issue the warning once and then, if desired, raise an exception when the nothing was found instead of returning an empty dictionary. |
|
It may be even simpler than what I described. Need to check existing code and one of the PRs. The warning might be able to be suppressed altogether (commented out) and an exception raised prior to exit based on the dictionary being empty. Need to make sure the conditions under which this would happen (make sure does not effect default tool initialization). |
…efined dictionary when attempting to find a valid msvc batch script. Raise an MSVCVersionNotFound exception when the default msvc version is requested and there are no msvc versions installed. Suppress raising an MSVCVersionNotFound exception during default msvc tool initialization. Add additional tests.
|
Updates:
Code path cases are shown below. Test status summary:
Case 1 - Build with non-msvc tools with no msvc installed instances: Case 2 - Request default msvc with no msvc installed instances: Case 3 - Path 1 in msvc_find_valid_batch_script (induced via code hack): Case 4 - Path 2 in msvc_find_valid_batch_script: Case 5 - Path 3 in msvc_find_valid_batch_script: Case 6 - Path 4 in msvc_find_valid_batch_script (should never happen - induced by code hack): |
|
Returning an exception instead of warning now causes failures for the linux build(s). I'll look into soon... |
|
@jcbrill - can you clarify what Paths 1-4 are for? |
|
@bdbaddog Sure. Annotated find_valid_batch_script fragment (just prior to exit): if not d:
env['TARGET_ARCH']=req_target_platform
installed_vcs = get_installed_vcs(env)
if version_installed:
# PATH 1 - version detected, batch file failed
err_msg = "MSVC version {} working host/target script was not found.\n" \
" Host = {}, Target = {}\n" \
" Visual Studio C/C++ compilers may not be set correctly".format(
version, host_platform, target_platform
)
elif version and installed_vcs:
# PATH 2 - version is defined and there are installed vcs
err_msg = "MSVC version {} was not found.\n" \
" Visual Studio C/C++ compilers may not be set correctly.\n" \
" Installed versions are: {}".format(version, installed_vcs)
elif version:
# PATH 3 - version is defined and there are no installed vcs
err_msg = "MSVC version {} was not found.\n" \
" No versions of the MSVC compiler were found.\n" \
" Visual Studio C/C++ compilers may not be set correctly".format(version)
else:
# PATH 4 - version is undefined/None and/or there are no installed vcs
# if there are installed vcs, version should be defined
# if there are no installed vcs, empty default version should have failed in setup
err_msg = "No versions of the MSVC compiler were found.\n" \
" Visual Studio C/C++ compilers may not be set correctly"
raise MSVCVersionNotFound(err_msg)In english:
The intent was to provide an error message that conveys as much information as possible about why an empty dictionary would have been returned which would have ultimately lead to cl.exe not being found. |
|
The failures for the windows builds are almost all of the type version XX.X not found, (14.1 and 14.2 installed) for most of the msvs tests. That is, a version is hard-coded in the test that is not actually installed. Before, this would have issued a warning. Now it is an exception. The no_msvc test actually succeeded when it should have failed. Not sure why yet.,, |
Ah, yes, that's an old familiar pain point. Improve some code, no real code using it will ever have a problem, but the tests have hardcoded the exact behavior one is cleaning up, and so they have to be tweaked... been there many a time :-( |
|
I need some guidance. For the vm tests with only mingw installed and no msvc installations, I looked into Tool\mingw.py to figure out where it is looking for mingw. Since I build my own versions of mingw, I put a specific version folder in I could not understand why the default builds with no msvc installed instances were attempting to use msvc when there was a version of mingw-w64 installed. Forcing Here's the rub: due to an omission in tool\mingw.py, the This explains why the following fails with a specific version in the When the mingw code is adjusted to use the same paths in both This appears to a bug. Should I open an issue and a PR or just the PR? Suggestions appreciated. Aside: there is room for improvement with the order of the search paths, not hard-coding the windows system drive, and taking into account the platform bit size when looking for mingw/mingw-w64 folders (i.e., subfolder mingw32 vs mingw64). |
|
That tool initialization is poor is a known issue, subject of more than one issue and entries in "future generation blue-sky thinking" and so forth. The issue you're running into is that the two required functions aren't plumbed together - even if Unless @bdbaddog disagrees, an issue that documents that mingw doesn't check the same way in |
The root cause is that Mingw Exists is returning False when it should be returning True. No compilers in the win32 platform list are found so the list is intialized to the first element of the list which is 'msvc'. When Mingw exists returns True, it is the only compiler found in the 'win32' list and is used as expected. Currently, mingw generate uses "mingw_paths + find_version_specific_mingw_paths" while exists only uses "mingw_paths". The installed tools are not found by exists so it does is filtered from the platform compiler tools list. When the platform tools list is empty, the first element is used. In this case, msvc which does not exist. Ok. Will open an issue later today. |
|
@jcbrill - Yes, please add an issue re mingw paths not consistently used between |
|
@mwichmann I have a history with the default selection of mingw tools. I tend to have multiple versions and builds of mingw in none of the places that scons used to look. When cygwin was added to the list it broke my builds as the cygwin tools were added to the paths and I was manually appending the local tools. Had to switch to prepending local tools to env path. |
Right - I got that. I was saying that the two were out of skew - |
It appears that I am the misbehaving tool :) |
…s not found. Remove exception for default msvc version and adjust test accordingly.
…ible combinations. Update exception message formatting.
|
Apologies to @mwichmann and @bdbaddog for turning into a pumpkin for about two weeks. Life intervened in both expected and unexpected ways.
By suppressing the warning for the default tools and no MSVC_VERSION, it is possible that a build will fail (cl not found) without warning. The RELEASE.txt and CHANGES.txt should be fairly accurate with regard to this situation. I am going to have to run some tests with the current master for not found behavior. I'm a little fuzzy at the moment concerning the prior code and how not found was handled. I just need some time reviewing the existing code.
I don't have any that are ready to issue. The following are on my short-term radar:
The first item should probably be done before the next release as it completes the "escape hatch" mechanisms for msvc batch files until first-class support for features can be added (if ever). It does require some validation: that the msvc version batch file actually supports arguments. The content of the user The rest don't necessarily have to be included in the next release as there is no urgency. The PR for the preview version is woefully out-of-date and will take a non-trivial amount of work to revive. One or two of the msvc bug scrub items could probably be closed via adding documentation. Figuring out where would be another kettle of fish.
Understood. Will (likely) join soon... |
|
@jcbrill - Agreed that holding release for item 1 above is worthwhile. As far as validating the contents thereof, I think we can punt on that, and just file an enhancement issue once merged so we don't drop that. There may be reasonable ways to do something useful with validation, but certainly that shouldn't hold up progress. (since use would most likely be by "advanced" users?) Previous msvc logic would basically leave env['ENV'] without updates to find/run msvc if no version is found, yielding the "cl.exe not found". |
I agree. The intent with the "escape hatch" mechanisms was to provide one way to get around any limitations in the current implementation and not necessarily the best way. Basically, it is way to prevent a user getting "stuck" until something better comes along.
That was my immediate recollection as well. The current difference will be for the default tool list and no MSVC_VERSION specification: no warning issued and "cl.exe not found". The prior version would issue a warning for the default tools configuration. |
|
@jcbrill - re pumpkin. No worries, life happens. your continued contributions are very welcome and worth waiting for! :) |
|
Off topic for this thread: the following msvc-bug-scrub items likely should be tagged as "enhancement": #4106, #4048, #3664, #4149. I suppose the last two could be controversial. For the current architecture, it is an enhancement. Not sure if all remaining bug scrub items can make it into next release or not. The remaining items are fairly old so there is not really any apparent urgency. |
|
@bdbaddog and @mwichmann Please review at your convenience. Illustrations of old and new behavior. Comments and questions always welcome. Presented below are the results from 4 experimental configurations with 8 scons invocations (methods) for each experiment for the master and the warnfix PR. The results are presented below in a table for the master, a table for the warnfix PR, a table with the primary differences, and some notes. Wordsmithing the For the test methods with msvc installed, the default builds will succeed and the version specific builds will fail (9.0Exp is not installed). This was by design. Note: I could not figure out how to prevent word-wrap inside the tables. They are easier to interpret without word-wrap. C'est la vie. Experiment Configuration
SCons Environment Configuration
Master
Warnfix PR (MSVC_NOTFOUND_POLICY=Warning)
Differences between Master and Warnfix PR
For [1] and [2] (no compilers installed, force using msvc): the msvc warning is suppressed for the default tools. This can lead to build failures without a warning if the environment is used to build a program or library. For [3] and [4] (no compilers installed, using gcc): the msvc warning is suppressed for non-msvc tools. For [5] and [6] (no msvc installed, gcc installed): there was a change in behavior from the current master to the warnfix PR when SConstruct (8 test cases): |
I'd say all but #3664 are enhancements. And perhaps that one too, I've added a comment there with what I hope is a somewhat accurate recap and reasoned way forward? Of course allowing the user to specify the script arguments is a workaround to adding another MSVC_ envvar. (and adds more flexibility, but is more complicated than adding MSVC_TOOLSET (or other named var).. |
I added my 2cents in the other thread before seeing this. I would say you have captured the situation concisely. |
|
Comment 100.. Just because.. ;) Re So for both those cases, previously we'd get a no msvc warning, when none was explicitly requested? |
Yes. Previously (master), we would get a warning because the msvc compiler was "force set" as the default compiler even though there were no detected instances. When the default tools initialized, there was a warning that was triggered when
Wrong is probably too strong. The tool initialization always set a default compiler even if it does not exist (which I'm guessing probably makes the implementation simpler/straightforward as there is always a compiler configured). There is a fundamental problem with the default environment that when the tools are setup/initialized it is too early to know if and/or how they might actually be used if at all. Sort of a "closing the gate after the horse has left the barn problem". There aren't too many options here:
Someone will be annoyed either way. There are legitimate scons uses that don't require a c compiler, so suppressing the warning makes sense. |
The testing around this request revealed that a minor tweak to the implementation was required. Unfortunately, this means that I will have to revisit the previous experiments to make sure that they still work as expected which is fairly laborious. And so it goes... Setting the command-line environment via the msvc batch files and then using SCons via The issue is illustrated below using a VS2022 Preview command-line configuration that targets ARM64. The executable is built from the command-line prior to calling SCons to verify that the ARM64 executable is built (windows error indicating invalid architecture). Any executable that runs successfully under windows is built for the wrong architecture. When using Experiment Configuration
SCons Environment Configuration
Note Some of the M2 and M6 experiments issue the following warning: This warning is NOT included in the tables below. Master
Warnfix PR (MSVC_NOTFOUND_POLICY=Warning)
Table Footnotes
General Comments
|
|
Re: Sadly I've seen a LOT of projects building with SCons recommend this. |
It could be a problem whenever the command-line configured target is different from the default tools configured target. Going back to when 64-bit support was added in VS2005(?). The default msvc tools tend to default to the host architecture. Cross-compiling would be an example:
Arm/ARM64 just add more combinations. |
VS2022 is the only Preview installer that I have on hand. For the recent tests, I wanted to use a command-line version of msvc that is not detected by SCons. That made it easier when going through the paths to see what was going on and which version was configured. |
|
The os.environ() usage is being suggested even for supported MSVC. (As seen in the wild). And/or document the change needed to make that work (as you said |
It probably would be a good idea to gather examples of the existing options available and describe why one might choose one method over another: MSVC_USE_SCRIPT=None, MSVC_USE_SCRIPT/MSVC_USE_SCRIPT_ARGS, when MSVC_VERSION should be used, etc. As usual, there are a few different things going on that makes it more complicated. Using One implication of this is that if the version of msvc cannot be detected and there are no other msvc versions installed, if any other c compiler is installed and detected it will be used (see M1/E2 above). I believe that MSVC_VERSION is used in some of the tools for determining tool options/formats (possibly really old versions of msvc circa VS2005?). Even if there is only one version of msvc installed and that version is detected by scons, there could still be a discrepancy between the target arch in the environment and the target arch for the "default" msvc tool (as demonstrated above). Issues are more likely to arise when there are more than one version of msvc installed even if all versions of msvc are detected. As the gap between the environment version and the default tools version widens, the possibility of an undesirable outcome increases. For example, VS2019 and VS2013 are installed. If the command-line is configured for VS2013 and the default tools are VS2019, even if the target architectures are the same there could be issues with msvc runtime dependencies depending on what and how something is being built. For example, building a DLL could inadvertently be using ucrt instead of msvcr120. Keeping in mind that MSVC_USE_SCRIPT_ARGS was added not that long ago (and not made it into a formal release), I'm not sure why os.environ would be more attractive than using MSVC_USE_SCRIPT and MSVC_USE_SCRIPT_ARGS. Although an MSVC_VERSION would be necessary if the installed version of msvc was not detectable and the only version installed. Sample output illustrating potential issue
Below is the difference in constructed ENV paths and the cl.exe path for os.environ with and without Command-line msvc root (14.3 Preview): This is the path through the begin marker and the cl.exe found for M1/E3 ( This is the path through the begin marker and the cl.exe found for M2/E3 ( |
I am not wild about a documented feature that always issues a warning without a straightforward way to disable the warning (there may be such a way and I am unaware of said method). There are two places where an attempt to configure the msvc environment is made but checking for cl.exe is bypassed. The manner in which the existing code is written (annotated): could possibly be changed to: Which would "fall-through":
These would seem like a better use of a warning: replace a warning for "this feature was employed" to "could not find the compiler". Something similar could be done with the "use_script" case as well. My 2cents. Postscript: Probably need a "cache used" boolean variable so that the cl.exe check only issues the cache portion of the message if the cache was used in the relevant code path. |
|
If I don't misremember, the msvc stuff uses the SCons warnings framework, which can be configured on/off/etc. If that was the question. |
I am confident that you are correct. Documentation fragment (emphasis added):
If nothing else, it should probably be mentioned in the documentation that a warning will be issued and how to turn it off. |
|
@jcbrill @mwichmann - Any opposition to merging this PR as is, and then we can create another PR to polish up these final details? |
Fine with me. I wasn't planning on doing anything else for this PR after my last commit this morning unless there were any requests. |
3 participants
This PR is a proposed fix for #2813.
Requisite documentation is incomplete.Contributor Checklist:
CHANGES.txt(and read theREADME.rst)My TODO list:
Docs: Documentation for functions set_msvc_notfound_policy and get_msvc_notfound_policy (see below)?Also need to test:
Should and where would the set_msvc_notfound_policy and get_msvc_notfound_policy functions be documented?Added docstrings for set_msvc_notfound_policy and get_msvc_notfound_policy as discussed below.