Conversation
…rms; add Nim test files for static and shared libraries
…ks in dependency flags
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 advances Xmake's support for the Nim programming language. It introduces a new, extensive test project that validates Nim's ability to link with both static and shared C libraries, as well as external system dependencies like zlib and stb_image. Concurrently, the core Nim language rules and toolchain definitions within Xmake have been updated to correctly handle system include paths, system library linking, architecture-specific compilation flags, and advanced runtime path (RPATH) configurations, ensuring more robust and flexible Nim project builds. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 Nim language support within the xmake build system. It introduces new test cases for linking Nim code with C headers and libraries, demonstrating static and shared library integration. Crucially, the changes to xmake/languages/nim/xmake.lua and xmake/modules/core/tools/nim.lua enhance the Nim toolchain's ability to handle architecture-specific flags, system include directories, system links, and runtime path (rpath) configurations across various platforms. These additions are essential for robust cross-compilation and linking of Nim projects, bringing its support closer to other well-established languages in xmake.
| if self:is_plat("linux", "macosx", "bsd") then | ||
| if level == "debug" or level == "all" then | ||
| return "--passL:-s" | ||
| return '--passL:"-s"' |
There was a problem hiding this comment.
The change from --passL:-s to --passL:"-s" improves consistency in how linker flags are quoted. While functionally equivalent in many cases, explicit quoting can prevent issues with spaces or special characters in paths/flags in other contexts.
--passL:"-s"
| -- make the includedir flag | ||
| function nf_includedir(self, dir) | ||
| return {"--passC:-I" .. path.translate(dir)} | ||
| return {string.format('--passC:"-I%s"', path.translate(dir))} |
There was a problem hiding this comment.
Using string.format for constructing the --passC:"-I%s" flag is a good practice. It enhances readability and ensures proper quoting of the include path, which is important for paths containing spaces or special characters.
return {string.format('--passC:"-I%s"', path.translate(dir))}
| function nf_link(self, lib) | ||
| if self:is_plat("windows") then | ||
| return "--passL:" .. lib .. ".lib" | ||
| return string.format('--passL:"%s.lib"', lib) |
There was a problem hiding this comment.
Similar to the include directory flag, using string.format and explicit quoting for the Windows library path (.lib) improves consistency and robustness against paths with special characters.
return string.format('--passL:"%s.lib"', lib)
| return "--passL:" .. lib .. ".lib" | ||
| return string.format('--passL:"%s.lib"', lib) | ||
| else | ||
| return string.format('--passL:"-l%s"', lib) |
There was a problem hiding this comment.
Applying string.format and explicit quoting to the Unix-like library flag (-l%s) maintains consistency with other flag constructions and can prevent potential parsing issues.
return string.format('--passL:"-l%s"', lib)
| function nf_linkdir(self, dir) | ||
| if self:is_plat("windows") then | ||
| return {"--passL:-libpath:" .. path.translate(dir)} | ||
| return {string.format('--passL:"-libpath:%s"', path.translate(dir))} |
There was a problem hiding this comment.
The use of string.format with explicit quoting for the Windows library search path (-libpath:%s) is a good improvement for consistency and handling paths correctly.
return {string.format('--passL:"-libpath:%s"', path.translate(dir))}
| return {string.format('--passL:"-libpath:%s"', path.translate(dir))} | ||
| else | ||
| return {"--passL:-L" .. path.translate(dir)} | ||
| return {string.format('--passL:"-L%s"', path.translate(dir))} |
There was a problem hiding this comment.
Consistent application of string.format and explicit quoting for Unix-like library search paths (-L%s) improves the overall quality and robustness of flag generation.
return {string.format('--passL:"-L%s"', path.translate(dir))}
#7272