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

Add best practices for plugin #191

Merged
merged 15 commits into from
Mar 4, 2024
Merged

Conversation

mhmohona
Copy link
Contributor

@mhmohona mhmohona commented Nov 8, 2023

Explanation

Added best practice for plugins.

Related issue

#85

What type of PR is this

/kind documentation

Proposed Changes

Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
Copy link
Collaborator

alabulei1 commented Nov 8, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Overall Summary:

The majority of the changes in the GitHub Pull Request are straightforward additions and improvements to the plugin documentation, without introducing any potential problems or errors. However, there are a few noteworthy findings:

  1. The patch assumes prior familiarity with the WasmEdge project and its repository structure. Some clarification and additional context may be needed to accommodate a wider range of users.
  2. Instructions for building and running tests assume a Unix-like environment and the use of the sudo command, which may not be valid for all users.
  3. The documentation lacks guidance on how to interpret test results or handle test failures.
  4. In some patches, there is a lack of code changes to demonstrate the implementation of best practices or the test guide.
  5. Instructions for creating a plugin in C/C++ have some inaccuracies and assumptions that may hinder understanding for users.
  6. Some sections of the documentation have been removed without adequately addressing the impact on users' understanding and usage.
  7. The patch adds a hyperlink that may not function correctly or may lead to an invalid file.

Apart from these findings, the changes provide valuable guidance on best practices for developing, testing, securing, and publishing WasmEdge plugins. The documentation improvements enhance clarity and readability overall.

Details

Commit 746f6985fb19edd559b15a08a0aa9fec25414380

Key changes:

  • Added a new document best_practice.md in the docs/contribute/plugin/ directory.
  • Added content to the best_practice.md document, describing best practices for developing a WasmEdge plugin.
  • Added a Chinese translation of the best_practice.md document in the i18n/zh/docusaurus-plugin-content-docs/current/contribute/plugin/ directory.

Potential problems:

  • There are no potential problems with the changes in this pull request. The changes appear to be straightforward and do not introduce any errors or issues.

Commit 785cd6e2df378f485d13236f22f5a24699f862f2

Key changes:

  • Added a new section titled "Best Practice" to the plugin best practices documentation.

Potential problems:

  • No potential problems found. The patch appears to be a simple addition of a new section with no issues.

Commit 7ca91af314645c0668ceaa81a5cc25418406666e

Key changes in the patch:

  • The "Testing the Plugin" section has been updated.
  • The section now provides more detailed information about different types of testing - unit testing, integration testing, and fuzz testing.
  • Links to examples and test cases in the WasmEdge repository have been added.

Potential problems:

  • No potential problems are identified in the patch.

Commit 3c1f946fa6c4b6eab63a13732799da0cc4cb86bc

Key changes in the patch:

  • Added instructions on how to build and run tests for the wasmedge-image plugin.
  • Provided steps to analyze the test results.

Potential problems:

  • The patch does not introduce any code changes, only documentation updates. Therefore, there may not be any potential problems with the patch itself.
  • However, there are some considerations related to the documentation:
    • The patch assumes that the reader is already familiar with the WasmEdge project and its repository structure.
    • The patch assumes that the reader has a basic understanding of building and running tests using common development tools like cmake and make.
    • The provided commands for building and installing WasmEdge and the wasmedge-image plugin assume a Unix-like environment and the use of the sudo command. These assumptions may not be valid for all users, leading to potential issues or confusion.
    • The patch suggests using the ctest command to run tests, but it does not clarify how to interpret the test results or handle any failures encountered.

Overall, the patch provides valuable information on testing a WasmEdge plugin but could benefit from additional context and clarification to accommodate a wider range of users.

Commit a50b0138828f7b1e3462eebd46fbe542804b8cd4

Key changes in the patch:

  1. Added a new document test_plugin.md that provides guidance on writing tests for WasmEdge plugins.
  2. Updated the existing best_practice.md document with the new section on testing plugins and added a link to the test_plugin.md document.

Potential problems:

  1. It seems that the patch includes changes to multiple repositories, which may cause confusion and conflicts during the merge process. It would be better to separate the changes into separate commits or patches for each repository.
  2. The patch introduces a new document and updates an existing one, but it doesn't include any actual code changes or bug fixes. It would be useful to include actual code changes in the patch to demonstrate the implementation of the best practices and the test guide.

Commit feaeddd1caf0473eadbadff64fce1a67f3d1bacd

Key changes in the patch:

  • The patch adds code examples and instructions for creating a plugin in C or C++.
  • It provides guidance on compiling the plugin as a shared library using the g++ compiler.
  • It suggests testing the plugin after the compilation process.

