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

Windows: Fix arch detection via VCTOOLSINSTALLDIR if not first in PATH #93589

Merged

Conversation

mhilbrunner
Copy link
Member

@mhilbrunner mhilbrunner commented Jun 25, 2024

Trying to invoke scons on Windows with VCTOOLSINSTALLDIR set (as is the case in the default VS / GDK command prompts) resulted in the following error for me:

KeyError: 'bin':
  ...
  File "C:\git\godot\.\platform\windows\detect.py", line 144:
    return msvc_target_aliases[first_path_arch]

Turns out we have this piece of code in the Windows platform detect.py, trying to determine the arch:

# VS 2017 and newer.
if os.getenv("VCTOOLSINSTALLDIR"):
host_path_index = os.getenv("PATH").upper().find(os.getenv("VCTOOLSINSTALLDIR").upper() + "BIN\\HOST")
if host_path_index > -1:
first_path_arch = os.getenv("PATH").split(";")[0].rsplit("\\", 1)[-1].lower()
return msvc_target_aliases[first_path_arch]

Here is what this tries to do:

  • It checks if VCTOOLSINSTALLDIR is set
    (which for me has a value like C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\).
  • if so, it appends BIN\HOST to that value and searches for it in PATH and if found, saves the starting index of that in PATH.
  • It should find a value like C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\bin\Hostx64\x64 in PATH; it now tries to extract the arch bit in ...\Hostx64\... (x64).

However, there are 2 errors here:

  1. Line 145 does not actually use the found index in the PATH. os.getenv("PATH").split(";")[0] just gets the first element of PATH, then .rsplit("\\", 1)[-1] cuts parts of. For me, this resulted in a value of bin instead of x64, as VCTOOLSINSTALLDIR was not the first in PATH. Changing it to os.getenv("PATH")[host_path_index:]... to actually start at the found index fixes this and resolves the error.
  2. There is a missing guard to check whether the found arch actually exists in the dict before returning it. All other similar places in the file have such guards. So, I added one here with if first_path_arch in msvc_target_aliases.keys(), which would at least have prevented this error.

This seems to have been introduced in #64921.

@akien-mga akien-mga changed the title Windows: Fix arch detection via VCTOOLSINSTALLDIR if not first in PATH Windows: Fix arch detection via VCTOOLSINSTALLDIR if not first in PATH Jun 28, 2024
@akien-mga akien-mga merged commit 3cefe89 into godotengine:master Jun 28, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@mhilbrunner mhilbrunner deleted the fix-windows-vs-arch-detection branch June 29, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants