Skip to content
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.

Sign up for GitHub

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: adjust the compiler search and warning message when configuring the environment. #4432

Merged
merged 4 commits into from
Oct 29, 2023
Merged

MSVC: adjust the compiler search and warning message when configuring the environment. #4432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf5dc45
Adjust the final compiler executable search and warning message when …
jcbrill Oct 11, 2023
494d4d2
Rework msvc cl not found warning message order and construction.
jcbrill Oct 13, 2023
4a01e14
Merge branch 'master' into jbrill-msvc-clsearch
jcbrill Oct 15, 2023
fe0165d
Updates to CHANGES.txt and RELEASE.txt.
jcbrill Oct 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,24 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER
msvs. Moving any of these tools that used relative imports to the scons site tools
folder would fail on import (i.e., the relative import paths become invalid when
moved).
- The detection of the msvc compiler executable (cl.exe) has been modified:
* The host os environment path is no longer evaluated for the existence of the
msvc compiler executable when searching the detection dictionary.
* The 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., a msvc compiler executable already exists in the
user's ENV path prior to detection).
* The warning message issued when a msvc compiler executable is not found in the
detection dictionary was modified by adding the word "requested":
Old warning: "Could not find MSVC compiler 'cl'."
New warning: "Could not find requested MSVC compiler 'cl'.".
* An additonal sentence is appended to the warning message issued when an msvc
compiler executable is not found in the msvc detection dictionary and is found
in the user's ENV path prior to detection:
" A 'cl' was found on the scons ENV path which may be erroneous."

From Vitaly Cheptsov:
- Fix race condition in `Mkdir` which can happen when two `SConscript`
Expand Down
6 changes: 6 additions & 0 deletions RELEASE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ CHANGED/ENHANCED EXISTING FUNCTIONALITY
batch file exists but an individual msvc toolset may not support the host/target
architecture combination. For example, when using VS2022 on arm64, the arm64 native
tools are only installed for the 14.3x toolsets.
- MSVC: 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. See CHANGES.txt for details.
- Extend range of recognized Java versions to 20.
- Builder calls (like Program()) now accept pathlib objects in source lists.
- The Help() function now takes an additional keyword argument keep_local:
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I would be happy to change the name :)

- MSCommon: Test SConfTests.py would fail when mscommon debugging was enabled via the
MSVC_MSCOMMON_DEBUG environment variable. The mscommon logging filter class registered
with the python logging module was refactored to prevent test failure.
Expand Down
24 changes: 18 additions & 6 deletions SCons/Tool/MSCommon/vc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1515,18 +1515,30 @@ def msvc_setup_env(env):
SCons.Warnings.warn(SCons.Warnings.VisualCMissingWarning, warn_msg)
return None

found_cl_path = None
found_cl_envpath = None

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)
found_cl_envpath = SCons.Util.WhereIs('cl', env['ENV'].get(k, []))
env.PrependENVPath(k, v, delete_existing=True)
debug("env['ENV']['%s'] = %s", k, env['ENV'][k])

# 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)
debug("cl paths: d['PATH']=%s, ENV['PATH']=%s", repr(found_cl_path), repr(found_cl_envpath))

# final check to issue a warning if the requested compiler is not present
if not found_cl_path:
warn_msg = "Could not find requested MSVC compiler 'cl'."
if CONFIG_CACHE:
propose = f"SCONS_CACHE_MSVC_CONFIG caching enabled, remove cache file {CONFIG_CACHE} if out of date."
warn_msg += f" SCONS_CACHE_MSVC_CONFIG caching enabled, remove cache file {CONFIG_CACHE} if out of date."
else:
propose = "It may need to be installed separately with Visual Studio."
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."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. that's not clear from the CHANGES.txt (and release.txt) blurbs.
So these changes only apply when MSVC_USE_SETTINGS is set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

debug(warn_msg)
SCons.Warnings.warn(SCons.Warnings.VisualCMissingWarning, warn_msg)

def msvc_exists(env=None, version=None):
Expand Down
2 changes: 1 addition & 1 deletion test/MSVC/MSVC_USE_SETTINGS.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
""" % locals())

test.run(arguments="--warn=visual-c-missing .", status=0, stderr=None)
test.must_contain_all(test.stderr(), "Could not find MSVC compiler 'cl'")
test.must_contain_all(test.stderr(), "Could not find requested MSVC compiler 'cl'")

test.write('SConstruct', """
env = Environment(MSVC_USE_SETTINGS='dict or None')
Expand Down