Conversation
Summary of ChangesHello, 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 enhances the Xmake build system's integration with the Zig programming language, specifically improving the linking process for shared libraries on Windows. It introduces a refined mechanism to correctly identify and include necessary object and source files during the linking phase, resolving a known issue and ensuring more robust build outcomes for Zig projects. 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. Changelog
Activity
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 introduces a fix for linking shared libraries with Zig on Windows when the project contains source files from other languages. A new local function, _linkfiles, has been added to correctly filter and assemble the list of files for the Zig linker, addressing the issue of unresolved external symbols. The changes are logical and effectively solve the problem. I have one suggestion to refactor the new function for improved readability by reducing nested logic.
| local function _linkfiles(self, objectfiles, targetkind, opt) | ||
| opt = opt or {} | ||
| local target = opt.target | ||
| if targetkind == "shared" and target and target:is_plat("windows") then | ||
| local sourcebatch = target:sourcebatches()["zig.build"] | ||
| if sourcebatch and sourcebatch.sourcefiles and sourcebatch.objectfiles then | ||
| local zigobjectfiles = {} | ||
| for _, objectfile in ipairs(sourcebatch.objectfiles) do | ||
| zigobjectfiles[path.translate(objectfile)] = true | ||
| end | ||
| local linkfiles = {} | ||
| for _, objectfile in ipairs(table.wrap(objectfiles)) do | ||
| if not zigobjectfiles[path.translate(objectfile)] then | ||
| table.insert(linkfiles, objectfile) | ||
| end | ||
| end | ||
| table.join2(linkfiles, sourcebatch.sourcefiles) | ||
| return linkfiles | ||
| end | ||
| end | ||
| return objectfiles | ||
| end |
There was a problem hiding this comment.
This function can be slightly refactored to improve readability by reducing nesting. Using guard clauses at the beginning of the function makes the main logic path clearer and the code easier to follow.
local function _linkfiles(self, objectfiles, targetkind, opt)
opt = opt or {}
local target = opt.target
if not (targetkind == "shared" and target and target:is_plat("windows")) then
return objectfiles
end
local sourcebatch = target:sourcebatches()["zig.build"]
if not (sourcebatch and sourcebatch.sourcefiles and sourcebatch.objectfiles) then
return objectfiles
end
local zigobjectfiles = {}
for _, objectfile in ipairs(sourcebatch.objectfiles) do
zigobjectfiles[path.translate(objectfile)] = true
end
local linkfiles = {}
for _, objectfile in ipairs(table.wrap(objectfiles)) do
if not zigobjectfiles[path.translate(objectfile)] then
table.insert(linkfiles, objectfile)
end
end
table.join2(linkfiles, sourcebatch.sourcefiles)
return linkfiles
end
| end | ||
| end | ||
| return objectfiles | ||
| end |
There was a problem hiding this comment.
? why? I've fixed it (#7395). The current patch is rather hacky and cannot be approved.
There was a problem hiding this comment.
I tried this one (#7395) but it still didn't work, maybe I'm wrong but I wasn't able to have access to symbols even when I was on dev branch
There was a problem hiding this comment.
It works for me. -lc has been added.
c:\users\wangrunqing\appdata\local.xmake\packages\z\zig\0.15.2\ebe55ddddf714323a6cd2869c52a8e6b\zig.exe build-lib -dynamic -fPIC -target x86_64-windows-msvc -lc -lkernel32 -lntdll -fstrip -dead_strip -femit-bin=build\windows\x64\release\ZigDynLibWindows.dll build.objs\ZigDynLibWindows\windows\x64\release\src\exported_function.zig.obj
PS C:\Users\wangrunqing\Downloads\Zig-DynLib-Windows> xmake f -c
checking for platform ... windows (x64)
checking for Microsoft C/C++ Compiler (x64) ... ok
checking for Microsoft Visual Studio (x64) version ... 2026
PS C:\Users\wangrunqing\Downloads\Zig-DynLib-Windows> xmake -rv
checking for C:\Users\wangrunqing\AppData\Local\.xmake\packages\z\zig\0.15.2\ebe55ddddf714323a6cd2869c52a8e6b\zig ... c:\users\wangrunqing\appdata\local\.xmake\packages\z\zig\0.15.2\ebe55ddddf714323a6cd2869c52a8e6b\zig.exe
checking for the zig compiler (zc) ... zig.exe
checking for c:\users\wangrunqing\appdata\local\.xmake\packages\z\zig\0.15.2\ebe55ddddf714323a6cd2869c52a8e6b\zig.exe ... ok
checking for flags (-O ReleaseFast) ... ok
checking for flags (--cache-dir build\.objs\ZigDynLibWindows\windows\x64\release\zig-cache) ... ok
checking for cl.exe ... C:\Program Files\Microsoft Visual Studio\18\Insiders\VC\Tools\MSVC\14.50.35503\bin\HostX64\x64\cl.exe
checking for the c++ compiler (cxx) ... cl.exe
[ 23%]: <ZigDynLibWindows> compiling.release src\exported_function.zig
c:\users\wangrunqing\appdata\local\.xmake\packages\z\zig\0.15.2\ebe55ddddf714323a6cd2869c52a8e6b\zig.exe build-obj -target x86_64-windows-msvc -fPIC -O ReleaseFast --cache-dir build\.objs\ZigDynLibWindows\windows\x64\release\zig-cache -femit-bin=build\.objs\ZigDynLibWindows\windows\x64\release\src\exported_function.zig.obj src\exported_function.zig
checking for C:\Users\wangrunqing\AppData\Local\.xmake\packages\z\zig\0.15.2\ebe55ddddf714323a6cd2869c52a8e6b\zig ... c:\users\wangrunqing\appdata\local\.xmake\packages\z\zig\0.15.2\ebe55ddddf714323a6cd2869c52a8e6b\zig.exe
checking for the zig shared library linker (zcsh) ... zig.exe
[ 47%]: <ZigDynLibWindows> linking.release ZigDynLibWindows.dll
c:\users\wangrunqing\appdata\local\.xmake\packages\z\zig\0.15.2\ebe55ddddf714323a6cd2869c52a8e6b\zig.exe build-lib -dynamic -fPIC -target x86_64-windows-msvc -lc -lkernel32 -lntdll -fstrip -dead_strip -femit-bin=build\windows\x64\release\ZigDynLibWindows.dll build\.objs\ZigDynLibWindows\windows\x64\release\src\exported_function.zig.obj
checking for cl.exe ... C:\Program Files\Microsoft Visual Studio\18\Insiders\VC\Tools\MSVC\14.50.35503\bin\HostX64\x64\cl.exe
checking for the c compiler (cc) ... cl.exe
[100%]: build ok, spent 10.969sThere was a problem hiding this comment.
Does the dll file contains exported symbols ?
There was a problem hiding this comment.
It's simply a problem with missing libc library symbols; adding -lc will solve it. I don't know what you mean by "exported symbols".
|
I believe #7395 has been fixed, at least it works for me. If you still have problems, please provide more error logs. |
2 participants
Fix #7395