[feat] implement a separate VmBuilder::build method for async cases

[apepkuss]

In this PR a separate build method is implemented in VmBuilder for creating a Vm with the support for AsyncWasiModule.

[juntao]

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

---

Overall, the pull request seems to be a set of minor updates, refactorings, and improvements to the code and related artifacts, such as test cases and GitHub workflows. However, there are some potential issues and errors that need to be addressed by the contributor and the maintainers before merging the changes.

One of the main issues is related to the lack of context and documentation about the changes and why they were made. This may create confusion for future maintainers and result in issues or regressions if the changes are incomplete or incorrect. Therefore, the contributor should provide more detailed explanations about the changes in the commit messages, pull request description, and code comments.

Another issue is related to the use of conditional compilation statements and how they affect the functionality and testing of the code. The contributor should make sure that all code paths and scenarios are covered by the test cases and that the code compiles correctly for different configurations and platforms.

There are also some specific issues identified in some patches, such as the potential data race in the new VmBuilder::build method, the unclear purpose of the exit_code() method, the removal of async_wasi_ctx for the Linux target, and the lack of testing for the code examples after the updates.

In summary, while the pull request includes many useful and valuable updates, the potential issues and errors in some patches require further investigation and clarification to ensure the correctness and stability of the code. The contributor and the reviewers should work together to address these issues before merging the changes to the main branch.

Details

Commit 4efcb0efe863b8c0aa5afdeae184ed605aaf8d3e

The patch exposes the WasiCtx struct in the wasmedge-sys crate and renames the parameter in async_wasi.rs from data to ctx. It also changes the create method in AsyncWasiModule to use WasiCtx instead of AsyncWasiCtx. The change itself seems fine and isn't expected to cause any issues. However, the commit message and the patch title don't provide enough context about why these changes were made, which could pose a problem for future maintainers trying to understand the reasoning behind the changes. Additionally, the patch modifies several files in one commit, which makes it harder to review specific changes. The patch would be easier to review if it were divided into smaller, more focused commits.

Commit c23c6f61224b887207517a6d69400eb8ba12fd7c

The patch adds a new public type called WasiCtx in the Rust SDK library's lib.rs file. It also adds a #[cfg(..)] attribute to conditionally expose it only if certain features and target operating systems are met.

The change seems straightforward, and there's no immediate problem with adding this type. It may be a part of a larger set of changes in the Pull Request that is not visible in this patch, so it's important to consider the context of the larger change. However, if this change is isolated, then it can be reviewed independently without issues.

Commit 0479e078ffacebad04ceb08192e54972f1d60176

