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

Detect whether VC++ compiler can be found in configure.py #2086

Merged
merged 5 commits into from
Oct 23, 2022

Conversation

randomascii
Copy link
Contributor

First time contributors to ninja on Windows will run "python configure.py --bootstrap" and will probably be met with this cryptic error:

WindowsError: [Error 2] The system cannot find the file specified

It may take some debugging to realize that the file being looked for is cl.exe, the VC++ compiler. This change adds an explicit check for cl.exe being in the path and raise an exception so that this message is displayed:

Exception: cl.exe not found. Run again from the Developer Command Prompt for VS

This makes no difference to ninja builds, but makes the getting-started experience for developers wanting to contribute to ninja go more smoothly.

randomascii and others added 3 commits July 26, 2021 16:19
The first experience for most developers who start working on ninja is
this cryptic error message:

bootstrapping ninja...
Traceback (most recent call last):
  File "configure.py", line 329, in <module>
    if platform.msvc_needs_fs():
  File "configure.py", line 89, in msvc_needs_fs
    stderr=subprocess.PIPE)
  File "python\bin\lib\subprocess.py", line 394, in __init__
    errread, errwrite)
  File "python\bin\lib\subprocess.py", line 644, in _execute_child
    startupinfo)
WindowsError: [Error 2] The system cannot find the file specified

This message happens when bootstrap.py first tries to invoke cl.exe and
it cannot be found. This change looks for cl.exe and warns if it is not
in the user's path, leading to this friendlier message:

bootstrapping ninja...
Traceback (most recent call last):
  File "configure.py", line 317, in <module>
    raise Exception('cl.exe not found. Run again from the Developer Command Prompt for VS')
Exception: cl.exe not found. Run again from the Developer Command Prompt for VS
Copy link
Contributor

@digit-google digit-google left a comment

Choose a reason for hiding this comment

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

Very nice patch :)

configure.py Outdated
@@ -305,7 +305,16 @@ def binary(name):
else:
n.variable('ar', configure_env.get('AR', 'ar'))

def SearchPath(exe_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: rename this to search_windows_path() since this doesn't handle anything else and uses the same naming convention as the rest of this file.

configure.py Outdated
@@ -305,7 +305,16 @@ def binary(name):
else:
n.variable('ar', configure_env.get('AR', 'ar'))

def SearchPath(exe_name):
"""Find an executable (.exe, .bat, whatever) in the system path."""
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The comment and naming used here suggest that this behaves like the Win32 SearchPath() function and will probe for <exe_name>.bat, <exe_name>,exe or <exe_name>.cmd, which this function does not. Using "find a file in the current Windows path" would be more accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point about the name and the comment. The function is OS-generic so I renamed it to search_system_path - it could be used on any OS, it just currently isn't. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the name change, I would just mention that the function is not OS-generic though. I.e. on Unix you would want to use ':', not ';' for the split, and you would need to handle empty items, which translate to "search the current path" (and frequent source of conflict and security issues, happens all the time with LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/some/new/path, when LD_LIBRARY_PATH was empty, and now your library search path will always start in the current directory, boom).

Please do not handle this here, but that's why search_windows_path() was a simpler name :-) And path searching is always trickier than expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points. I could rename to search_windows_path, or leave it as-is. Fixing the path separator is easy enough (os.pathsep instead of ';', perhaps a worthwhile change anyway) but it sounds like it still wouldn't be truly generic.

@jhasse jhasse merged commit 023d8a8 into ninja-build:master Oct 23, 2022
@randomascii randomascii deleted the compiler_detect branch October 24, 2022 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants