MSVC: adjust the compiler search and warning message when configuring the environment. #4432
Conversation
…setting up the msvc environment. Changes: * The search for the msvc compiler executable (cl.exe) no longer inspects the OS system path in certain situations when setting up the msvc environment. * When the msvc compiler executable is not found during setup of the msvc environment, the warning message issued takes into account whether or not a possibly erroneous compiler executable was already present in the scons environment path.
| warn_msg = f"Could not find MSVC compiler 'cl'. {propose}" | ||
| warn_msg += " It may need to be installed separately with Visual Studio." | ||
| if found_cl_envpath: | ||
| warn_msg += " A 'cl' was found on the scons ENV path which may be erroneous." |
There was a problem hiding this comment.
if the env['ENV']['PATH'] contains path to the requested and also otherwise found cl.exe, will this message get issued?
If the user specifically sets all the env['ENV'][*]'s set by msvc initialization will this get issued even if that is correct?
There was a problem hiding this comment.
if the env['ENV']['PATH'] contains path to the requested and also otherwise found cl.exe, will this message get issued?
No.
if not found_cl_path: <--- ONLY IF CL.EXE NOT FOUND IN SETUP DICT
warn_msg = "Could not find requested MSVC compiler 'cl'."
if CONFIG_CACHE:
warn_msg += f" SCONS_CACHE_MSVC_CONFIG caching enabled, remove cache file {CONFIG_CACHE} if out of date."
else:
warn_msg += " It may need to be installed separately with Visual Studio."
if found_cl_envpath:
warn_msg += " A 'cl' was found on the scons ENV path which may be erroneous."
debug(warn_msg)
SCons.Warnings.warn(SCons.Warnings.VisualCMissingWarning, warn_msg)
If the user specifically sets all the env['ENV'][*]'s set by msvc initialization will this get issued even if that is correct?
I'm not exactly sure what you're asking. The paths that come through this code are a request for the setup dictionary from MSVC_USE_SCRIPT (user script specification), MSVS_USE_SETTINGS (user dictionary specification), and MSVC autodetection.
Bypassing detection exits before this point:
else:
debug('MSVC_USE_SCRIPT set to False')
warn_msg = "MSVC_USE_SCRIPT set to False, assuming environment " \
"set correctly."
SCons.Warnings.warn(SCons.Warnings.VisualCMissingWarning, warn_msg)
return None
The message is invoked when a requested setup dictionary does not contain a compiler executable. The possibly erroneous fragment is indicating the there is a compiler in the user's path which may not be what was expected as the requested setup dictionary failed to include a compiler executable.
There was a problem hiding this comment.
OK. that's not clear from the CHANGES.txt (and release.txt) blurbs.
So these changes only apply when MSVC_USE_SETTINGS is set?
There was a problem hiding this comment.
MSVC_USE_SCRIPT => user-specified script (intent to get setup config from provided script output)
MSVC_USE_SETTINGS => user-specified dictionary (intent to get setup config from provided dictionary)
MSVC_VERSION => auto-detection (intent to get setup config from selected script output)
BYPASS => early-exit
Basically, the user requested a "valid" setup configuration. Failing to find a compiler executable in the setup dictionary indicates that the request failed.
The augmented message is necessary because if/when there is a compiler executable already in the user's environment (which may or may not be the desired compiler), the build may continue and could possibly complete successfully with the "wrong" compiler.
There is no guarantee that the configured system environment is the same as the requested setup environment.
There was a problem hiding this comment.
Perhaps the terminology is confusing: at the code level the "setup config" is a dictionary. All three methods that do not exit early return a setup configuration dictionary.
There was a problem hiding this comment.
So please be explicit about such in the CHANGES and RELEASE. While you may immediately know the correlation between the text and the variables above, most people won't.
There was a problem hiding this comment.
The correlation between the text and the variables above did not change.
With the exception of the bug fix, all that is different is that possibly another sentence is included.
-
The bug fix (inspecting the os path via find_program_path which was removed).
CHANGES.TXT:
The compiler detection no longer considers the host os environment path.RELEASE.TXT:
The search for the msvc compiler executable (cl.exe) no longer inspects the OS system path in certain situations when setting up the msvc environment. -
If necessary, the new third sentence.
CHANGES.TXT:
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. Different warnings are produced 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)RELEASE.TXT:
When the msvc compiler executable is not found during setup of the msvc environment, the warning message issued takes into account whether or not a possibly erroneous compiler executable was already present in the scons environment path.
There was a problem hiding this comment.
Old message:
Could not find MSVC compiler 'cl'. It may need to be installed separately with Visual Studio.
New message (no cl on env path) with word requested:
Could not find requested MSVC compiler 'cl'. It may need to be installed separately with Visual Studio.
New message (cl on env path) with word requested and more information:
Could not find requested MSVC compiler 'cl'. It may need to be installed separately with Visual Studio. A 'cl' was found on the scons ENV path which may be erroneous.
CHANGES.txt
Outdated
| compiler detection no longer considers the host os 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. Different warnings are produced when the msvc compiler is not detected in the |
There was a problem hiding this comment.
Probably worth indicating the old message and the new message here.
I've searched CHANGES.txt before for message text to figure out which version changed it.
There was a problem hiding this comment.
The only change to the old message was inserting the word "requested".
new: "Could not find requested MSVC compiler 'cl'."
old: "Could not find MSVC compiler 'cl'."
Other than the word requested, the first two sentences are the same.
The third sentence is new (i.e., it did not appear before).
For all intents and purposes, the old message is augmented in certain circumstances (i.e., the compiler is not found in the requested dictionary and exists already in the environment path).
Is it really necessary?
Manually resolved conflicts: * CHANGES.txt
| @@ -101,6 +105,8 @@ FIXES | |||
| - MSVC: Erroneous construction of the installed msvc list (as described above) caused an | |||
| index error in the msvc support code. An explicit check was added to prevent indexing | |||
| into an empty list. Fixes #4312. | |||
| - MSVC: The search for the msvc compiler executable (cl.exe) no longer inspects the | |||
| OS system path in certain situations when setting up the msvc environment. | |||
There was a problem hiding this comment.
I don't see code change which would have this effect in this PR?
Or is this code having this effect?
if not seen_path and k == 'PATH':
seen_path = True
There was a problem hiding this comment.
Searching for the compiler executable using the method find_program_path was removed:
- # final check to issue a warning if the compiler is not present
- if not find_program_path(env, 'cl'):
- debug("did not find %s", _CL_EXE_NAME)
The find_program_path searches the OS path for the program when the program is not found in the scons environment which is undesirable in this case. A compiler executable path from the OS PATH should not be used in this context.
This was a bug. When looking at the existing code, one would have to know what find_program_path is actually doing internally to realize there is a problem. The find_program_path source is shown below.
A similar issue arises when the detected dictionary keys are added to the env and then the environment path is searched. Was the found compiler executable in the detection dictionary, already in the user's environment, or both?
To solve these particular issues, the detection dictionary and the user's environment are both searched immediately prior to adding the detected path elements to the environment:
seen_path = False
for k, v in d.items():
if not seen_path and k == 'PATH':
seen_path = True
found_cl_path = SCons.Util.WhereIs('cl', v) # <-- DETECTED PATH
found_cl_envpath = SCons.Util.WhereIs('cl', env['ENV'].get(k, [])) # <-- env['ENV'] PATH
env.PrependENVPath(k, v, delete_existing=True) # <-- ADD DETECTED TO env['ENV']
debug("env['ENV']['%s'] = %s", k, env['ENV'][k])
The code fragment Is searching the detection dictionary PATH element for a compiler executable. If no compiler executable is found in the detection dictionary this means that the user's request has failed to produce a valid compiler executable and a warning will be issued.
The code fragment is also searching the users env['ENV']['PATH'] for a compiler executable and may append the "erroneous compiler" sentence to the end of the warning.
When there is no compiler present in the requested version detection dictionary and there is a compiler already present in the user's environment, regardless of the warning issued, the build may succeed possibly using a compiler executable that is not the same version as the request.
If if still doesn't make sense, let me know, and I'll try again.
find_program_path source:
def find_program_path(env, key_program, default_paths=None, add_path: bool=False) -> Optional[str]:
"""
Find the location of a tool using various means.
Mainly for windows where tools aren't all installed in /usr/bin, etc.
Args:
env: Current Construction Environment.
key_program: Tool to locate.
default_paths: List of additional paths this tool might be found in.
add_path: If true, add path found if it was from *default_paths*.
"""
# First search in the SCons path
path = env.WhereIs(key_program)
if path:
return path
# Then in the OS path
path = SCons.Util.WhereIs(key_program)
if path:
if add_path:
env.AppendENVPath('PATH', os.path.dirname(path))
return path
# Finally, add the defaults and check again.
if default_paths is None:
return path
save_path = env['ENV']['PATH']
for p in default_paths:
env.AppendENVPath('PATH', p)
path = env.WhereIs(key_program)
# By default, do not change ['ENV']['PATH'] permananetly
# leave that to the caller, unless add_path is true.
env['ENV']['PATH'] = save_path
if path and add_path:
env.AppendENVPath('PATH', os.path.dirname(path))
return path
There was a problem hiding this comment.
So using env['ENV']['PATH'] to find cl is only valid if MSVC_USE_SCRIPT is set to false? Otherwise you'll get the warning?
(As a side note, the variable named "d" isn't descriptive and keeps hurting my eyes.. not for this PR, but if you could name it something which indicates it's purpose at some point my eyes would thank you! ;)
There was a problem hiding this comment.
So using env['ENV']['PATH'] to find cl is only valid if MSVC_USE_SCRIPT is set to false? Otherwise you'll get the warning?
Not quite.
When MSVC_USE_SCRIPT is set to false this code is not executed as there is an early exit so there is never a check for a compiler. However, a different warning is always issued. It would probably be easy enough to suppress the warning though if a compiler was found in env['ENV']['PATH'].
Early exit:
...
else:
debug('MSVC_USE_SCRIPT set to False')
warn_msg = "MSVC_USE_SCRIPT set to False, assuming environment " \
"set correctly."
SCons.Warnings.warn(SCons.Warnings.VisualCMissingWarning, warn_msg)
return None
found_cl_path = None
found_cl_envpath = None
...
(As a side note, the variable named "d" isn't descriptive and keeps hurting my eyes.. not for this PR, but if you could name it something which indicates it's purpose at some point my eyes would thank you! ;)
I completely agree.
That was not my doing. It has been that way since 2.0.0.final.0:
use_script = env.get('MSVC_USE_SCRIPT', True)
if SCons.Util.is_String(use_script):
debug('vc.py:msvc_setup_env() use_script 1 %s\n' % repr(use_script))
d = script_env(use_script)
elif use_script:
d = msvc_find_valid_batch_script(env,version)
debug('vc.py:msvc_setup_env() use_script 2 %s\n' % d)
if not d:
return d
else:
debug('MSVC_USE_SCRIPT set to False')
warn_msg = "MSVC_USE_SCRIPT set to False, assuming environment " \
"set correctly."
SCons.Warnings.warn(SCons.Warnings.VisualCMissingWarning, warn_msg)
return None
for k, v in d.items():
debug('vc.py:msvc_setup_env() env:%s -> %s'%(k,v))
env.PrependENVPath(k, v, delete_existing=True)
There was a problem hiding this comment.
And I would be happy to change the name :)
This PR closes existing loopholes when searching for a msvc compiler executable (cl.exe) in the configured msvc dictionary and the target scons environment.
Changes:
The second in a sequence of smaller PRs from #4409.
Contributor Checklist:
CHANGES.txt(and read theREADME.rst)