This patch introduces a new VmBuilder::build method that separates the creation of a Vm instance for non-async (#[cfg(not(feature = "async"))]) cases from async (#[cfg(all(feature = "async", target_os = "linux"))]) cases. The patch adds a new parameter to the async VmBuilder::build method to receive a mutable reference to a WasiCtx argument.

The patch also includes new logic to load and register plugin instances and to create the AsyncWasiModule for async builds. It refactors the original implementation of VmBuilder::build to be used for non-async cases.

The most important change is the introduction of a new method to handle the creation of different types of Vm instances. This is important mainly because async and non-async cases require different logic to be implemented. Therefore, separating these two cases in different methods is a good practice to improve code organization.

One potential problem is that the new mutable reference parameter for the async build method might create a data race when used by different threads. Therefore, the WasiCtx argument must be used in a thread-safe context if it is going to be passed to this method.

Commit 766e8b1f22ea74254609ef03f004d761043d0663

This is a patch to update the test code in the executor.rs file. The changes include adding target_os to #[cfg] conditional compilation directives. The executor.rs file has test cases that validate the expected behavior of the runtime. The patch is updating these tests to include support for async operations and add support specifically for the Linux platform.

The change is well-defined and targets specific functionality. It has defined and isolated test cases to validate expected behavior. The use of conditional compilation directives is also a good practice to provide platform support.

However, without the context of the complete code changes in the pull request, it is difficult to determine if these test changes are complete and cover all edge cases. An issue may arise if there is any use of characteristic features where the tests may not cover the complete functionality of the code changes.

Commit a80ed9e95f0c5061a96034313d5bcb7e4c6e31bb

This is a patch that modifies several Rust examples in the Wasmer SDK. The changes include updating feature flags to only apply on Linux targets and reformatting the code.

There are no significant problems with the patch, and all of the changes are relatively minor improvements. However, the changes only affect the Rust examples and not the Wasmer runtime, so they may not be relevant to other parts of the project.

Commit ea61fd70622ed94928fd4223b802f815389f073b

The patch seems to be updating the test code in the Rust SDK. Specifically, it is adding a new conditional compilation check to exclude a block of code when the "async" feature is not enabled. The change only affects the compiler.rs file, with one insertion of a single line of code.

As a reviewer, I don't see any potential problems with this patch as it seems to be a small and straightforward update to existing code. The change is not critical, so it can be reviewed and merged quickly. However, it would be good to confirm with the contributor if there is any reason for excluding the code when the "async" feature is not enabled.

Commit 1762732b4af769c77b494978dc454dfe4c2f4fe0

Key changes: The patch updates the rustdoc and code formatting. It doesn't change the functionality of the library much, except for some conditional compilation statements. It uses #[cfg(not(feature = "async"))] to exclude some code blocks on async build.

Potential problems: None.

Overall, the changes are minor and do not affect any of the existing functionality of the library. The use of conditional compilation statements is a good practice to ensure that the code compiles for different use cases. Therefore, the code changes and the rustdoc update look good to me.

Commit cf81b36e264b7c76838a0c4a8cc7387f4b02a48e

This patch updates the pull request paths for the "bindings-rust" workflow in the ".github/workflows/bindings-rust.yml" file. It adds four new paths, "examples/", "src/", "crates/**", and ".github/workflows/bindings-rust.yml", to the existing path of ".github/workflows/bindings-rust.yml". This change ensures that the workflow will be triggered when changes are made to any of the specified paths. There are no potential problems with this patch.

Commit 21873d3914ddf224813990124cbcc0c26b613975

The patch introduces new methods to the WasiCtx structure in the async-wasi crate. The new methods are push_args, push_envs and push_preopen.

It seems that the changes are focused on improving the async-wasi implementation by making it easier to use predefined options instead of creating individual ones each time.

There don't seem to be any potential problems with the changes.

Commit 51435f87e004294d3b3b3493aaf340f5392402ba

The patch implements new APIs in the AsyncWasiModule of the wasmedge-sys crate. It creates a WasiCtx instance and initializes it with the values received as parameters, adds sync/async host functions to the module, and provides an implementation for the exit_code method. The changes seem to be well-contained and add useful functionality.

There are no potential problems identified in the patch. The differences in the implementation between the cases when wasi_ctx is Some and when it is None are well-defined and controlled by the feature async and the target operating system linux.

Commit c6db70d4af40033bfe4d32ee6b325642d12ae77d

The patch adds two new methods to the WasiInstance struct: initialize() and exit_code(). The initialize() method initializes the WASI host module with the given parameters, and exit_code() returns the WASI exit code. The patch is well organized and is properly documented with examples.

One potential issue is that the new methods are only available for the async feature and the linux target. This may limit their usefulness in certain scenarios. It is also not clear from the patch what the purpose of the exit_code() method is, and how it fits into the overall functionality of the WASI module.

Commit 552e78fbd04827a274524b18d9680cb96a90c23b

This patch refactors the VmBuilder::build method by removing the WasiCtx parameter and replacing it with None when creating a new AsyncWasiModule. This applies only when the async feature is enabled and the target is linux. The build method now returns a WasmEdgeResult<Vm>.

The key change is to replace the WasiCtx parameter with None when creating a new AsyncWasiModule.

There does not appear to be any potential problems with this patch as it is quite small and makes a small but necessary change.

Commit 9cc96731d6243b53995377a2865fdec2a05af07d

The patch updates two Rust code examples in the examples directory of the Rust SDK. It updates the VmBuilder::build method for creating a Vm instance, replacing None with an empty parameter as its argument. Potential problems are not visible from the patch alone. However, more testing is needed to ensure that the code examples work as expected after the updates.

Commit 8dd65b9e70b3433a79c4678e28f5d9d5c1caa9ff

This patch updates the .github/workflows/bindings-rust.yml file to include Rust 1.70 in the test matrix for different OSes. The key change is the addition of Rust 1.70 in the test matrix.

There don't seem to be any potential problems with this change, as it simply adds a new version of Rust to the test matrix. However, it's worth noting that if there are any issues specific to Rust 1.70, this patch may need to be updated or reverted.

Commit 43f2fd9006ad22b096d0f470e09917802ae3c7d7

This is a patch that updates the Rust version in the GitHub workflow file bindings-rust.yml. The current version of Rust 1.70, 1.69, 1.68 is being updated to 1.70.0, 1.69, 1.68. The patch seems to be a routine update and will likely not cause any issues.

Commit e4c8ff100bb4aea38e1802a9a599418c19ff87bb

Key changes:

• The test code in the executor module of the wasmedge-sys crate was updated.
• The async_wasi_ctx (an instance of an asynchronous WASI context) is no longer being created for the Linux target when the async feature is turned on.

Potential problems:

• It's not clear why the async_wasi_ctx is no longer created for the Linux target when the async feature is turned on. This might impact the test outcome and correctness of the wasmedge-sys crate on Linux systems. It should be investigated if this was intentional or just an oversight before merging this pull request.
• The commit title is not descriptive of the changes made in the patch, which makes it hard to understand why the test code was updated. The commit message should be modified to provide a clear explanation of what has changed and why.

[apepkuss]

@L-jasmine Could you please help review this PR? Thanks a lot!

[apepkuss]

Hi @hydai WasmEdge failed to build on Windows. It seems a lld-link error:

lld-link: error: undefined symbol: public: void __cdecl WasmEdge::Host::WASI::FindHolderBase<class WasmEdge::Host::WASI::FindHolder>::reset(void)

I'm not sure if it is a known error on Windows, so please check it again: https://github.com/WasmEdge/wasmedge-rust-sdk/actions/runs/5240851644/jobs/9462187704#step:6:407

[apepkuss]

@L-jasmine Thanks for the review!