-
-
Notifications
You must be signed in to change notification settings - Fork 325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
msvc initialization can blow up if nonexistant or not installed TARGET_ARCH requested #4312
Comments
@jcbrill I can make the test pass by just wrapping the block of code in lines 191-203 in a |
@mwichmann I'll look into it. At first glance, I'm a little concerned about why it reports that no versions of the MSVC compiler were found:
|
@mwichmann If you have it running "stand-alone", is there any chance you could enable the MSCommon debugging (i.e., |
Was just typing that I hadn't captured the debug.... will do that |
debug log added |
Meanwhile, it's certainly finding a compiler - 14.3 (I can add the cache file too if useful)... there was no arm or arm64 support in 11.0, but I assume we're not smart enough to know that much? |
Does the toolset |
No... in the directory a bit up, |
Still not sure what is happening. The log for 14.3 reports:
It appears like it is only checking 14.3 with a given I'm a little rusty evidently... EDIT: changed 14.2 to 14.3. |
@mwichmann The root problem is with the construction of the installed VCS cache list (i.e., The installed VCS list is cached (i.e., computed once). However, there are indirect dependencies on values from the environment. Most notably, the TARGET_ARCH. In some of the tests, when the installed VCS cache list is constructed, the TARGET_ARCH (e.g., The quick fix below uses an undefined TARGET_ARCH during the initial construction and then restores the original value prior to exit. This should be enough to accurately detect all installations with at least one usable HOST/TARGET combination.
This is a quick fix for the immediate problem: The Still not exactly sure how/why the original symptom is manifested yet. It seems to have disappeared with the quick fix. |
When testing on my development box which happens to have MSVC 11.0 Express installed, the arm batch file appears to succeed. However, the debug log for the batch file invocation indicates usage errors but no exception is raised because cl.exe is found on the path. I believe it to be the correct cl.exe as well. Unfortunately, when reviewing the relevant code, there is a bug involving the host platform and likely just express versions due to an error of omission of an additional check for existence. I need to test that an additional check does not break anything that is currently working as expected. |
@mwichmann updated branch available at: The implementation now passes the msvc/msvs tests (79 tests, 5 skipped) on my development box and in a VMWare virtual machine with a minimal VS2022 (x86 and amd64 for latest 14.3 toolset) as described above (79 tests, 15 skipped). Changes:
|
I don't know if it's relevant to the issue under investigation here (and sorry haven't caught up with the above few comments), but came across these two errors in a pylint report:
|
No worries. _Data.msvc_tools is initialized to None. It is later assigned a set(). In between there is code in functions that tests for membership. I'm guessing that might be why pylint is complaining. I'll take at look at the call sequence. I believe that the issue under investigation is the result of an internal state being constructed improperly (i.e., the installed vcs cache) which causes other issues. This morning I added the requisite configuration data structures for ARM64 hosts. |
Yes, it has a LOT of complaints on the SCons codebase where it can't be 100% sure that something that starts out with a sentinel |
Ah. yeah, that's a pain when tests start failing, even when you made things better. I did try running that test with no caching, FWIW, which didn't seem to have an impact. |
According to git blame, the installed vcs cache was added 13 years ago. Back then, there weren't too many options for targets. The code used to construct the installed vcs list indirectly evaluates TARGET_ARCH. This is BAD for a cached value as the value of For the tests that fail, the installed vcs list is using I'm not sure why it is failing now unless there was some change that might have affected the order in which environments are initialized (i.e., default vs user). For the initial construction |
Unrelated: the final check to see if This could not possibly be a good idea. I think in vc.py is should be SCons\Tool\MSCommon.py (function msvc_setup_env, lines 1280-1288):
find_program_path fragment:
SCons.Util.WhereIs fragment:
|
@mwichmann It is certainly a GOOD THING to make sure the list has at least one entry. I added the test on my end along with a debug message if the list is empty. It was an error of omission and hubris to assume the list would be populated upon first use. I've given myself a reprimand and a time-out. Given that the code incorrectly thought there were no installed VCs, I still don't understand why this issue did not arise in the previous testing concerning no VCs installed. With the detection corrected, the tests pass without the additional check. However, better with the additional test than without it. Still stress testing the detection fix and the ARM64 host support... |
Currently testing candidate: https://github.com/jcbrill/scons/tree/jbrill-gh4312-fixplus Previous branch deleted. |
@mwichmann When you have nothing better to do, could you take a peek at #4312 (comment) above? Another set of eyes would be useful as I have convinced myself that the warning message could be suppressed based on finding (any) cl.exe on the system path (i.e., os.environ) when no cl.exe was found on the detected environment path (i.e., env). Probably unlikely, but without a valid cl.exe in the environment, possibly destined for failure downstream during the build. As always, I could be wrong. |
Not sure quite what the question is... there are a few tools that directly use both |
Why does it matter if cl.exe was found on the system path when it is missing from the constructed environment path? My guess is that should a user have a cl.exe on the system path and there is a problem with the constructed environment, the build could fail without the warning which in that case seems to be useful/important diagnostic information. |
assuming someone actually does something with that information - which I think, in general, we don't. |
There is no guarantee that the tool found in the system environment is the same tool that was requested in scons. The environment may be configured for one version of msvc (either via command-line batch file or permanently registered environment variables) while the scons msvc version request is different. |
Indeed, that's a good point. So to me it seems like the behavior of |
Do you mean in find_program_path()? |
See #4312 (comment) above for original question and code fragments. The final check to issue a warning if the compiler is not present in In this context, Why should the warning message should be suppressed if a cl.exe is not in the constructed environment but happens to found on the system path? The cl.exe on the system path may have no relation to the msvc version requested. Am I missing something? |
Gotcha. |
I assume there was a reason why this system evolved the way it did, but I don't know it, as a "newcomer". Even |
SCons.Util.WhereIs will only use os.environ['PATH'] if the path argument is None. env.WhereIs will pass
I don't understand. The scons environment path should be checked for cl.exe. The system path (os.environ) should not be checked for cl.exe. Three code paths lead to the check: MSVC_USE_SCRIPT (user-defined script) , MSVC_USE_SETTINGS (user-defined dictionary), and MSVC_USE_SCRIPT (MSVC_VERSION request). The only case that does an early exit is when MSVC_USE_SCRIPT is set to False to bypass detection. All three cases require a cl.exe instance on the environment path or a downstream compile/link command likely will likely fail. In particular, MSVC_USE_SETTINGS doesn't go through any other of the detection code so the check is relatively important.
In this context, scons did not find the tool requested. I'm not sure finding a cl.exe on the system path is useful information as the check is only performed for the three cases listed above and not when a user is actually bypassing detection via MSVC_USE_SCRIPT. For example, the system environment could be configured for the 2008 x86 cl.exe. The scons build request could be for 2022 arm64. I'm not sure reporting tool found on the system path is necessarily reliable and/or useful. |
Other tools use these functions, and for those, configuration is pretty much as simple as finding the tool. |
Well, one could argue either way: that a message noting that a desired executable was found, but only in a path that won't be used, has some benefit - or that it's completely useless. What's presumably not useful is to proceed as if all were well if it is found, but not in the execution environment, because then it won't work. |
+1 |
Sigh. Apologies for hijacking. To wade even deeper into the weeds, there is yet another possible state to consider: the requested tool was not found but there is a tool already in the users IMHO, there needs to be a warning if the requested tool was not found regardless of whether a tool is found in the An additional warning should probably be issued if any other tool is found in the environment as there is no easy way to check if the found tool is "the same" as the requested tool. Bug-riddled pseudo-code to illustrate the possible states:
|
The scenario is: on a lightly provisioned (but working) Windows instance with VS2022,
test/MSVC/TARGET_ARCH.py
blew up scons with anIndexError
indexing into an empty list. In fact, the third and fourth tests in that file both do this, the specific lines are these:It takes a little work to generate a backtrace (I ended up pulling it into a standalone SConstruct and running outside the test framework), but the output including trace looks like this, trimmed down. I'll attach the full trace also, but I don't think the rest of it is interesting:
Adding a little check shows in this circumstance
_Data.default_tools_re_list
is empty, seems like a guard against this case is needed.msvciniterr.txt
The text was updated successfully, but these errors were encountered: