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 significantly expands Xmake's capabilities by introducing first-class support for C# projects. It enables developers to define, build, and manage C# applications and libraries using Xmake's declarative syntax, seamlessly integrating with the .NET ecosystem. The changes encompass automatic project file generation, robust 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for C# projects in xmake. It adds a new csharp rule, a dotnet toolchain, and integrates with the dotnet CLI for building, publishing, and managing dependencies via NuGet. The implementation is well-structured, with clear separation of concerns into modules for project generation, property mapping, and dependency detection. The test suite is also expanded with various C# project types. My review includes a few suggestions for improvement, such as fixing an invalid target framework in a test project, correcting a typo in a user-facing menu, refactoring some duplicated code, and optimizing the install rules for better performance.
|
|
||
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <TargetFramework>net10.0</TargetFramework> |
There was a problem hiding this comment.
The TargetFramework net10.0 is not a valid Target Framework Moniker (TFM). This will cause the build to fail. Please use a valid TFM, for example net8.0 if you intend to use .NET 8.
<TargetFramework>net8.0</TargetFramework>
| {category = "Cross Complation Configuration/Compiler Configuration" } | ||
| , {nil, "cs", "kv", nil, "The C# Compiler" } | ||
|
|
||
| , {category = "Cross Complation Configuration/Linker Configuration" } | ||
| , {nil, "ld", "kv", nil, "The Linker" } | ||
| , {nil, "sh", "kv", nil, "The Shared Library Linker" } | ||
|
|
||
| , {category = "Cross Complation Configuration/Compiler Flags Configuration" } | ||
| , {nil, "csflags", "kv", nil, "The C# Compiler Flags" } | ||
|
|
||
| , {category = "Cross Complation Configuration/Linker Flags Configuration" } | ||
| , {nil, "ldflags", "kv", nil, "The Binary Linker Flags" } | ||
| , {nil, "shflags", "kv", nil, "The Shared Library Linker Flags" } |
There was a problem hiding this comment.
There is a typo "Complation" in the category names for the configuration menu. It should be "Compilation". This is a user-facing string that appears in xmake f --menu.
{category = "Cross Compilation Configuration/Compiler Configuration" }
, {nil, "cs", "kv", nil, "The C# Compiler" }
, {category = "Cross Compilation Configuration/Linker Configuration" }
, {nil, "ld", "kv", nil, "The Linker" }
, {nil, "sh", "kv", nil, "The Shared Library Linker" }
, {category = "Cross Compilation Configuration/Compiler Flags Configuration" }
, {nil, "csflags", "kv", nil, "The C# Compiler Flags" }
, {category = "Cross Compilation Configuration/Linker Flags Configuration" }
, {nil, "ldflags", "kv", nil, "The Binary Linker Flags" }
, {nil, "shflags", "kv", nil, "The Shared Library Linker Flags" }
| -- | ||
|
|
||
| -- attempt to check it from known flags | ||
| function _check_from_knownargs(flags, opt) |
There was a problem hiding this comment.
The opt parameter in the _check_from_knownargs function is declared but not used. It can be removed to improve code clarity.
function _check_from_knownargs(flags)
| function _find_target_root(targets, name) | ||
| local namelower = name:lower() | ||
| for targetname in pairs(targets or {}) do | ||
| local targetpkg = targetname:match("^([^/]+)") or targetname | ||
| local targetpkglower = targetpkg:lower() | ||
| local targetnamelower = targetname:lower() | ||
| local normalized_targetpkg = targetpkglower:gsub("[._%-]", "") | ||
| local normalized_name = namelower:gsub("[._%-]", "") | ||
| if targetnamelower == namelower | ||
| or targetnamelower:startswith(namelower .. "/") | ||
| or targetpkglower == namelower | ||
| or normalized_targetpkg == normalized_name then | ||
| return targetname | ||
| end | ||
| end | ||
| end | ||
|
|
||
| function _find_library_root(libraries, name) | ||
| local namelower = name:lower() | ||
| for libraryname in pairs(libraries or {}) do | ||
| local librarypkg = libraryname:match("^([^/]+)") or libraryname | ||
| local librarypkglower = librarypkg:lower() | ||
| local librarynamelower = libraryname:lower() | ||
| local normalized_librarypkg = librarypkglower:gsub("[._%-]", "") | ||
| local normalized_name = namelower:gsub("[._%-]", "") | ||
| if librarynamelower == namelower | ||
| or librarynamelower:startswith(namelower .. "/") | ||
| or librarypkglower == namelower | ||
| or normalized_librarypkg == normalized_name then | ||
| return libraryname | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
The functions _find_target_root and _find_library_root are nearly identical. They can be refactored into a single generic function to reduce code duplication and improve maintainability.
For example, you could create a generic function _find_root(collection, name) and call it from both places:
function _find_root(collection, name)
local namelower = name:lower()
for itemname in pairs(collection or {}) do
local itempkg = itemname:match("^([^"]+)") or itemname
local itempkglower = itempkg:lower()
local itemnamelower = itemname:lower()
local normalized_itempkg = itempkglower:gsub("[._%-]", "")
local normalized_name = namelower:gsub("[._%-]", "")
if itemnamelower == namelower
or itemnamelower:startswith(namelower .. "/")
or itempkglower == namelower
or normalized_itempkg == normalized_name then
return itemname
end
end
end
function _find_target_root(targets, name)
return _find_root(targets, name)
end
function _find_library_root(libraries, name)
return _find_root(libraries, name)
end
xmake/rules/csharp/install.lua
Outdated
| function main(target, opt) | ||
| local function _q(arg) | ||
| arg = tostring(arg) | ||
| if arg:find("[%s\"]") then | ||
| arg = "\"" .. arg:gsub("\"", "\\\"") .. "\"" | ||
| end | ||
| return arg | ||
| end | ||
|
|
||
| local csprojfile = assert(csharp_common.find_or_generate_csproj(target), "target(%s): missing csharp .csproj file!", target:name()) | ||
| local dotnet = csharp_common.get_dotnet_program(target) | ||
| local configuration = csharp_common.build_mode_to_configuration() | ||
| local verbosity = csharp_common.get_dotnet_verbosity() | ||
|
|
||
| local install_path = target:installdir() | ||
| if target:is_binary() then | ||
| install_path = target:installdir("bin") | ||
| elseif target:is_static() or target:is_shared() then | ||
| install_path = target:installdir("lib") | ||
| end | ||
| if not install_path or #install_path == 0 then | ||
| return | ||
| end | ||
|
|
||
| local install_abs = path.is_absolute(install_path) and install_path or path.absolute(install_path, os.projectdir()) | ||
| os.mkdir(install_abs) | ||
|
|
||
| local rid = csharp_common.get_runtime_identifier(target) | ||
| local argv = { | ||
| "publish", csprojfile, | ||
| "--nologo", | ||
| "--configuration", configuration, | ||
| "--verbosity", verbosity, | ||
| "--output", install_abs | ||
| } | ||
| if rid and target:is_binary() then | ||
| table.join2(argv, {"--runtime", rid}) | ||
| end | ||
| csharp_common.append_target_flags(target, argv) | ||
|
|
||
| local runopt = csharp_common.get_dotnet_runopt(csprojfile) | ||
| if os.vrunv then | ||
| os.vrunv(dotnet, argv, runopt) | ||
| elseif os.runv then | ||
| os.runv(dotnet, argv, runopt) | ||
| elseif os.execv then | ||
| os.execv(dotnet, argv, runopt) | ||
| elseif os.vrun then | ||
| local cmd = _q(dotnet) | ||
| for _, arg in ipairs(argv) do | ||
| cmd = cmd .. " " .. _q(arg) | ||
| end | ||
| os.vrun(cmd, runopt) | ||
| elseif os.run then | ||
| local cmd = _q(dotnet) | ||
| for _, arg in ipairs(argv) do | ||
| cmd = cmd .. " " .. _q(arg) | ||
| end | ||
| os.run(cmd, runopt) | ||
| else | ||
| local targetdir = target:targetdir() | ||
| if targetdir and os.isdir(targetdir) then | ||
| os.cp(path.join(targetdir, "**"), install_abs, {rootdir = targetdir}) | ||
| end | ||
| end | ||
|
|
||
| if target:is_binary() then | ||
| local targetdir = target:targetdir() | ||
| if targetdir and os.isdir(targetdir) then | ||
| os.cp(path.join(targetdir, "**"), install_abs, {rootdir = targetdir}) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
The install rule re-runs dotnet publish, which is inefficient because the build step already produces the necessary artifacts. The install step should copy artifacts from the build output directory (target:targetdir()) to the installation directory. This avoids redundant work and aligns with the typical xmake build lifecycle.
function main(target, opt)
local install_path = target:installdir()
if target:is_binary() then
install_path = target:installdir("bin")
elseif target:is_static() or target:is_shared() then
install_path = target:installdir("lib")
end
if not install_path or #install_path == 0 then
return
end
local install_abs = path.is_absolute(install_path) and install_path or path.absolute(install_path, os.projectdir())
os.mkdir(install_abs)
local targetdir = target:targetdir()
if targetdir and os.isdir(targetdir) then
os.cp(path.join(targetdir, "**"), install_abs, {rootdir = targetdir})
end
end
| function main(target, batchcmds, opt) | ||
| local csprojfile = assert(csharp_common.find_or_generate_csproj(target), "target(%s): missing csharp .csproj file!", target:name()) | ||
| local dotnet = csharp_common.get_dotnet_program(target) | ||
| local configuration = csharp_common.build_mode_to_configuration() | ||
| local verbosity = csharp_common.get_dotnet_verbosity() | ||
|
|
||
| local install_path = target:installdir() | ||
| if target:is_binary() then | ||
| install_path = target:installdir("bin") | ||
| elseif target:is_static() or target:is_shared() then | ||
| install_path = target:installdir("lib") | ||
| end | ||
|
|
||
| local install_abs | ||
| local argv = { | ||
| "publish", csprojfile, | ||
| "--nologo", | ||
| "--configuration", configuration, | ||
| "--verbosity", verbosity | ||
| } | ||
| if install_path and #install_path > 0 then | ||
| install_abs = path.is_absolute(install_path) and install_path or path.absolute(install_path, os.projectdir()) | ||
| table.join2(argv, {"--output", install_abs}) | ||
| end | ||
|
|
||
| local rid = csharp_common.get_runtime_identifier(target) | ||
| if rid and target:is_binary() then | ||
| table.join2(argv, {"--runtime", rid}) | ||
| end | ||
| csharp_common.append_target_flags(target, argv) | ||
|
|
||
| batchcmds:show_progress(opt.progress, "${color.build.target}publishing.csharp.$(mode) %s", target:name()) | ||
| if install_abs then | ||
| batchcmds:mkdir(install_abs) | ||
| end | ||
| batchcmds:vrunv(dotnet, argv, csharp_common.get_dotnet_runopt(csprojfile)) | ||
| end |
There was a problem hiding this comment.
Similar to the install rule, this installcmd rule re-runs dotnet publish. For efficiency, it should instead generate batch commands to copy the build artifacts from target:targetdir() to the installation directory.
function main(target, batchcmds, opt)
local install_path = target:installdir()
if target:is_binary() then
install_path = target:installdir("bin")
elseif target:is_static() or target:is_shared() then
install_path = target:installdir("lib")
end
if not install_path or #install_path == 0 then
return
end
local install_abs = path.is_absolute(install_path) and install_path or path.absolute(install_path, os.projectdir())
batchcmds:mkdir(install_abs)
local targetdir = target:targetdir()
if targetdir and os.isdir(targetdir) then
batchcmds:show_progress(opt.progress, "${color.build.target}installing.csharp.$(mode) %s", target:name())
batchcmds:cp(path.join(targetdir, "**"), install_abs, {rootdir = targetdir})
end
end
|
wow！这个可以，看了之后在想有没有可能结合SourceGenerator 编译时生成 C# 源代码（*.g.cs）来做P/Invoke的胶水代码生成，打通C#项目和C/C++的交互性。 |
wow! This works. After reading it, I wondered if it would be possible to combine SourceGenerator to generate C# source code (*.g.cs) during compilation to generate glue code for P/Invoke and open up the interactivity between C# projects and C/C++. |
#7342
It also works on macOS.