Potential problems:

  • The patch lacks a description or explanation of the purpose and significance of creating a plugin.
  • The code example provided for defining a function in C/C++ is incorrect. The language should be specified as cpp, not ccp.
  • The patch assumes the usage of the g++ compiler for creating a shared library, without considering other platforms or build systems.
  • The instructions for compiling the shared library do not mention the need to link against the WasmEdge runtime or any other necessary libraries.
  • The patch does not provide any information on how to register the plugin with the WasmEdge runtime or how to use it.
  • It suggests testing the plugin without providing any information on how to perform the testing or what it should entail.

Overall, the patch provides a basic starting point for creating a plugin but lacks important details and guidance on key aspects such as purpose, registration, and testing.

Commit 7236393d1aafc1c067e251c308a4f83bce0b9636

Key changes:

  • Added a new section titled "Securing the Plugin" in the best_practice.md file.
  • The new section emphasizes the importance of security in software development and provides guidelines for ensuring plugin security.

Potential problems:

  • No potential problems found in this patch.

Commit 5ea06795e9c66d97902cd505542616b4680dc249

Key changes:

  • Added a section on securing the plugin, including best practices for input validation, error handling, and secure coding practices.
  • Added a section on publishing the plugin, including information about the official WasmEdge plugin repository and the process of creating a pull request.

Potential problems:

  • No potential problems found.

Overall, the changes provide valuable information on best practices for developing and publishing WasmEdge plugins, including security considerations. The addition of the new sections improves the documentation for plugin developers.

Commit 807b3ecf16d04e920106f97cd1115c92f43d9e10

Key changes:

  • Added a new section titled "Using the Latest Version of WasmEdge" which provides best practices for using the latest version of WasmEdge, checking for updates, installing the latest version, verifying the installation, and updating existing plugins.
  • Updated the "Publishing the Plugin" section with detailed steps for exporting SDKs for C/C++ and Rust. This includes creating a header file, compiling the plugin, packaging the SDK, creating a Rust library, building the Rust library, and packaging the Rust SDK.

Potential problems:

  • No potential problems were identified in the provided patch. The changes seem to be focused on adding helpful information and best practices for developing and publishing plugins for WasmEdge.

Commit 8922c75fd97f37ac9a4be28fce1d39ff701490ca

Key changes:

  • Updated the formatting of the document.
  • Added a note about developing custom tests for WasmEdge plugins.

Potential problems:

  • No major issues or problems were identified in this patch.

Commit df784aca328b8d466ed8f0d238553876e31fe144

Key changes in the patch:

  1. Removal of sections about checking for updates and verifying the installation of WasmEdge.
  2. Softening the statement about the need to update existing plugins, and adding a link to the WasmEdge API reference.
  3. Adding links to specific language guides for developing plugins in C, C++, and Rust.
  4. Removing the section about exporting SDKs for C/C++ and providing a Rust-specific SDK section.
  5. Adding an example Rust code for creating a Rust library that interfaces with the wasmedge-image plugin.
  6. Adjusting the formatting and structure of the document.

Potential problems:

  1. The removal of the sections about checking for updates and verifying the installation may prevent new developers from understanding how to stay up-to-date with the latest WasmEdge versions or how to ensure the correct installation.
  2. The softening of the statement about updating existing plugins may lead to outdated plugins that are not compatible with the latest WasmEdge version.
  3. The removal of the section about exporting SDKs for C/C++ may make it difficult for developers to understand how to distribute their plugins.
  4. The addition of the Rust-specific SDK section may introduce inconsistency and confusion since the other sections focus on C and C++.
  5. It's unclear whether the example Rust code provided is complete or if it requires additional code to function correctly.

Commit 1cd8779f80de6d49cf21a87e9fbf40aad8af9ec7

Key changes:

  • Added a hyperlink to the image plugin in the documentation.
  • Removed the instructions for building the WasmEdge runtime and image plugin, which was redundant and already covered in other documentation.
  • Reorganized the content to improve clarity and readability.

Potential problems:

  • The hyperlink added may not work correctly if the URL is incorrect or the file it's pointing to doesn't exist.
  • The removed instructions for building the WasmEdge runtime and image plugin may confuse users who are expecting to find those instructions in the documentation. It would be helpful to provide a brief explanation or reference to where users can find those instructions.

Commit e7f91abddba1ae64c5ba049b8f0a4e45ef97e9b0

Key Changes:

  • Added a hyperlink for the image plugin.
  • Removed sections about building and installing the WasmEdge runtime and the wasmedge-image plugin.

