fix_libc++_cannot_find_stdmodule #7074
Conversation
Summary of ChangesHello @SnowinterCat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where Xmake failed to locate Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue with locating libc++ standard modules by introducing a helper function parse_link_files to resolve symbolic links. However, the implementation of parse_link_files is incomplete as it doesn't handle chained symbolic links, a common scenario on Linux systems. Additionally, the logic for resolving module paths has been changed to be relative to the clang executable's directory, which contradicts LLVM's documentation and is likely incorrect. I have provided suggestions to address both of these issues to ensure robust and correct behavior.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the logic for locating libc++ standard modules by adding more robust path resolution, including handling for symbolic links and various relative path scenarios. The changes are logical and address the described problem. I've provided a few suggestions to prevent potential runtime errors from nil values and to improve code style by scoping helper functions locally.
| wprint("libc++.modules.json is invalid! path: %s", path.normalize(modules_json_path)) | ||
| return | ||
| end | ||
| local std_module_directory = path.directory(modules_json.modules[1]["source-path"]) |
There was a problem hiding this comment.
The code does not check if modules_json.modules[1]["source-path"] exists before using it. If it's nil, path.directory(nil) will be called, which becomes path.directory("nil") and returns . This would lead to incorrect path resolution. It's safer to check for the existence of "source-path" and print a warning if it's missing.
local source_path = modules_json.modules[1]["source-path"]
if not source_path then
wprint("source-path not found in libc++.modules.json: %s", path.normalize(modules_json_path))
return
end
local std_module_directory = path.directory(source_path)
| try_std_module_directory = path.join(path.directory(get_original_file(get_clang_path(target))), std_module_directory) | ||
| if os.isdir(try_std_module_directory) then | ||
| return {path.normalize(path.join(try_std_module_directory, "std.cppm")), path.normalize(path.join(try_std_module_directory, "std.compat.cppm"))} | ||
| end |
There was a problem hiding this comment.
get_clang_path(target) can return nil, which would cause get_original_file(nil) to be called. This will likely lead to a runtime error. You should check if get_clang_path returns a valid path before using it.
local clang_path = get_clang_path(target)
if clang_path then
try_std_module_directory = path.join(path.directory(get_original_file(clang_path)), std_module_directory)
if os.isdir(try_std_module_directory) then
return {path.normalize(path.join(try_std_module_directory, "std.cppm")), path.normalize(path.join(try_std_module_directory, "std.compat.cppm"))}
end
end
| function parse_link_files(filepath) | ||
| if not os.islink(filepath) then | ||
| return filepath | ||
| end | ||
| local target = os.readlink(filepath) | ||
| if path.is_absolute(target) then | ||
| return target | ||
| end | ||
| return path.join(path.directory(filepath), target) | ||
| end | ||
|
|
||
| function get_original_file(filepath) | ||
| while os.islink(filepath) do | ||
| filepath = parse_link_files(filepath) | ||
| end | ||
| return filepath | ||
| end |
There was a problem hiding this comment.
The new functions parse_link_files and get_original_file are only used within this file. It's good practice to declare them as local to limit their scope and avoid polluting the module's global namespace.
local function parse_link_files(filepath)
if not os.islink(filepath) then
return filepath
end
local target = os.readlink(filepath)
if path.is_absolute(target) then
return target
end
return path.join(path.directory(filepath), target)
end
local function get_original_file(filepath)
while os.islink(filepath) do
filepath = parse_link_files(filepath)
end
return filepath
end
1 participant
fix #7073