Fix vswhere.exe discovery for non-default Visual Studio installations#1248
Fix vswhere.exe discovery for non-default Visual Studio installations#1248Adityakk9031 wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a PATH-first vswhere discovery helper used by MSVC detection with robust try/except and utf-8-with-replace decoding, updates MSVC-not-found messaging to a multiline instructional error, and adds an Unreleased changelog entry documenting the vswhere discovery fix. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor BuildScript as "build_dll.py"
participant PATH as "PATH"
participant Vswhere as "vswhere.exe"
participant ProgramFiles as "ProgramFiles(x86)/ProgramFiles"
participant VSEnv as "vcvars64.bat / Env"
BuildScript->>PATH: check for vswhere.exe
alt found in PATH
PATH-->>BuildScript: path to vswhere.exe (rgba(0,128,0,0.5))
else not found
BuildScript->>ProgramFiles: check common Program Files locations
ProgramFiles-->>BuildScript: path or not found (rgba(255,165,0,0.5))
end
BuildScript->>Vswhere: run vswhere -latest -property installationPath
Vswhere-->>BuildScript: installationPath or error (rgba(0,128,255,0.5))
alt installationPath present
BuildScript->>VSEnv: run vcvars64.bat to load env
VSEnv-->>BuildScript: env vars or error (rgba(0,0,128,0.5))
BuildScript-->>BuildScript: verify MSVC version and return config
else error / not found
BuildScript-->>BuildScript: log verbose warning and return ""
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR improves Visual Studio/MSVC discovery reliability for non-standard installations. The changes add a new Key improvements:
All previously identified issues have been addressed:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[find_host_compiler called] --> B{Windows?}
B -->|No| C[Return CXX or g++]
B -->|Yes| D{VS env configured?}
D -->|Yes + cl.exe found| E[Return cl.exe path]
D -->|No or cl.exe missing| F[Call _find_vswhere]
F --> G{Check PATH}
G -->|Found| H[Return vswhere path]
G -->|Not found| I{Check ProgramFiles x86}
I -->|Found| H
I -->|Not found| J{Check ProgramFiles}
J -->|Found| H
J -->|Not found| K[Return empty string]
H --> L[Run vswhere -latest]
L -->|Success| M[Get VS install path]
L -->|Failure| K
M --> N[Run vcvars64.bat]
N -->|Success| O[Set environment vars]
N -->|Failure| K
O --> P{Validate cl.exe}
P -->|Found| Q{Check VCToolsVersion}
P -->|Missing| K
Q -->|Valid format| R{Version >= 14.29?}
Q -->|Invalid| K
R -->|Yes| E
R -->|No| K
Last reviewed commit: d30dc55 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
warp/_src/build_dll.py
Outdated
| if not vswhere_path: | ||
| return "" # Signal to caller that vswhere.exe not found | ||
|
|
||
| vs_path = run_cmd(f'"{vswhere_path}" -latest -property installationPath').decode().rstrip() |
There was a problem hiding this comment.
Missing error handling if vswhere command fails. run_cmd() raises CalledProcessError on failure (see line 68), which would crash here instead of gracefully returning empty string
| vs_path = run_cmd(f'"{vswhere_path}" -latest -property installationPath').decode().rstrip() | |
| try: | |
| vs_path = run_cmd(f'"{vswhere_path}" -latest -property installationPath').decode().rstrip() | |
| except subprocess.CalledProcessError: | |
| return "" # Signal to caller that vswhere command failed |
Additional Comments (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
warp/_src/build_dll.py (1)
110-114: Nit: use"vswhere.exe"for consistency with othershutil.whichcalls in this file.All other
shutil.whichcalls in this file include the explicit.exesuffix (e.g.,shutil.which("cl.exe")on Lines 149 and 176). On Windows, both forms resolve identically viaPATHEXT, but the suffix makes the search target unambiguous.✏️ Suggested change
- path_result = shutil.which("vswhere") + path_result = shutil.which("vswhere.exe")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/build_dll.py` around lines 110 - 114, Update the shutil.which call to search for "vswhere.exe" instead of "vswhere" to match the explicit .exe convention used elsewhere (e.g., the other shutil.which("cl.exe") calls); locate the call that assigns path_result = shutil.which("vswhere") and change the search string to "vswhere.exe" while keeping the existing verbose_cmd print and return behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 116-118: Remove the implementation-detail sentence "The build
system now checks `PATH` before falling back to default locations" from the
CHANGELOG entry about vswhere.exe discovery (leave the user-facing fix sentence
about discovery failing for non-default or package-manager installs), or
consolidate it into the first sentence so the entry only states the user-visible
change and not internal behavior.
---
Nitpick comments:
In `@warp/_src/build_dll.py`:
- Around line 110-114: Update the shutil.which call to search for "vswhere.exe"
instead of "vswhere" to match the explicit .exe convention used elsewhere (e.g.,
the other shutil.which("cl.exe") calls); locate the call that assigns
path_result = shutil.which("vswhere") and change the search string to
"vswhere.exe" while keeping the existing verbose_cmd print and return behavior
intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@warp/_src/build_dll.py`:
- Around line 163-175: The code around vs_path/vsvars_path uses
run_cmd(...).decode() without handling empty output or decode errors; update the
block using run_cmd to (1) decode with a safe strategy (e.g.,
output_bytes.decode("utf-8", errors="replace") or catch UnicodeDecodeError) when
calling run_cmd for vs_path and for output, and (2) explicitly check vs_path for
an empty string after decoding and return/log a clear diagnostic (instead of
silently returning "") so callers/users know VS wasn't found; reference the
vs_path variable, the vsvars_path (vcvars64.bat) construction, and the run_cmd
calls so you update both decode sites and add the empty-check for vs_path.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@warp/_src/build_dll.py`:
- Around line 163-166: The broad except that currently reads except
(subprocess.CalledProcessError, Exception) should be narrowed to explicitly
catch subprocess.CalledProcessError and UnicodeDecodeError (e.g., except
(subprocess.CalledProcessError, UnicodeDecodeError):) for both places handling
run_cmd output (the vs_path decode block and the later similar block), and, for
a longer-term fix, update run_cmd where it does e.output.decode() to use
errors="replace" so UnicodeDecodeError cannot escape (this lets callers safely
catch only subprocess.CalledProcessError later).
warp/_src/build_dll.py
Outdated
| .decode("utf-8", errors="replace") | ||
| .rstrip() | ||
| ) | ||
| except (subprocess.CalledProcessError, Exception): |
There was a problem hiding this comment.
Catching bare Exception is overly broad and may hide unexpected errors like KeyboardInterrupt or programming bugs. Consider catching only specific exceptions like subprocess.CalledProcessError, UnicodeDecodeError, and OSError
warp/_src/build_dll.py
Outdated
| output = run_cmd(f'"{vsvars_path}" && set').decode() | ||
| try: | ||
| output = run_cmd(f'"{vsvars_path}" && set').decode("utf-8", errors="replace") | ||
| except (subprocess.CalledProcessError, Exception): |
There was a problem hiding this comment.
Same broad exception handling issue - consider catching only specific exception types
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@warp/_src/build_dll.py`:
- Line 169: The current except clause except (subprocess.CalledProcessError,
Exception) in build_dll.py is a blind catch (BLE001) because Exception subsumes
CalledProcessError; change the handler to explicitly catch the intended
exception types (e.g., except subprocess.CalledProcessError as e: and, if
needed, except UnicodeDecodeError as e:) at both locations referenced, and
include the caught exception variable in the process/error handling. Also update
run_cmd (the function that does .decode()) to use .decode("utf-8",
errors="replace") as a long-term fix so callers can safely catch only
subprocess.CalledProcessError.
Additional Comments (1)
|
Additional Comments (1)
This is a pre-existing issue (the original code had the same pattern), but since this PR reworked this section and added the |
Closes NVIDIA#1235 Signed-off-by: Aditya kumar singh <143548997+Adityakk9031@users.noreply.github.com>
|
@shi-eric have a look |
|
@christophercrouzet please review it |
|
This looks good to me. I'll see if we can get it into the 12.1 release. |
shi-eric
requested review from
c0d1f1ed
and removed request for
christophercrouzet
|
/ok to test d30dc55 |
|
@Adityakk9031 Thanks for the MR. I don't have any strictly technical concerns with it, but it adds 67 lines of code so I just wanted to verify that it's worth the complexity. Could you detail the exact issue you encountered and why all of the changes in this MR are necessary to address it? |
2 participants
Description
This PR fixes an issue where
vswhere.exediscovery would silently fail for non-default Visual Studio installations or whenvswherewas installed via a standalone package manager (e.g., Chocolatey, Scoop, winget).Changes:
_find_vswhere()helper inwarp/_src/build_dll.pywith multi-strategy path discovery. It now searchesPATHbefore falling back to the standard VS Installer paths under%ProgramFiles(x86)%and%ProgramFiles%.find_host_compiler()with a call to the new helper.build_lib.pywhen MSVC cannot be found. The message now mentions actionable workarounds, such as using the--msvc-pathand--sdk-pathflags or ensuringvswhere.exeis in thePATH.CHANGELOG.mdunder theFixedsection.Summary by CodeRabbit
Bug Fixes
Improvements
Documentation