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.

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: Fix unhandled MissingConfiguration exception. #4419

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

jcbrill
Copy link
Contributor

@jcbrill jcbrill commented Sep 22, 2023

Fix an issue with an unhandled MissingConfiguration exception due to an msvc registry query that returns a path that does not exist. Multiple invocation paths were not prepared to handle the MissingConfiguration exception. The MissingConfiguration exception type was removed.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

Fix an issue with an unhandled MissingConfiguration exception due to an msvc registry query that returns a path that does not exist.  Multiple invocation paths were not prepared to handle the MissingConfiguration exception.  The MissingConfiguration exception type was removed.
@jcbrill
Copy link
Contributor Author

jcbrill commented Sep 22, 2023

The [Scons-users] Build fails due to MissingConfiguration error mailing list issue was caused by a vc find_vc_dir method call in vs.py. The find_vc_dir function raised MissingConfiguration which was not handled by the vs.py invocation.

The MissingConfiguration exception was raised inside the loop evaluating registry queries for an msvc version. In general, it is better to ignore the missing msvc folder and continue because there may be additional registry key queries that might be successful.

The MissingConfiguration exception was removed in #4409 for this reason.

@bdbaddog
Copy link
Contributor

So now the bad configuration yields no errors or warnings unless you're in debug mode?

@jcbrill
Copy link
Contributor Author

jcbrill commented Sep 22, 2023

So now the bad configuration yields no errors or warnings unless you're in debug mode?

Yes.

The "main" paths simply caught the exception and ignored it anyway (one caught MissingConfiguration and another caught VisualCException). Unfortunately, not all paths handled the exception. It almost appears like MissingConfiguration was meant for internal use.

It is a bad configuration in the sense that the registry and file system don't agree. There is still not a usable msvc installation which was detected and should be ignored as it is in a detection loop. Although for 10.0 it doesn't matter.

10.0 folders and registry entries are also created by winsdk 7.1 rather than the visual studio installer (although the vc folder should have been present).

@jcbrill
Copy link
Contributor Author

jcbrill commented Sep 22, 2023

The "unprotected" code path was from get_installed_visual_studios in vs.py.

get_installed_vcs in vc.py catches and ignores VisualCException.

msvc_find_valid_batch_script in vc.py used to catch and ignore MissingConfiguration.

There are three (fairly recent) toolset query methods that are "unprotected" as well.

Summary:

  • The two "main" vc.py invocations caught and ignored the exception.
  • The one vs.py invocation was not protected.
  • The three "toolset" vc.py invocations are not protected.

@mwichmann mwichmann added MSVC Microsoft Visual C++ Support bug labels Sep 22, 2023
@bdbaddog
Copy link
Contributor

So this PR is about removing partially or poorly implemented detection of corrupted or incorrectly configured msvc install.

That sounds reasonable, but should we add an issue to handle this properly so we don't lose the idea?

@bdbaddog bdbaddog merged commit dbd616a into SCons:master Sep 22, 2023
4 of 5 checks passed
@jcbrill
Copy link
Contributor Author

jcbrill commented Sep 22, 2023

So this PR is about removing partially or poorly implemented detection of corrupted or incorrectly configured msvc install.

Yes.

The intent is to ignore the corrupted or incorrectly configured msvc install. This was achieved by raising an exception and catching it somewhere higher up in the call stack.

The bug is that the exception was never caught in vs.py.

Looking solely at the typical vc.py usage, only the debug log would indicate the registry/filesystem inconsistency. It does not appear like this exception was intended to terminate the build but rather as a way to move up the call stack via catching the exception.

Due to the code evolution, ignoring the msvc root should continue in the detection loop for the version. A msvc root not found is handled correctly.

That sounds reasonable, but should we add an issue to handle this properly so we don't lose the idea?

Up to you. It really is an implementation detail. I suspect that it goes back a long way. For tracking, it wouldn't hurt though.

@jcbrill jcbrill deleted the jbrill-msvc-missingconfig branch September 22, 2023 18:18
@mwichmann mwichmann added this to the 4.6 milestone Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug MSVC Microsoft Visual C++ Support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants