gemini-code-assist[bot] commented on code in PR #272:
URL: https://github.com/apache/tvm-ffi/pull/272#discussion_r2534308512
##########
python/tvm_ffi/_optional_torch_c_dlpack.py:
##########
@@ -97,11 +101,18 @@ def load_torch_c_dlpack_extension() -> Any: # noqa:
PLR0912
args.append("--build-with-cuda")
elif device == "rocm":
args.append("--build-with-rocm")
- subprocess.run(
- args,
- check=True,
- )
- assert lib_path.exists(), "Failed to build torch c dlpack addon."
+
+ # use capture_output to reduce noise when building the torch c
dlpack addon
+ result = subprocess.run(args, check=False, capture_output=True)
+ if result.returncode != 0:
+ msg = [f"Build failed with status {result.returncode}\n"]
Review Comment:
The error message construction for a failed build includes a trailing
newline in the first part of the message. This is inconsistent with similar
error handling in `_build_optional_torch_c_dlpack.py` and results in an extra
blank line in the output. For consistency and cleaner error messages, I suggest
removing the `\n`.
```suggestion
msg = [f"Build failed with status {result.returncode}"]
```
##########
python/tvm_ffi/utils/_build_optional_torch_c_dlpack.py:
##########
@@ -625,43 +614,91 @@ def _generate_ninja_build(
str(Path(path).resolve()) for path in extra_include_paths
]
+ # append include paths
+ for path in include_paths:
+ cflags.append(f"-I{path}")
Review Comment:
The way include paths are added to `cflags` may not correctly handle paths
with spaces. The expression `f"-I{path}"` creates a single string like
`"-I/path with/spaces"`. When passed to the compiler as a single argument
without a shell, it will likely be misinterpreted. To handle paths with spaces
correctly and robustly, it's safer to pass `-I` and the path as separate
arguments in the command list.
```suggestion
for path in include_paths:
cflags.extend(["-I", str(path)])
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
