Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake(enhance):Enhanced full WASM library and application compilation #2971

Merged
merged 4 commits into from
Jan 23, 2025

Conversation

xuxin930
Copy link
Contributor

Summary

This is a complete enhancement and addition to the WASM build support for the CMake version.
#2324

Enhanced full WASM library and application compilation

1.add complete compilation FLAGS for wasm toolchain
2.wasm build no longer traverses the native directory, saving build time
3.implement OPT and AOT process actions for wasm files
4.create a bridge interface for navtie build and wasm build
5.instead of traversing all directories again, use nuttx's wasm_add function as the registration

Impact

enhance wasm feat

Testing

# add wasm application

    wasm_add_application(
      NAME
      hello_world
      SRCS
      hello_world.c
      STACK_SIZE
      4096
      WINCLUDES
      ${INCDIR}
      WAMR_MODE
      INT
      WLDFLAGS
      ${WLDFLAGS}
      WCFLAGS
      ${CFLAGS}
      INSTALL_NAME
      ${CMAKE_BINARY_DIR}/bin/wasm/hello.wasm)
      
 # add wasm lib
 
  wasm_add_library(
    NAME
    frameworks
    SRCS
    ${SRCS}
    WINCLUDES
    ${INCDIR}
    WCFLAGS
    ${CFLAGS})

1.add complete compilation FLAGS for wasm toolchain
2.wasm build no longer traverses the native directory, saving build time
3.implement OPT and AOT process actions for wasm files
4.create a bridge interface for navtie build and wasm build

Signed-off-by: xuxin19 <[email protected]>
System is unknown to cmake, create:
Platform/WASI to use this system, please post your config file on discourse.cmake.org so it can be added to cmake
Your CMakeCache.txt file was copied to CopyOfCMakeCache.txt. Please post that file on discourse.cmake.org.
CMake Error at CMakeLists.txt:100 (add_subdirectory):
  add_subdirectory given source
  /home/data/vela/tmp/apps/frameworks/security/ta/hello_world
  /home/data/vela/tmp/apps/frameworks/security/ta/" which is not an
  existing directory.

Signed-off-by: xuxin19 <[email protected]>
Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @xuxin930 :-)

Lets just wait for the CI checks to complete :-)

@nuttxpr
Copy link

nuttxpr commented Jan 23, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, but it is missing some critical information.

Here's a breakdown of what's good and what needs improvement:

Strengths:

  • Clear Summary of Changes: The summary explains the what and how of the changes related to WASM build support. The link to the related NuttX Apps PR is helpful.
  • Impact Description (though lacking detail): It acknowledges the impact is an enhancement of the WASM feature.
  • Example CMake Usage: Providing examples of how to use the new CMake functions is excellent for demonstrating the changes and helping users.

Weaknesses (requiring attention before merging):

  • Missing Issue References: If this addresses a specific issue in either NuttX or NuttX Apps, those issue numbers should be explicitly linked. Even if it's a new feature not tied to an existing issue, consider creating an issue to track the feature request.
  • Insufficient Impact Detail: "enhance wasm feat" is far too vague. Be specific:
    • User Impact: How will users interact with this new functionality? Are there new configuration options? Will existing WASM workflows change?
    • Build Impact: You mention build time improvements. Quantify this if possible (e.g., "Reduces WASM build time by approximately X%"). Are there any new build dependencies?
    • Hardware Impact: Does this affect any specific architectures or boards?
    • Documentation Impact: Does the documentation need to be updated to explain this new feature? If so, confirm that documentation updates are included in the PR.
    • Security Impact: Even if there's no security impact, explicitly state "NO."
    • Compatibility Impact: Are there any backward compatibility concerns? Does this change how existing WASM applications will build?
  • Incomplete Testing Information: The CMake example is helpful, but it doesn't show actual test results. The template asks for logs before and after the change to demonstrate the improvement. You must provide evidence that the changes work as intended on real hardware or a simulator. Include:
    • Build Host Details: OS, CPU architecture, compiler version.
    • Target Details: Architecture (e.g., sim, RISC-V, ARM), board, and configuration.
    • Actual Test Logs: Show the output of relevant build commands and any tests run on the target. Demonstrate the improved build time if applicable.

Example of Improved Testing Section:

Testing

I confirm that changes are verified on local setup and works as intended:
* Build Host(s): Ubuntu 22.04, x86_64, GCC 11.3.0
* Target(s): sim:qemu-rv32ima

Testing logs before change (building the hello_world example with the old WASM build system):

```

Testing logs after change (building the same example with the new CMake functions):

<Insert actual build log showing improved time taken>

Additionally, I ran the following tests on the QEMU simulator:

<Output of tests demonstrating functionality>



In summary, the PR is on the right track, but it needs more detail and evidence to meet the NuttX standards for review and merging.  Address the weaknesses outlined above, and the PR will be much stronger.

@xiaoxiang781216 xiaoxiang781216 merged commit add50b3 into apache:master Jan 23, 2025
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants