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
Ignored Files
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 introduces significant new functionality by adding support for the Ninja build generator. The implementation is comprehensive, covering build, clean, install, and run targets. The code is well-structured into helper functions, which is great for organization. I have identified a bug in the dollar-sign escaping logic for Ninja files that could cause issues in certain edge cases, and I've provided a fix. Additionally, I've pointed out a particularly large and complex function that could benefit from refactoring to improve long-term maintainability. Overall, this is a valuable addition to the project.
configure
Outdated
| # escape value for writing into ninja files | ||
| _ninja_escape() { | ||
| local value="${1}" | ||
| value=$(printf '%s' "${value}" | sed 's/\$\$/\$/g' | sed 's/\$/\$\$/g') |
There was a problem hiding this comment.
The escaping logic in _ninja_escape appears to be flawed for certain edge cases. For instance, a string with three consecutive dollar signs ($$$) would be incorrectly escaped, potentially leading to a corrupt build.ninja file. A simpler, more robust implementation would be to replace every literal $ with $$.
| value=$(printf '%s' "${value}" | sed 's/\$\$/\$/g' | sed 's/\$/\$\$/g') | |
| value=$(printf '%s' "${value}" | sed 's/\$/$$/g') |
configure
Outdated
| _ninja_add_install_target() { | ||
| local target=${1} | ||
| _get_target_file "${target}"; local targetfile="${_ret}" | ||
| path_filename "${targetfile}"; local filename="${_ret}" | ||
| _get_target_item "${target}" "installdir"; local installdir="${_ret}" | ||
| _get_target_item "${target}" "kind"; local targetkind="${_ret}" | ||
|
|
||
| local install_for_soname=false | ||
| local version="" | ||
| local soname="" | ||
| local extension="" | ||
| local filename_basename="" | ||
| if test_eq "${targetkind}" "shared"; then | ||
| _get_target_item "${target}" "version"; version="${_ret}" | ||
| _get_target_soname "${target}"; soname="${_ret}" | ||
| _get_target_extension "${target}"; extension="${_ret}" | ||
| path_basename "${filename}"; filename_basename="${_ret}" | ||
| if test_nz "${soname}" && test_nz "${version}"; then | ||
| local targetfilename_with_version_guess="${filename}.${version}" | ||
| if test_eq "${extension}" ".dylib"; then | ||
| targetfilename_with_version_guess="${filename_basename}.${version}${extension}" | ||
| fi | ||
| if test_nq "${soname}" "${filename}" && test_nq "${soname}" "${targetfilename_with_version_guess}"; then | ||
| install_for_soname=true | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| local ninjadir="${xmake_sh_builddir}/.ninja" | ||
| local scriptdir="${xmake_sh_projectdir}/${ninjadir}" | ||
| mkdir -p "${scriptdir}" | ||
| local scriptfile_rel="${ninjadir}/install_${target}.sh" | ||
| local scriptfile="${xmake_sh_projectdir}/${scriptfile_rel}" | ||
|
|
||
| _shell_escape_single_quotes "${_install_prefix_default}"; local escaped_prefix_default="${_ret}" | ||
| _shell_escape_single_quotes "${installdir}"; local escaped_installdir_template="${_ret}" | ||
| _shell_escape_single_quotes "${xmake_sh_projectdir}"; local escaped_projectdir="${_ret}" | ||
| _shell_escape_single_quotes "${_install_bindir_default}"; local escaped_bindir_template="${_ret}" | ||
| _shell_escape_single_quotes "${_install_libdir_default}"; local escaped_libdir_template="${_ret}" | ||
| _shell_escape_single_quotes "${_install_includedir_default}"; local escaped_includedir_template="${_ret}" | ||
|
|
||
| cat > "${scriptfile}" <<EOF | ||
| #!/bin/sh | ||
| set -e | ||
|
|
||
| prefix=${escaped_prefix_default} | ||
| if [ -n "\${PREFIX}" ]; then | ||
| prefix="\${PREFIX}" | ||
| fi | ||
| destdir="\${DESTDIR}" | ||
| installdir_template=${escaped_installdir_template} | ||
| prefix_sed=\$(printf '%s' "\${prefix}" | sed 's/[#&"\\]/\\&/g') | ||
| placeholder='\${prefix}' | ||
|
|
||
| projectdir=${escaped_projectdir} | ||
| cd "\${projectdir}" | ||
|
|
||
| installdir="" | ||
| if [ -z "\${installdir_template}" ]; then | ||
| installdir="\${destdir}" | ||
| if [ -n "\${installdir}" ]; then | ||
| prefix_trim="\${prefix#/}" | ||
| installdir="\${installdir}/\${prefix_trim}" | ||
| else | ||
| installdir="\${prefix}" | ||
| fi | ||
| else | ||
| installdir="\${installdir_template}" | ||
| installdir=\$(printf '%s' "\${installdir}" | sed "s#\${placeholder}#\${prefix_sed}#g") | ||
| if [ -n "\${destdir}" ]; then | ||
| case "\${installdir}" in | ||
| /*) ;; | ||
| *) installdir="\${destdir}/\${installdir}" ;; | ||
| esac | ||
| fi | ||
| fi | ||
|
|
||
| installdir_sed=\$(printf '%s' "\${installdir}" | sed 's/[#&"\\]/\\&/g') | ||
|
|
||
| resolve_prefix_path() { | ||
| printf '%s' "\$1" | sed "s#\${placeholder}#\${installdir_sed}#g" | ||
| } | ||
|
|
||
| copy_file() { | ||
| local src="\$1" | ||
| local dst_template="\$2" | ||
| local dst | ||
| dst=\$(resolve_prefix_path "\${dst_template}") | ||
| echo "installing \${src} to \${dst}" | ||
| mkdir -p "\$(dirname "\${dst}")" | ||
| cp -p "\${src}" "\${dst}" | ||
| } | ||
|
|
||
| bindir_template=${escaped_bindir_template} | ||
| libdir_template=${escaped_libdir_template} | ||
| includedir_template=${escaped_includedir_template} | ||
| EOF | ||
|
|
||
| if test_eq "${targetkind}" "binary"; then | ||
| _shell_escape_single_quotes "${targetfile}"; local escaped_targetfile="${_ret}" | ||
| local dest_template="${_install_bindir_default}/${filename}" | ||
| _shell_escape_single_quotes "${dest_template}"; local escaped_dest_template="${_ret}" | ||
| echo "copy_file ${escaped_targetfile} ${escaped_dest_template}" >> "${scriptfile}" | ||
| elif ${install_for_soname}; then | ||
| local base_template="${_install_libdir_default}/${filename}" | ||
| local version_template="${_install_libdir_default}/${filename}.${version}" | ||
| if test_eq "${extension}" ".dylib"; then | ||
| path_basename "${filename}"; local basename="${_ret}" | ||
| version_template="${_install_libdir_default}/${basename}.${version}${extension}" | ||
| fi | ||
| _shell_escape_single_quotes "${targetfile}"; local escaped_targetfile="${_ret}" | ||
| _shell_escape_single_quotes "${version_template}"; local escaped_version_template="${_ret}" | ||
| _shell_escape_single_quotes "${soname}"; local escaped_soname="${_ret}" | ||
| _shell_escape_single_quotes "${filename}"; local escaped_filename="${_ret}" | ||
| cat >> "${scriptfile}" <<EOF | ||
| copy_file ${escaped_targetfile} ${escaped_version_template} | ||
| dst_version=\$(resolve_prefix_path ${escaped_version_template}) | ||
| dst_dir=\$(dirname "\${dst_version}") | ||
| filename_version=\$(basename "\${dst_version}") | ||
| ( | ||
| cd "\${dst_dir}" | ||
| ln -sf "\${filename_version}" ${escaped_soname} | ||
| ln -sf ${escaped_soname} ${escaped_filename} | ||
| ) | ||
| EOF | ||
| elif test_eq "${targetkind}" "static" || test_eq "${targetkind}" "shared"; then | ||
| _shell_escape_single_quotes "${targetfile}"; local escaped_targetfile="${_ret}" | ||
| local dest_template="${_install_libdir_default}/${filename}" | ||
| _shell_escape_single_quotes "${dest_template}"; local escaped_dest_template="${_ret}" | ||
| echo "copy_file ${escaped_targetfile} ${escaped_dest_template}" >> "${scriptfile}" | ||
| fi | ||
|
|
||
| _get_target_item "${target}" "headerfiles"; local headerfiles="${_ret}" | ||
| if test_nz "${headerfiles}"; then | ||
| local srcheaderfile="" | ||
| for srcheaderfile in ${headerfiles}; do | ||
| string_split "${srcheaderfile}" ":" | ||
| local srcheaderpath="${_ret}" | ||
| local rootdir="${_ret2}" | ||
| local prefixdir="${_ret3}" | ||
| local headername="${_ret4}" | ||
| if test_z "${headername}"; then | ||
| path_filename "${srcheaderpath}"; headername="${_ret}" | ||
| fi | ||
| local dstheaderdir_template="${_install_includedir_default}" | ||
| if test_nz "${prefixdir}"; then | ||
| dstheaderdir_template="${dstheaderdir_template}/${prefixdir}" | ||
| fi | ||
| local dstheaderfile_template="" | ||
| if test_nz "${rootdir}"; then | ||
| path_relative "${rootdir}" "${srcheaderpath}"; local subfile="${_ret}" | ||
| dstheaderfile_template="${dstheaderdir_template}/${subfile}" | ||
| else | ||
| dstheaderfile_template="${dstheaderdir_template}/${headername}" | ||
| fi | ||
| _shell_escape_single_quotes "${srcheaderpath}"; local escaped_src="${_ret}" | ||
| _shell_escape_single_quotes "${dstheaderfile_template}"; local escaped_dst="${_ret}" | ||
| echo "copy_file ${escaped_src} ${escaped_dst}" >> "${scriptfile}" | ||
| done | ||
| fi | ||
|
|
||
| _get_target_item "${target}" "installfiles"; local installfiles="${_ret}" | ||
| if test_nz "${installfiles}"; then | ||
| local srcinstallfile="" | ||
| for srcinstallfile in ${installfiles}; do | ||
| string_split "${srcinstallfile}" ":" | ||
| local srcfilepath="${_ret}" | ||
| local rootdir="${_ret2}" | ||
| local prefixdir="${_ret3}" | ||
| local installname="${_ret4}" | ||
| if test_z "${installname}"; then | ||
| path_filename "${srcfilepath}"; installname="${_ret}" | ||
| fi | ||
| local dst_template="${installdir}" | ||
| if test_z "${dst_template}"; then | ||
| dst_template="\${prefix}" | ||
| fi | ||
| if test_nz "${prefixdir}"; then | ||
| dst_template="${dst_template}/${prefixdir}" | ||
| fi | ||
| if test_nz "${rootdir}"; then | ||
| path_relative "${rootdir}" "${srcfilepath}"; local subfile="${_ret}" | ||
| dst_template="${dst_template}/${subfile}" | ||
| else | ||
| dst_template="${dst_template}/${installname}" | ||
| fi | ||
| _shell_escape_single_quotes "${srcfilepath}"; local escaped_src="${_ret}" | ||
| _shell_escape_single_quotes "${dst_template}"; local escaped_dst="${_ret}" | ||
| echo "copy_file ${escaped_src} ${escaped_dst}" >> "${scriptfile}" | ||
| done | ||
| fi | ||
|
|
||
| echo "" >> "${scriptfile}" | ||
| chmod +x "${scriptfile}" | ||
|
|
||
| local command="sh ${scriptfile_rel}" | ||
| local description="installing.${_target_mode} ${target}" | ||
| _ninja_escape "${command}"; command="${_ret}" | ||
| _ninja_escape "${description}"; description="${_ret}" | ||
| echo "build install.${target}: command | ${targetfile}" >> "${xmake_sh_ninjafile}" | ||
| echo " command = ${command}" >> "${xmake_sh_ninjafile}" | ||
| echo " description = ${description}" >> "${xmake_sh_ninjafile}" | ||
| echo " restat = 0" >> "${xmake_sh_ninjafile}" | ||
| echo "" >> "${xmake_sh_ninjafile}" | ||
| } |
There was a problem hiding this comment.
The function _ninja_add_install_target is very long (over 200 lines) and has high complexity, primarily because it generates an entire shell script to handle installations. This concentration of logic makes the function difficult to read, understand, and maintain. I recommend refactoring this function by breaking it down into smaller, more focused helper functions. For example, you could create separate functions for generating the install script's header, handling shared library soname logic, and processing header and installation files. This would significantly improve the code's modularity and readability.
9019592 to
a10c579
Compare
Uh oh!
There was an error while loading. Please reload this page.