More xmake work.#2055
Conversation
📝 WalkthroughWalkthroughThe change introduces shared xmake helpers, MIPS runtime and linker configuration, application build scripts, conversion targets, support-target source filtering, a new xmake submodule, and repository filtering and ignore-rule updates. ChangesXmake-based MIPS build integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConversionTarget
participant BinaryDependency
participant BinaryTarget
participant ConversionTool
ConversionTarget->>BinaryDependency: resolve matching psx/mipsel binary
BinaryDependency->>BinaryTarget: read target file
BinaryTarget-->>ConversionTarget: return input path
ConversionTarget->>ConversionTool: run conversion with output path
ConversionTool-->>ConversionTarget: create converted artifact
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/filter-mips/filter.sh:
- Around line 13-14: Update both git filter-branch find predicates to exclude
descendants of every preserved subtree: uC-sdk, psxlua, and xmake-psx, matching
the existing wildcard exclusions used for EABase and EASTL. Keep the directory
exclusions unchanged and apply the same complete whitelist to both the -delete
and git rm passes.
In `@src/mips/common/crt0/cxxglue.c`:
- Line 164: Update the weak atexit definition to match the standard
int-returning signature, preserving its existing callback parameter and
returning 0 from the stub.
In `@tools/xmake/xmake.lua`:
- Around line 41-51: Update get_binary_dep so binary dependencies are validated
before filtering by the psx/mipsel condition: retain the existing single-binary
check, report a diagnostic when a binary dependency is not a psx target, and
ensure only psx/mipsel binaries are assigned to binary. Remove the unreachable
inner plat check and handle mismatched binary dependencies in reachable control
flow.
- Around line 81-98: Update the depend.on_changed call inside the on_build
callback to track both opts.input and the resolved conversion tool binary at
tooldep:targetfile() as dependency files. Preserve the existing conversion
command and input/output behavior so changes to either the game binary or the
tool trigger regeneration.
- Around line 3-13: Update common_rules() so it no longer directly includes both
src/support and src/supportpsx; rely on src/supportpsx/xmake.lua to include
src/support, or add an appropriate guard to ensure src/support/xmake.lua is
evaluated only once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e55eff8a-7e2c-4305-ad8c-dce2b8c9f9fe
📒 Files selected for processing (17)
.github/filter-mips/filter.sh.gitignore.gitmodulessrc/mips/common/crt0/cxxglue.csrc/mips/helloworld/xmake.luasrc/mips/psyqo/examples/hello/xmake.luasrc/mips/psyqo/xmake.luasrc/mips/xmake.ldsrc/mips/xmake.luasrc/support/xmake.luasrc/supportpsx/binloader.ccsrc/supportpsx/xmake.luathird_party/xmake-psxtools/authoring/xmake.luatools/exe2iso/xmake.luatools/ps1-packer/xmake.luatools/xmake/xmake.lua
| git filter-branch -f --tree-filter 'find third_party -depth -not -name uC-sdk -and -not -path third_party/psxlua -and -not -path third_party/EABase -and -not -path third_party/EABase/\* -and -not -path third_party/EASTL -and -not -path third_party/EASTL/\* -and -not -path third_party/xmake-psx -delete || true' --tag-name-filter cat --prune-empty | ||
| git filter-branch -f --tree-filter 'find third_party -depth -not -name uC-sdk -and -not -path third_party/psxlua -and -not -path third_party/EABase -and -not -path third_party/EABase/\* -and -not -path third_party/EASTL -and -not -path third_party/EASTL/\* -and -not -path third_party/xmake-psx -exec git rm -f {} \; || true' --tag-name-filter cat --prune-empty |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Preserve complete whitelisted subtrees.
Because find runs with -depth, the current predicates preserve only the directory entries for uC-sdk, psxlua, and xmake-psx; their descendants still match and are deleted. The second pass repeats the same faulty filters, so the exported history can lose required dependency sources.
Add /* exclusions for each preserved subtree in both commands, matching the existing EABase/EASTL patterns.
Proposed predicate fix
-find third_party -depth -not -name uC-sdk -and -not -path third_party/psxlua -and -not -path third_party/EABase -and -not -path third_party/EABase/\* -and -not -path third_party/EASTL -and -not -path third_party/EASTL/\* -and -not -path third_party/xmake-psx ...
+find third_party -depth -not -name uC-sdk -and -not -path third_party/uC-sdk/\* -and -not -path third_party/psxlua -and -not -path third_party/psxlua/\* -and -not -path third_party/EABase -and -not -path third_party/EABase/\* -and -not -path third_party/EASTL -and -not -path third_party/EASTL/\* -and -not -path third_party/xmake-psx -and -not -path third_party/xmake-psx/\* ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/filter-mips/filter.sh around lines 13 - 14, Update both git
filter-branch find predicates to exclude descendants of every preserved subtree:
uC-sdk, psxlua, and xmake-psx, matching the existing wildcard exclusions used
for EABase and EASTL. Keep the directory exclusions unchanged and apply the same
complete whitelist to both the -delete and git rm passes.
|
|
||
| // we're not going to care about exit cleanup | ||
| __attribute__((weak)) void __cxa_atexit(void (*func)(void*), void* arg, void* dso_handle) {} | ||
| __attribute__((weak)) void atexit(void (*func)(void)) {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Mismatch in atexit signature.
The standard C library signature for atexit returns an int (typically returning 0 on success), not void. Declaring it as void can lead to prototype mismatches if <stdlib.h> is included elsewhere, or undefined behavior if a caller attempts to evaluate the return value.
🐛 Proposed fix
-__attribute__((weak)) void atexit(void (*func)(void)) {}
+__attribute__((weak)) int atexit(void (*func)(void)) { return 0; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| __attribute__((weak)) void atexit(void (*func)(void)) {} | |
| __attribute__((weak)) int atexit(void (*func)(void)) { return 0; } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mips/common/crt0/cxxglue.c` at line 164, Update the weak atexit
definition to match the standard int-returning signature, preserving its
existing callback parameter and returning 0 from the stub.
| function common_rules() | ||
| add_rules("mode.debug", "mode.release") | ||
|
|
||
| includes( | ||
| path.join(support_root, "src", "support"), | ||
| path.join(support_root, "src", "supportpsx") | ||
| ) | ||
|
|
||
| add_requires("fmt", "zlib") | ||
| set_languages("c++26") | ||
| end |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does xmake includes() function deduplicate the same included file path across multiple different root xmake.lua config files in one project run?
💡 Result:
The xmake includes function does not inherently perform deduplication on the included file paths themselves [1][2]. The includes function is designed to load and parse additional xmake.lua files to construct the project's hierarchical configuration tree [1]. If you call includes("path/to/xmake.lua") multiple times in your project configuration, xmake will process each inclusion request. While xmake possesses a robust internal deduplication system for configuration values (such as defines, includedirs, and links) when building targets [3][4], this is distinct from the mechanism that parses project files via includes [5][6]. If the same file is included multiple times, its contents will be evaluated by the xmake interpreter each time it is encountered. Consequently, if that file contains definitions (such as target, option, or rule declarations), those definitions may be re-processed, which can lead to conflicts, warnings, or unintended side effects depending on how the declarations are structured [7]. It is standard practice to use conditional logic (e.g., checking if a variable or target is already defined) or to organize your project structure to ensure that specific configuration files are only included once [8][1].
Citations:
- 1: https://xmake.io/guide/project-configuration/multi-level-directories.html
- 2: https://xmake.io/posts/quickstart-11-subprojects.html
- 3: https://deepwiki.com/xmake-io/xmake/2.7-configuration-system
- 4: https://github.com/xmake-io/xmake/blob/69f46e71/xmake/core/base/scopeinfo.lua
- 5: https://github.com/xmake-io/xmake/blob/dev/xmake/core/project/project.lua
- 6: https://github.com/xmake-io/xmake/blob/69f46e71/xmake/core/project/project.lua
- 7: 尝试把 includes("子模块1", "子模块2" ...)，放到单独的 project.lua 里面，此时会报"warning: includes("子模块1") cannot find any files! xmake-io/xmake#6441
- 8: https://github.com/xmake-io/xmake/blob/69f46e71/core/xmake.lua
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the xmake-related files first.
git ls-files 'tools/xmake/**' '*xmake.lua' | sed 's#^`#FILE` #'
printf '\n--- tools/xmake/xmake.lua ---\n'
cat -n tools/xmake/xmake.lua
printf '\n--- references to common_rules() ---\n'
rg -n "common_rules\(\)" -S tools/xmake . --glob '*xmake.lua'
printf '\n--- includes(path.join(support_root... references ---\n'
rg -n 'includes\(.*support_root' -S tools/xmake . --glob '*xmake.lua'Repository: grumpycoders/pcsx-redux
Length of output: 5134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/support/xmake.lua ---'
cat -n src/support/xmake.lua
printf '\n%s\n' '--- src/supportpsx/xmake.lua ---'
cat -n src/supportpsx/xmake.lua
printf '\n%s\n' '--- tools/authoring/xmake.lua ---'
cat -n tools/authoring/xmake.lua
printf '\n%s\n' '--- tools/exe2iso/xmake.lua ---'
cat -n tools/exe2iso/xmake.lua
printf '\n%s\n' '--- tools/ps1-packer/xmake.lua ---'
cat -n tools/ps1-packer/xmake.lua
printf '\n%s\n' '--- src/mips/xmake.lua ---'
cat -n src/mips/xmake.luaRepository: grumpycoders/pcsx-redux
Length of output: 4972
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- top-level xmake.lua files ---'
git ls-files '*xmake.lua' | sed -n '1,40p'
printf '\n%s\n' '--- repository root xmake.lua if present ---'
if [ -f xmake.lua ]; then cat -n xmake.lua; fi
printf '\n%s\n' '--- xmake include references in repo ---'
rg -n '^\s*includes\(' -S . --glob '*xmake.lua'Repository: grumpycoders/pcsx-redux
Length of output: 1302
🌐 Web query:
xmake includes duplicate include same file multiple times global project run target redefinition
💡 Result:
In Xmake, issues involving "duplicate" files or targets often arise from how the build system interprets project structure, target declarations, and source file management [1][2][3]. Below is a synthesis of common causes and solutions for these issues. ### 1. Duplicate Target Name Redefinition In Xmake, target names must be unique within a project [3]. When you declare target("name") in multiple places (e.g., in subprojects included via includes()), Xmake interprets this as an attempt to modify or append to the existing target rather than creating a new one [2][3]. This often leads to conflicting build settings or duplicate file lists [2]. * Solution: Use unique names for distinct targets [2]. If you need multiple targets to produce similar outputs (e.g., a static library and a shared library), give them unique names and use set_basename() to ensure the generated file name remains the same [3]. * Alternative: Use namespace() to isolate targets in subprojects, which helps prevent unintended merging of targets with the same name across different directories [1][4]. ### 2. Duplicate Source Files (Same Name, Different Paths) Xmake can struggle when two source files with the same filename (e.g., src/a/renderer.c and src/b/renderer.c) are added to the same target, as they may attempt to output the same object file (renderer.obj), causing linker errors or build warnings [5]. * Solution: Ensure source files have unique filenames, or organize your project structure so that build tools can distinguish between them. For IDE project generation (like vs2022), this is particularly sensitive [5]. ### 3. Duplicate Inclusions (Header/Module Issues) If you are encountering errors related to "duplicate inclusion" of the same file (often seen with C++20 modules or precompiled headers), it is frequently due to how the compiler manages these files or how Xmake tracks dependencies across subprojects [6][7]. * Header Files: Use standard C/C++ header guards (#ifndef HEADER_H ...) to prevent multiple inclusions. * C++ Modules: MSVC and other compilers are strict about module interface units. If the same module is included/compiled multiple times across different subprojects, it can trigger C7661 or C3474 errors [6][7]. Ensure you are not inadvertently including the same module source file in multiple targets that are built together [7]. * Precompiled Headers: Ensure set_pcxxheader() is not conflicted by adding the same file to add_headerfiles() [8]. ### 4. General Recommendations * Check Policies: Use set_policy("diagnosis.check_build_deps", true) to debug why Xmake is rebuilding or including certain files unexpectedly [9]. * Use add_sysincludedirs: If you are dealing with external dependencies, use add_sysincludedirs() instead of add_includedirs() to inform the compiler that these are external, which can help manage include priority and warnings [10]. * Update Xmake: Many issues related to target management and namespaces have been addressed in recent development versions. Always test with the latest version using xmake update -s dev if you suspect a bug [5][11].
Citations:
- 1: Handle target name clashes in subprojects (Support namespace) xmake-io/xmake#3380
- 2: xmake添加多个deps时，deps内部有相互依赖的情况下编译无法通过 xmake-io/xmake#5342
- 3: xmake.lua 中包含两个同名但不同类的 target 时，编译报错 xmake-io/xmake#640
- 4: 不同 namespace 下同名 target 编译时出错 xmake-io/xmake#6292
- 5: 生成vs2022工程，同一工程不同目录有两个同名的文件，无法正常编译 xmake-io/xmake#2016
- 6: 模块存在重复定义 xmake-io/xmake#3927
- 7: 三个子项目使用了同一个c++模块,同一个文件不应该被编译三次?error C3474: 无法打开输出文件 xmake-io/xmake#3746
- 8: vs project cannot load if using precompiled header xmake-io/xmake#1171
- 9: xmake includes are always being built even if --shallow is used xmake-io/xmake#6316
- 10: Add support for external/system include directories xmake-io/xmake#1017
- 11: Option 'target' given more than once xmake-io/xmake#4381
Avoid including src/support twice
common_rules() pulls in both src/support and src/supportpsx, but src/supportpsx/xmake.lua already includes src/support. That can evaluate src/support/xmake.lua more than once in the same run, so keep the include graph single-path or guard the shared include.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/xmake/xmake.lua` around lines 3 - 13, Update common_rules() so it no
longer directly includes both src/support and src/supportpsx; rely on
src/supportpsx/xmake.lua to include src/support, or add an appropriate guard to
ensure src/support/xmake.lua is evaluated only once.
| if dep_target:get("kind") == "binary" and dep_target:get("plat") == "psx" and dep_target:get("arch") == "mipsel" then | ||
| if binary then | ||
| return nil, | ||
| "target can only have one binary dependency, found '" .. | ||
| binary:name() .. "' and '" .. dep_target:name() .. "'" | ||
| end | ||
| if dep_target:get("plat") ~= "psx" then | ||
| return nil, "target dependency '" .. dep_target:name() .. "' must be a psx target" | ||
| end | ||
| binary = dep_target | ||
| end |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unreachable validation branch in get_binary_dep.
The outer if at Line 41 already requires plat == "psx" before entering this block, so the if dep_target:get("plat") ~= "psx" check at Lines 47-49 can never be true — it's dead code. Any dependency with kind == "binary" but the wrong plat/arch (e.g. a host tool) is silently skipped instead of producing a diagnostic, and this inner check never fires to catch it.
🐛 Proposed fix to make the psx/mipsel validation reachable
- if dep_target:get("kind") == "binary" and dep_target:get("plat") == "psx" and dep_target:get("arch") == "mipsel" then
- if binary then
- return nil,
- "target can only have one binary dependency, found '" ..
- binary:name() .. "' and '" .. dep_target:name() .. "'"
- end
- if dep_target:get("plat") ~= "psx" then
- return nil, "target dependency '" .. dep_target:name() .. "' must be a psx target"
- end
- binary = dep_target
- end
+ if dep_target:get("kind") == "binary" then
+ if dep_target:get("plat") ~= "psx" or dep_target:get("arch") ~= "mipsel" then
+ return nil, "target dependency '" .. dep_target:name() .. "' must be a psx/mipsel target"
+ end
+ if binary then
+ return nil,
+ "target can only have one binary dependency, found '" ..
+ binary:name() .. "' and '" .. dep_target:name() .. "'"
+ end
+ binary = dep_target
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if dep_target:get("kind") == "binary" and dep_target:get("plat") == "psx" and dep_target:get("arch") == "mipsel" then | |
| if binary then | |
| return nil, | |
| "target can only have one binary dependency, found '" .. | |
| binary:name() .. "' and '" .. dep_target:name() .. "'" | |
| end | |
| if dep_target:get("plat") ~= "psx" then | |
| return nil, "target dependency '" .. dep_target:name() .. "' must be a psx target" | |
| end | |
| binary = dep_target | |
| end | |
| if dep_target:get("kind") == "binary" then | |
| if dep_target:get("plat") ~= "psx" or dep_target:get("arch") ~= "mipsel" then | |
| return nil, "target dependency '" .. dep_target:name() .. "' must be a psx/mipsel target" | |
| end | |
| if binary then | |
| return nil, | |
| "target can only have one binary dependency, found '" .. | |
| binary:name() .. "' and '" .. dep_target:name() .. "'" | |
| end | |
| binary = dep_target | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/xmake/xmake.lua` around lines 41 - 51, Update get_binary_dep so binary
dependencies are validated before filtering by the psx/mipsel condition: retain
the existing single-binary check, report a diagnostic when a binary dependency
is not a psx target, and ensure only psx/mipsel binaries are assigned to binary.
Remove the unreachable inner plat check and handle mismatched binary
dependencies in reachable control flow.
| on_build(function(target) | ||
| import("core.project.depend") | ||
|
|
||
| os.mkdir(target:targetdir()) | ||
| local binary, err = get_binary_dep(target) | ||
| if not binary then | ||
| raise(tool_name .. " target error: " .. err) | ||
| end | ||
| local opts = { | ||
| input = binary:targetfile(), | ||
| output = target:targetfile(), | ||
| } | ||
| depend.on_changed(function() | ||
| print(conversion_name .. " of %s to %s", opts.input, opts.output) | ||
| local tooldep = target:dep(tool) | ||
| os.vrunv(tooldep:targetfile(), { opts.input, "-o", opts.output }) | ||
| end, { files = opts.input }) | ||
| end) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Conversion output isn't invalidated when the conversion tool changes.
depend.on_changed only watches opts.input (Line 97). If the tool binary itself (pcsx.authoring, pcsx.exe2iso, pcsx.ps1-packer, etc.) is rebuilt after a change to its own source, the previously generated .iso/.psexe output is not considered stale, since only the input game binary is tracked. Stale conversion artifacts can silently ship after a tool bugfix without triggering a rebuild.
🔧 Proposed fix to also track the tool binary as a dependency file
on_build(function(target)
import("core.project.depend")
os.mkdir(target:targetdir())
local binary, err = get_binary_dep(target)
if not binary then
raise(tool_name .. " target error: " .. err)
end
+ local tooldep = target:dep(tool)
local opts = {
input = binary:targetfile(),
output = target:targetfile(),
}
depend.on_changed(function()
print(conversion_name .. " of %s to %s", opts.input, opts.output)
- local tooldep = target:dep(tool)
os.vrunv(tooldep:targetfile(), { opts.input, "-o", opts.output })
- end, { files = opts.input })
+ end, { files = { opts.input, tooldep:targetfile() } })
end)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| on_build(function(target) | |
| import("core.project.depend") | |
| os.mkdir(target:targetdir()) | |
| local binary, err = get_binary_dep(target) | |
| if not binary then | |
| raise(tool_name .. " target error: " .. err) | |
| end | |
| local opts = { | |
| input = binary:targetfile(), | |
| output = target:targetfile(), | |
| } | |
| depend.on_changed(function() | |
| print(conversion_name .. " of %s to %s", opts.input, opts.output) | |
| local tooldep = target:dep(tool) | |
| os.vrunv(tooldep:targetfile(), { opts.input, "-o", opts.output }) | |
| end, { files = opts.input }) | |
| end) | |
| on_build(function(target) | |
| import("core.project.depend") | |
| os.mkdir(target:targetdir()) | |
| local binary, err = get_binary_dep(target) | |
| if not binary then | |
| raise(tool_name .. " target error: " .. err) | |
| end | |
| local tooldep = target:dep(tool) | |
| local opts = { | |
| input = binary:targetfile(), | |
| output = target:targetfile(), | |
| } | |
| depend.on_changed(function() | |
| print(conversion_name .. " of %s to %s", opts.input, opts.output) | |
| os.vrunv(tooldep:targetfile(), { opts.input, "-o", opts.output }) | |
| end, { files = { opts.input, tooldep:targetfile() } }) | |
| end) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/xmake/xmake.lua` around lines 81 - 98, Update the depend.on_changed
call inside the on_build callback to track both opts.input and the resolved
conversion tool binary at tooldep:targetfile() as dependency files. Preserve the
existing conversion command and input/output behavior so changes to either the
game binary or the tool trigger regeneration.
1 participant
No description provided.