Potential Problems:

  • The hyperlink for the image plugin may not work or may not be relevant.
  • The removed sections about building and installing the WasmEdge runtime and the wasmedge-image plugin are necessary for the plugin's setup and usage and should not have been removed.
  • The patch does not provide any new content or improvements other than the hyperlink.

Commit 8971f8c7daafae51feb3d0fbe0a3cbf28996859d

Key changes in the patch:

  • The language in the code example has been changed from "C" to "C++".
  • Several sections of the document have been edited for clarity and wording.

Potential problems:

  • In the code example for validating inputs, the language is still marked as "cpp" instead of "c". This should be updated to "c" to match the code.
  • The section on compiling the plugin is missing some important details. It should include information on how to compile C++ plugins as well, not just C plugins.
  • The section on testing the plugin is not well-defined. It would be helpful to provide examples or guidelines for testing plugins in different languages.

Overall, the changes seem minor and there are no major issues.

Commit c5ac16e19d5d237c7d406f7280e2741217ca0fe3

Key changes in the patch:

  1. The title of the document has been changed to "Best Practice" instead of "Best Practices".
  2. The introduction sentence of the document has been modified to improve clarity and wording.
  3. The section on "Writing and Compiling the Plugin" has been completely rewritten to provide clearer instructions and examples.
  4. The section on "Error Handling and Input Validation" has been updated to provide better code examples and improve readability.
  5. The section on "Contributing to the WasmEdge Community" has been modified to include a link to the official WasmEdge repository.

Potential problems:

  1. The patch introduces a typo in the sentence "ensure the plugin is well purformed". It should be "ensure the plugin is well-performed".
  2. The code example in the "Writing and Compiling the Plugin" section is incorrect. It uses the language identifier ccp instead of cpp. This should be fixed.
  3. The code example in the "Error Handling and Input Validation" section uses the language identifier cpp instead of c. This should be fixed.
  4. In the last sentence, there is a minor typo. "successful and efficient plugin development process for WasmEdge" should be "successful and efficient plugin development process in WasmEdge".

Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
5. **Testing the Plugin:**
Testing is a crucial step in the development process to ensure that the plugin behaves as expected. There are various testing techniques such as unit testing, integration testing, and fuzz testing that can be used. Unit testing involves testing individual functions or units of code, integration testing involves testing the interaction between the plugin and the Wasm program, and fuzz testing involves generating random input to the plugin to test its robustness and resilience.

6. **Publishing the Plugin:**
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is too general. Can you describe more details?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alabulei1 do you think this section is okay now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You provide some details, which is good. But I don't think it's ok. As I discussed with you on Discord, can you please take the wasm-image plugin as an example?

@alabulei1
Copy link
Collaborator

Generally, you don't provide details in this guide.

@alabulei1
Copy link
Collaborator

Hi @mhmohona

Could you please check out the comments here?

Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
@mhmohona mhmohona requested a review from alabulei1 November 17, 2023 06:29
Signed-off-by: Mahfuza Humayra Mohona <[email protected]>

## Writing and Compiling the Plugin

To create a plugin, you need to define a function in C or C++, and declare it with the `extern "C"` keyword to ensure that the function is exported using C-compatible naming conventions. Then, compile the function as a shared library.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please provide some code sample.


## Registering the Plugin with WasmEdge Runtime

After compiling the shared library, you need to register the function with the WasmEdge runtime.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How to register the function? Please provide some details

docs/contribute/plugin/best_practice.md Show resolved Hide resolved

By following these steps, you can effectively run tests for the `wasmedge-image` plugin or any other WasmEdge plugin. Also if you want to develop your own tests follow [Writing Tests for WasmEdge Plugins](test_plugin.md) for details.

## Publishing the Plugin
Copy link
Collaborator

Choose a reason for hiding this comment

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

How to publish? If i want to contribute my code to the WasmEdge repo, what should I do?

@alabulei1
Copy link
Collaborator

Hi @mhmohona

As we discussed many times before, the guide you're writing now is too general to provide practical details for developers.

Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
docs/contribute/plugin/best_practice.md Outdated Show resolved Hide resolved
docs/contribute/plugin/best_practice.md Outdated Show resolved Hide resolved
docs/contribute/plugin/best_practice.md Outdated Show resolved Hide resolved
docs/contribute/plugin/best_practice.md Outdated Show resolved Hide resolved
docs/contribute/plugin/best_practice.md Show resolved Hide resolved
docs/contribute/plugin/best_practice.md Outdated Show resolved Hide resolved
docs/contribute/plugin/best_practice.md Outdated Show resolved Hide resolved
Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
Signed-off-by: Mahfuza Humayra Mohona <[email protected]>
@alabulei1 alabulei1 merged commit 6499fa3 into WasmEdge:main Mar 4, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants