Conversation
Summary of ChangesHello @waruqi, 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 significantly enhances the 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
|
There was a problem hiding this comment.
Code Review
This pull request significantly improves the configure script by refactoring several functions to use pure shell, which enhances performance and portability. It also introduces comprehensive support for the Ninja generator. The changes are well-structured and the move away from external commands like sed is a great improvement. I've identified a few areas for further improvement, including some code simplification for maintainability and a potential bug in the run script logic for Windows-like environments. Overall, this is a solid contribution.
| if is_host "msys" "cygwin" "mingw"; then | ||
| local projectdir="${xmake_sh_projectdir}" | ||
| local targetfile_rel="./${targetfile}" | ||
| local targetfile_alt="./build/${targetfile#*/}" | ||
| local targetbuilddir="${targetfile%/*}" | ||
| local targetbuilddir_rel="./${targetbuilddir}" | ||
| local targetbuilddir_alt="./build/${targetbuilddir#*/}" | ||
| local run_script="cd \"${projectdir}\" && target=\"${targetfile_rel}\"; alt=\"${targetfile_alt}\"; if [ ! -f \"\$target\" ] && [ -f \"\$alt\" ]; then target=\"\$alt\"; fi; if [ ! -f \"\$target\" ]; then echo \"[ninja run] missing ${targetfile_rel} (and fallback ${targetfile_alt})\"; ls -l \"${targetbuilddir_rel}\" || true; ls -l \"${targetbuilddir_alt}\" || true; exit 1; fi; \"\$target\"" | ||
| _shell_escape_single_quotes "${run_script}"; local run_script_escaped="${_ret}" | ||
| command="sh -lc ${run_script_escaped}" |
There was a problem hiding this comment.
The fallback logic for finding the target executable on msys/mingw environments seems flawed. The expression ${targetfile#*/} on line 4810 strips all directory components from the path, which can lead to incorrect paths and name collisions if multiple targets share the same filename in different directories. For example, build/foo/bar becomes bar, and the fallback path becomes ./build/bar. This is likely not the intended behavior and can cause the run command to fail or run the wrong executable.
A similar issue exists on line 4813 with ${targetbuilddir#*/}.
I recommend removing this complex and fragile fallback logic, or fixing it to correctly handle paths. The primary path should be sufficient if correctly configured.
| _ninja_escape() { | ||
| local rest="${1}" | ||
| case "${rest}" in | ||
| *\$*) | ||
| local result="" | ||
| while :; do | ||
| case "${rest}" in | ||
| *\$*) | ||
| result="${result}${rest%%\$*}\$\$" | ||
| rest="${rest#*\$}" | ||
| ;; | ||
| *) | ||
| result="${result}${rest}" | ||
| break | ||
| ;; | ||
| esac | ||
| done | ||
| _ret="${result}" | ||
| ;; | ||
| *) | ||
| _ret="${rest}" | ||
| ;; | ||
| esac | ||
| } |
There was a problem hiding this comment.
The while loop in this function can be made more concise. The inner case statement is a bit redundant and the loop can be expressed more directly, improving readability.
| _ninja_escape() { | |
| local rest="${1}" | |
| case "${rest}" in | |
| *\$*) | |
| local result="" | |
| while :; do | |
| case "${rest}" in | |
| *\$*) | |
| result="${result}${rest%%\$*}\$\$" | |
| rest="${rest#*\$}" | |
| ;; | |
| *) | |
| result="${result}${rest}" | |
| break | |
| ;; | |
| esac | |
| done | |
| _ret="${result}" | |
| ;; | |
| *) | |
| _ret="${rest}" | |
| ;; | |
| esac | |
| } | |
| _ninja_escape() { | |
| local rest="${1}" | |
| case "${rest}" in | |
| *\$*) | |
| local result="" | |
| while case "${rest}" in *\$*) true;; *) false;; esac; do | |
| result="${result}${rest%%\$*}\$\$" | |
| rest="${rest#*\$}" | |
| done | |
| _ret="${result}${rest}" | |
| ;; | |
| *) | |
| _ret="${rest}" | |
| ;; | |
| esac | |
| } |
| _shell_escape_single_quotes() { | ||
| local value="${1}" | ||
| case "${value}" in | ||
| *"'"*) | ||
| local escaped="" | ||
| while :; do | ||
| case "${value}" in | ||
| *"'"*) | ||
| escaped="${escaped}${value%%\'*}'\"'\"'" | ||
| value="${value#*\'}" | ||
| ;; | ||
| *) | ||
| escaped="${escaped}${value}" | ||
| break | ||
| ;; | ||
| esac | ||
| done | ||
| _ret="'${escaped}'" | ||
| ;; | ||
| *) | ||
| _ret="'${value}'" | ||
| ;; | ||
| esac | ||
| } |
There was a problem hiding this comment.
Similar to _ninja_escape, the while loop in this function can be simplified for better readability and conciseness.
| _shell_escape_single_quotes() { | |
| local value="${1}" | |
| case "${value}" in | |
| *"'"*) | |
| local escaped="" | |
| while :; do | |
| case "${value}" in | |
| *"'"*) | |
| escaped="${escaped}${value%%\'*}'\"'\"'" | |
| value="${value#*\'}" | |
| ;; | |
| *) | |
| escaped="${escaped}${value}" | |
| break | |
| ;; | |
| esac | |
| done | |
| _ret="'${escaped}'" | |
| ;; | |
| *) | |
| _ret="'${value}'" | |
| ;; | |
| esac | |
| } | |
| _shell_escape_single_quotes() { | |
| local value="${1}" | |
| case "${value}" in | |
| *"'"*) | |
| local escaped="" | |
| while case "${value}" in *"'"*) true;; *) false;; esac; do | |
| escaped="${escaped}${value%%\'*}"'\"'\"'" | |
| value="${value#*\'}" | |
| done | |
| _ret="'${escaped}${value}'" | |
| ;; | |
| *) | |
| _ret="'${value}'" | |
| ;; | |
| esac | |
| } |
configure
Outdated
| local command="" | ||
| local use_shell_wrapper=false | ||
| if is_host "msys" "cygwin" "mingw"; then | ||
| use_shell_wrapper=true | ||
| command="mkdir -p \"${objectdir}\" && ${compcmd}" | ||
| else | ||
| command="mkdir -p \"${objectdir}\" && ${compcmd}" | ||
| fi |
There was a problem hiding this comment.
The if/else block contains identical code for setting the command variable. This can be simplified by setting command before the if block and only handling use_shell_wrapper inside it, which removes redundancy and improves maintainability.
| local command="" | |
| local use_shell_wrapper=false | |
| if is_host "msys" "cygwin" "mingw"; then | |
| use_shell_wrapper=true | |
| command="mkdir -p \"${objectdir}\" && ${compcmd}" | |
| else | |
| command="mkdir -p \"${objectdir}\" && ${compcmd}" | |
| fi | |
| local use_shell_wrapper=false | |
| local command="mkdir -p \"${objectdir}\" && ${compcmd}" | |
| if is_host "msys" "cygwin" "mingw"; then | |
| use_shell_wrapper=true | |
| fi |
configure
Outdated
| local command="" | ||
| local use_shell_wrapper=false | ||
| if is_host "msys" "cygwin" "mingw"; then | ||
| use_shell_wrapper=true | ||
| command="mkdir -p \"${targetdir}\" && ${linkcmd}" | ||
| else | ||
| command="mkdir -p \"${targetdir}\" && ${linkcmd}" | ||
| fi |
There was a problem hiding this comment.
This if/else block has duplicated code for setting the command variable. It can be simplified for better readability and maintainability.
| local command="" | |
| local use_shell_wrapper=false | |
| if is_host "msys" "cygwin" "mingw"; then | |
| use_shell_wrapper=true | |
| command="mkdir -p \"${targetdir}\" && ${linkcmd}" | |
| else | |
| command="mkdir -p \"${targetdir}\" && ${linkcmd}" | |
| fi | |
| local use_shell_wrapper=false | |
| local command="mkdir -p \"${targetdir}\" && ${linkcmd}" | |
| if is_host "msys" "cygwin" "mingw"; then | |
| use_shell_wrapper=true | |
| fi |
xmake-io/xmake.sh#8