Conversation
Summary of ChangesHello @luadebug, 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 extends Xmake's capabilities by introducing comprehensive support for the WebAssembly System Interface (WASI) platform. It integrates WASI across core functionalities, including platform detection, build system configurations (CMake, Meson), package management (Conan), and Go environment settings. The changes ensure that projects targeting WASI can be built and managed effectively within the Xmake ecosystem, aligning WASI with existing WebAssembly (WASM) support where appropriate. 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
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 adds support for the WASI platform across various modules and scripts. The changes are mostly about adding wasi to platform checks alongside wasm. While most changes are correct, there are a few issues:
- In Meson configuration files, the
systemis incorrectly set toemscriptenfor WASI, where it should bewasi. - Some Emscripten-specific rules for
wasm(likeinstallfilesfor HTML output andpreloadfiles) are incorrectly applied to thewasiplatform, which has different characteristics and toolchains.
These issues should be addressed to ensure correct behavior for WASI builds.
| file:print("cpu = '%s'", cpu) | ||
| file:print("endian = 'little'") | ||
| elseif package:is_plat("wasm") then | ||
| elseif package:is_plat("wasm", "wasi") then |
There was a problem hiding this comment.
For Meson, the system for the WASI platform should be 'wasi', not 'emscripten'. This change incorrectly groups wasi with wasm to use 'emscripten' for both. This will lead to an incorrect cross-compilation setup for Meson-based packages targeting WASI. You should add a condition to set system = 'wasi' when the platform is wasi.
| file:print("cpu = '%s'", cpu) | ||
| file:print("endian = 'little'") | ||
| elseif is_plat("wasm") then | ||
| elseif is_plat("wasm", "wasi") then |
There was a problem hiding this comment.
Similar to another file in this PR, this change incorrectly sets the Meson system to 'emscripten' for the WASI platform. It should be 'wasi'. This will cause issues when trying to build Meson projects for WASI. Please add logic to differentiate between wasm and wasi and set the system property accordingly.
| -- copy other files generated by emcc (see https://emscripten.org/docs/tools_reference/emcc.html#emcc-o-target) | ||
| rule("platform.wasm.installfiles") | ||
| on_load("wasm", function (target) | ||
| on_load("wasm", "wasi", function (target) |
There was a problem hiding this comment.
This rule seems specific to Emscripten's HTML output, which is not applicable to the WASI platform. The WASI platform produces standalone .wasm binaries, not .html files with associated JavaScript. Applying this rule to wasi is likely incorrect and could lead to unexpected behavior during installation.
| -- @see https://github.com/xmake-io/xmake/issues/3613 | ||
| rule("platform.wasm.preloadfiles") | ||
| on_load("wasm", function (target) | ||
| on_load("wasm", "wasi", function (target) |
There was a problem hiding this comment.
The --preload-file flag is an Emscripten-specific feature for embedding files into a virtual filesystem. The WASI platform uses a different toolchain (wasi-sdk) and has its own filesystem APIs. Applying this rule to wasi is likely incorrect as the wasi-sdk toolchain does not support this flag.
|
I don't think we should add a separate wasi platform; the Wasm platform + the wasi toolchain is sufficient. xmake f -p wasm --toolchain=wasi --sdk=/xxxAdding a new platform doubles the maintenance workload, especially package management. xmake-repo requires additional maintenance support for the wasi platform. |
| if not python then | ||
| raise("emrun or python not found, which is required for running wasm target in browser!") | ||
| end | ||
| local url = "http://localhost:8000/" .. path.unix(path.relative(targetfile, rundir)) |
There was a problem hiding this comment.
This thing always kept opening .js file instead of .html though not about this PR.
|
Alright so far this works. |
Issue was confirmed that it present for now.
Resolves: #7327
After changes applied: