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

wamr: Add external module registration mechanism #2609

Merged
merged 2 commits into from
Sep 28, 2024

Conversation

no1wudi
Copy link
Contributor

@no1wudi no1wudi commented Sep 27, 2024

Summary

This patch adds module registration mechanism for WAMR runtime.

To register a module, these steps are required:

  1. Include Module.mk in your module's Makefile
  2. Define WAMR_MODULE_NAME as the module name
  3. Implement bool wamr_module_WAMR_MODULE_NAME_register(void) function in your module's source file
  4. Enable INTERPRETERS_WAMR_EXTERNAL_MODULE_REGISTRY in menuconfig

Impact

build system

Testing

Local

@nuttxpr
Copy link

nuttxpr commented Sep 27, 2024

[Experimental Bot, please feedback here]

The PR summary is well-written and informative, but the other sections lack detail. Here's a breakdown of what's missing and how to improve it:

Impact:

  • Insufficient Detail: Simply stating "build system" isn't enough. Be specific about how the build system changes:
    • Are new Kconfig options added?
    • Do existing Makefiles need modification to use this feature?
    • Will users see any changes in the build output?
  • Address all points: The impact section has several bullet points. Even if they don't apply, explicitly state "NO" for each to show you've considered them.

Testing:

  • Uninformative: "Local" doesn't tell us anything useful.
  • Provide specifics:
    • Build Host: What operating system, architecture, and compiler did you use?
    • Target: Which simulator or physical board did you test on? Include the architecture and board configuration.
  • Show, don't tell: Instead of just saying "Testing logs...", include actual build logs (if relevant) and, more importantly, logs demonstrating the module registration mechanism working as intended.

Example of improved sections:

Impact:

  • Is new feature added? YES - Adds a module registration mechanism for WAMR runtime.
  • Is existing feature changed? NO
  • Impact on user (will user need to adapt to change)? YES - Users who want to create WAMR modules will need to follow the new registration process outlined in the summary.
  • Impact on build (will build process change)? YES - Enabling the INTERPRETERS_WAMR_EXTERNAL_MODULE_REGISTRY option in menuconfig will change the build process to incorporate registered modules.
  • Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO
  • Impact on documentation (is update required / provided)? YES - Documentation should be updated to explain the module registration process and how to create WAMR modules.
  • Impact on security (any sort of implications)? NO
  • Impact on compatibility (backward/forward/interoperability)? NO
  • Anything else to consider? N/A

Testing:

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): Ubuntu 20.04, x86_64, GCC 9.4.0
  • Target(s): QEMU simulator, ARM Cortex-M4F, board_config_name

Testing logs before change:

(Logs demonstrating the absence of module registration or any issues it aims to solve)

Testing logs after change:

(Logs showing successful module registration, module loading, and ideally, module functionality)

In conclusion: While the PR addresses a clear need, it lacks the necessary details for reviewers to assess its impact and verify its functionality. By providing specific information in the Impact and Testing sections, you'll significantly improve the PR's quality and make the review process smoother.

@no1wudi no1wudi force-pushed the wasm branch 2 times, most recently from 2cf621a to d65e909 Compare September 27, 2024 09:31
@no1wudi
Copy link
Contributor Author

no1wudi commented Sep 27, 2024

The style issue should be fixed with apache/nuttx#13673

This patch adds module registration mechanism for WAMR runtime.

To register a module, these steps are required:
1. Include Module.mk in your module's Makefile
2. Define WAMR_MODULE_NAME as the module name
3. Implement bool wamr_module_WAMR_MODULE_NAME_register(void) function in your module's source file
4. Enable INTERPRETERS_WAMR_EXTERNAL_MODULE_REGISTRY in menuconfig

Signed-off-by: Huang Qi <[email protected]>
This example demonstrates how to register a external module
into WAMR runtime.

Signed-off-by: Huang Qi <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 2ecaf0e into apache:master Sep 28, 2024
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants