WasmEdge / wasmedge-rust-sdk

Embed WasmEdge functions in a Rust host app
Apache License 2.0
29 stars 15 forks source link

[CI] Update `rust-static-lib` workflow #96

Closed apepkuss closed 10 months ago

apepkuss commented 10 months ago

In this PR, the rust-static-lib workflow is updated to match the latest codebase and fix #92.

juntao commented 10 months ago

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


Overall Summary: The patch primarily consists of updating dependencies and making improvements to the workflow. However, there are a few potential issues and errors that need to be addressed.

Potential problems:

  1. Lack of clear documentation and explanations in the commit message and pull request description. It would be beneficial to provide more context and justification for the changes.
  2. Removal of environment variable assignments (WASMEDGE_DIR and WASMEDGE_BUILD_DIR) may cause issues if they were being used in subsequent steps.
  3. Removal of Rustfmt and Clippy steps may impact code quality and adherence to style guidelines. The reasons behind this change should be communicated.
  4. Revised test commands and addition of flags (--features static and additional features) require documentation and explanations for their purpose and impact.

Important Findings:

  1. The removal of the libboost-all-dev dependency without clear justification could result in build failures if the code relies on it.
  2. The addition of the --features static flag should be verified to ensure correct implementation and testing.

Overall, the patch needs better documentation and clarification to ensure that all changes are well-documented and justified. The potential issues and errors, such as removal of important dependencies and lack of explanations for certain changes, need to be addressed.

Details

Commit e9a9e60ddf4fb5b9bedd4b650ae1b830c347e8da

Key changes in the patch:

  1. Updated the Docker image from wasmedge/wasmedge:latest to wasmedge/wasmedge:ubuntu-build-clang.
  2. Added a new step to checkout the WasmEdge Rust SDK.
  3. Removed the commented out build step for WasmEdge.
  4. Removed the working directory specification for rustfmt and clippy steps.
  5. Removed the environment variable assignments for WASMEDGE_DIR and WASMEDGE_BUILD_DIR in the clippy and first Test Rust SDK steps.
  6. Added a new Test Rust SDK with async feature step.

Potential problems:

  1. The purpose of the change is not clearly mentioned in the commit message. It would be helpful to provide more context on why these updates are needed.
  2. The removed environment variable assignments (WASMEDGE_DIR and WASMEDGE_BUILD_DIR) could have been used in subsequent steps, and their removal may cause issues.
  3. It's unclear why the Test Rust Bindings step was renamed to Test Rust SDK.
  4. The changes in the Test Rust SDK step may require additional explanations or justification.

Overall, the patch seems to mostly update dependencies and improve the workflow. However, better documentation and clearer explanations of the changes would be beneficial.

Commit 32765949da1212329480c52a61912a34e22a41ca

Key changes:

Potential problems:

Additional notes:

Commit cda8174fefad4cad1d7a7797973ccdb2c72d780e

Key changes:

Potential problems:

Commit bf47dc6467b9fc82267de780184b641b6b6dde9c

Key changes:

Potential problems:

Commit 6cd58a3e88e0cfbec20433c5677b5e1d24468bd5

Key changes:

Potential problems:

Overall, the key change is the removal of the libboost-all-dev dependency, which may cause issues if the code relies on it. The reasoning behind this change should be clarified in the pull request.

apepkuss commented 10 months ago

@hydai Please help review this PR. Thanks a lot!

apepkuss commented 10 months ago

@hydai The review issue has been fixed. Please help review the updated. Thanks!

apepkuss commented 10 months ago

@hydai Thanks for the review!