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

Update rustdocs #55

Merged
merged 17 commits into from
Aug 17, 2023
Merged

Update rustdocs #55

merged 17 commits into from
Aug 17, 2023

Conversation

apepkuss
Copy link
Collaborator

In this PR, the rustdocs are updated to show the features which enable the relevant APIs.

Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Copy link
Member

juntao commented Aug 17, 2023

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


Summary:

Overall, the pull request titled "Update rustdocs" includes several patches that update the documentation and metadata of the project. Most of the changes appear to be straightforward and without any potential problems. However, there are a few areas that require further clarification and justification.

The most important findings in this review include:

  1. The purpose of the --cfg "docsrs" flag added to the rustdoc-args field in Cargo.toml is unclear. More context and information are needed to understand the motivation behind this change.

  2. The conditional compilation attributes (#[cfg_attr(docsrs, feature(doc_cfg))] and #[cfg_attr(docsrs, doc(cfg(...)))]) added for documentation purposes need to be properly explained and documented to maintain clarity for future developers.

  3. The change in the required Rust version in the README.md file could potentially break compatibility with older versions of Rust. It is important to verify that all other dependencies are compatible with the new requirement.

  4. The move of the async module into the wasi module should be documented and justified with an appropriate comment or commit message. The deletion of the src/async.rs file without explanation or migration plan may cause confusion for other contributors.

  5. The duplication of the build method in the VmBuilder struct should be consolidated into a single implementation, considering the features and target OS. The code formatting in the patch is inconsistent and should be standardized. It is also important to clarify if the WasiContext is required for all builds or only for async builds on Linux.

In conclusion, while most of the changes in the pull request are minor updates to the documentation and metadata, there are a few areas that need further clarification and justification. Overall, additional context, comments, and documentation would greatly enhance the understanding and review of these patches.

Details

Commit aa186b3a07901e64cdacdbec6da5fe8ce783a3eb

Key changes:

Potential problems:

  • There don't seem to be any potential problems with this patch. It is a simple update to the repository URL, which seems to be correct.

Commit 2a2d6b95c658c1fb32af432dd9c7e7f9ab37860a

Key changes in this patch include:

  • The table format in the README.md file has been updated to include additional information about supported architectures and linking types.
  • The formatting and alignment of the table in the lib.rs file has been improved.

Potential problems:

  • There doesn't appear to be any potential problems with this patch. It seems to be a straightforward update to the documentation.

Commit b572436c426724b9e502ecc68736b11ce250e938

Key changes:

  • Added [package.metadata.docs.rs] section to Cargo.toml.
  • Added features field with a list of features.

Potential problems:

  • There doesn't seem to be any obvious problems with the patch. It simply adds metadata to Cargo.toml to specify additional features for use with docs.rs documentation generation. However, without further context, it is difficult to determine if this change is necessary or if there are any potential issues with it.

Commit 009c7812a93c764677882a393a299ed930dbd15d

Key changes:

  • Added docs.rs metadata to the Cargo.toml file under [package.metadata.docs.rs]

Potential problems:

  • None identified

Overall, the patch adds docs.rs metadata to the project's Cargo.toml file. No potential problems were identified.

Commit 97a231c1bc70c9b36c1cf0ed2ddd30301046366c

Key changes:

  • Added the --cfg flag with the "docsrs" parameter to the rustdoc-args field in Cargo.toml.

Potential problems:

  • It's unclear why the --cfg "docsrs" flag is being added to the rustdoc-args field. More context is needed to understand the purpose of this change and whether it is necessary.
  • It would be helpful to have a commit message that provides more information about the motivation behind this change.

Overall, this patch is quite small and it's difficult to fully evaluate without more context. I recommend discussing the purpose of adding the --cfg "docsrs" flag and providing additional details in the commit message.

Commit c2ef84c5250b0897043f20043116fd5798011114

Key changes:

  • Added a new module async, which defines async related types.
  • Added a new function call_func_async to the Executor implementation, which is only available when the async feature is enabled and the target OS is Linux.
  • Added #![cfg_attr(docsrs, feature(doc_cfg))] attribute to enable conditional documentation compilation for different features.
  • Added #[cfg_attr(docsrs, doc(cfg(feature = "async")))] attribute to conditionally document the async feature.
  • Removed #[doc(hidden)] attribute from the ast_module module.
  • Added #[cfg_attr(docsrs, doc(cfg(feature = "aot")))] attribute to conditionally document the aot feature.
  • Added #[cfg_attr(docsrs, doc(cfg(feature = "async")))] attribute to conditionally document the async feature.
  • Added #[cfg(not(feature = "async"))] attribute to conditionally document the async feature for the WasiModule type.

Potential problems:

  • It is not clear why the call_func_async function is only available for the async feature and the Linux target OS. This decision needs to be justified and documented.
  • The added attributes for conditional documentation compilation (#[cfg_attr(docsrs, feature(doc_cfg))] and #[cfg_attr(docsrs, doc(cfg(...)))]) should be properly explained and documented to ensure clarity for future developers.

Commit b2fdc5b914ef208f380af175bf831c308be9948d

Key changes:

  • The README.md file was updated to indicate that the minimum required Rust version for wasmedge-sys is now 1.69 instead of 1.68.
  • Several functions in the async/module.rs file have been changed from pub to fn.

Potential problems:

  • The change in the required Rust version might break compatibility with older versions of Rust. Verify that all other dependencies are compatible with the new requirement.
  • The change from pub to fn for the functions in async/module.rs might indicate that these functions were mistakenly made public when they should not have been. Review the intended visibility of these functions and ensure that they are being used correctly.

Commit 4370f3115b047b921bbba4eedb9e01134dcddf4c

Key Changes:

  • Added a comment for the create_with_custom_wrapper function, stating that the caller should guarantee the life cycle of both the real_fn and the data object.
  • Added a comment for the call function, explaining the arguments and potential error.

Potential Problems:

  • No potential problems were found in this patch. The added comments provide useful information to the users of these functions.

Commit c5546ee82660f2dd0fa5c2bb8741e55feb17df5e

Key changes:

  • Added a new line to the [package.metadata.docs.rs] section in the Cargo.toml file.
  • Added a rustdoc-args field with the value ["--cfg", "docsrs"].

Potential problems:

  • There doesn't appear to be any problems with this change. It seems to be a minor update to the documentation configuration.
  • However, without more context, it is difficult to fully understand the impact of this change. It would be helpful to have a description of why this change was made and what specific purpose it serves.

Commit 95ec21d3933e34239dbc415206f331d4f63c4d75

The key changes in this patch are as follows:

  • Added #[cfg_attr(docsrs, doc(cfg(feature = "aot")))] attribute to documentation for the Config struct and its methods.
  • Added #[cfg_attr(docsrs, doc(cfg(feature = "aot")))] attribute to documentation for the CompilerConfigOptions struct and its methods.
  • Added #[cfg_attr(docsrs, doc(cfg(all(feature = "async", target_os = "linux"))))] attribute to documentation for the Executor struct and its methods.
  • Added #[cfg_attr(docsrs, doc(cfg(all(feature = "async", target_os = "linux"))))] attribute to documentation for the Func struct and its methods.
  • Added #[cfg_attr(docsrs, doc(cfg(all(feature = "async", target_os = "linux"))))] attribute to documentation for the FuncRef struct and its methods.
  • Added #[cfg_attr(docsrs, doc(cfg(all(feature = "async", target_os = "linux"))))] attribute to documentation for the ImportObjectBuilder struct and its methods.
  • Added #[cfg_attr(docsrs, doc(cfg(feature = "ffi")))] attribute to documentation for the ImportObject struct and its methods.
  • Added #![cfg_attr(docsrs, feature(doc_cfg))] attribute at the top of the file to enable conditional documentation based on feature flags.

Potential problems with this patch:

  • None of the changes are functionally significant. They are all related to documentation attributes and will not affect the behavior of the code.
  • The code appears to be properly formatted and consistent with the surrounding codebase.

Overall, this patch seems to be a minor update to the documentation in the codebase.

Commit 0bddd88a3201536da048a7077579b413161af99c

Key changes:

  • The async module has been refactored and moved into the wasi module.
  • The src/async.rs file has been deleted.
  • Some imports have been modified in src/executor.rs and src/externals/function.rs to refer to the async module within the wasi module.
  • The src/wasi.rs file has been modified to conditionally include an async module for Linux platforms.

Potential problems:

  • It is not clear why the async module was moved into the wasi module. This change should be documented with an appropriate comment or commit message.
  • The deletion of the src/async.rs file without any explanation or migration plan may cause confusion for other contributors.
  • The changes in the import statements should be reviewed to ensure they are correct and do not introduce any compilation errors.
  • The conditional inclusion of the async module in src/wasi.rs needs to be carefully evaluated to ensure it is correct and does not break any functionality.

Commit 56677152b1988e2080ce32cbc0e865bc5de5f36c

Key Changes:

  • The patch adds a new line of code in the src/io.rs file.
  • The added line is a lint suppression directive for the unused_mut warning.

Potential Problems:

  • It's important to review why the lint warning for unused_mut is being suppressed. The reason should be documented or explained in a code comment. Without proper justification, suppressing warnings can lead to overlooked issues or code quality problems.

General Feedback:

  • The code change itself seems straightforward and only involves adding the lint suppression directive.
  • However, it would be beneficial to have more context or information about the motivation behind this change. Providing additional information in the pull request description or comments would help reviewers better understand the intention.

Commit b0ee8115b08f39c3ce534f2026a00de755ef8af6

Key changes:

  • The import statement for WasiInstance has been updated in the src/vm.rs file.
  • The with_wasi_context method has been added to the VmBuilder struct.
  • The build method has been moved to a new private method named build_vm in the VmBuilder struct.

Potential problems:

  • It seems that the build method is duplicated in the VmBuilder struct. One version is for non-async builds and the other is for async builds. They should be consolidated into a single implementation, taking into account the features and target OS.
  • The code formatting in the patch is inconsistent, with some lines indented with spaces and others with tabs. This should be standardized.
  • It's unclear if the WasiContext is required for all builds or only for async builds on Linux. This could affect the behavior of the build method and how it is used in the codebase.
  • It would be helpful to have some context or comments explaining the purpose and impact of these changes. This would make the code easier to understand and review.

Commit 554f4761c07547535efe3e687cfbe1613bed154f

Key changes:

  • The patch updates the release workflow for the wasmedge-sdk repository.
  • It updates the command for building the API documentation to include the --cfg docsrs flag, which sets the docsrs configuration option for RUSTDOCFLAGS.
  • The target directory for the documentation is changed to ./target.

Potential problems:

  • The --cfg docsrs flag is used in both the Build API document and Build Async API document steps. It's unclear why this flag is necessary and what its purpose is. It would be helpful to document this in the comments or provide an explanation in the pull request description.
  • The RUSTDOCFLAGS="--cfg docsrs" command is used in both the Build API document and Build Async API document steps. It's possible that this command should only be used in one of the steps, depending on the specific requirements of the project. If it needs to be used in both steps, it would be helpful to explain why in the comments or pull request description.

Overall, the changes appear to be focused on updating the release workflow and improving the documentation generation process. However, it would be beneficial to provide more context and explanation for the changes in the pull request description.

Commit d89053b5fcc568047b8334010fbd5b19b033fb9e

Key Changes:

  • Added a new workflow file: .github/workflows/release-async-api-docs.yml.
  • Added a workflow to publish async API documentation for the wasmedge-sdk crate.
  • The workflow runs on an Ubuntu 22.04 environment using the wasmedge/wasmedge:ubuntu-build-clang container.
  • The workflow includes steps to set up the build environment, install WasmEdge, install Rust nightly, build async API documentation, and deploy the documentation to the gh-pages branch.

Potential Problems:

  • It's not clear if the workflow is correctly configured. The concurrency section is present, but without knowledge of the project's specific requirements, it's difficult to determine if the configuration is appropriate.
  • The workflow uses a specific container image wasmedge/wasmedge:ubuntu-build-clang for the job. Ensure that this image contains the necessary dependencies for the build process.
  • The workflow installs the llvm-15-dev package, but it's not mentioned why this specific version is required. Verify if it's the correct version for the project.
  • The workflow installs Rust nightly using dtolnay/rust-toolchain@nightly. Consider whether it's necessary to use the nightly version, or if a stable version would be sufficient.
  • The step to deploy the async API documentation to gh-pages is conditionally executed only if the branch is main. Ensure that this condition aligns with the project's branch naming conventions.
  • Confirm that the wasmedge-rust-sdk directory exists and is correctly checked out in the workflow steps.
  • Verify that the RUSTDOCFLAGS environment variable being set is necessary for generating the desired documentation.

Overall, the key changes appear to introduce a new workflow for publishing async API documentation. However, further review and testing are required to ensure the configuration and steps are correct for the project.

Commit 0af2a3dcb26b48a4aa73768b15fa62dedd3d3cf5

Key changes:

  • The comments for the exports and get_export methods in the Module implementation have been updated to provide more detailed descriptions.

Potential problems:

  • No potential problems are identified in this patch. The changes are straightforward and do not introduce any functionality issues or code errors.

Commit 30210c571fedb4c2885fed56a0f937155a58e2e4

Key Changes:

  • The RUSTDOCFLAGS environment variable has been added to the cargo doc command in the "Build API document" step.
  • The --features async flag has been removed from the cargo doc command.

Potential Problems:

  • The --features aot feature flag has been added to the cargo doc command, but it was previously present before the update. Make sure that this change is intentional and that it doesn't cause any issues with the API documentation.
  • The --features async flag has been removed from the cargo doc command. Verify if removing this flag won't affect the functionality of the API documentation and if it was intended.

Overall, the changes seem simple and focused on the build workflow for the API documentation. However, it is important to double-check the changes related to feature flags to ensure they are correct and intentional.

@apepkuss apepkuss requested a review from L-jasmine August 17, 2023 07:30
@apepkuss
Copy link
Collaborator Author

@L-jasmine Thanks for the review!

@apepkuss apepkuss merged commit eca1ca4 into WasmEdge:main Aug 17, 2023
@apepkuss apepkuss deleted the doc/update-rustdocs branch August 17, 2023 09:44
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.

3 participants