support rust async && support https client

[L-jasmine]

No description provided.

[juntao]

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

---

Overall Summary:

The pull request includes several key changes, such as support for Rust async and support for HTTPS client. There are potential issues and errors that need to be addressed.

In the first pull request, there are potential problems related to the renaming of the "socket" function, modification of the Cargo.toml file, and updates to the examples and modules. The changes to multiple files and the commented-out code in the examples should also be reviewed.

In the second pull request, potential problems arise from the bumped version of libquickjs.a and the removal of constants in the binding.rs file. More context and explanations for these changes would be helpful.

The third pull request introduces potential problems with changes to multiple files, the large number of insertions and deletions, and the introduction of a new file "src/quickjs_sys/js_promise.rs". The updated and added dependencies should be reviewed, along with the purpose of the new "aho-corasick" dependency.

The fourth pull request raises concerns about the absolute path in the .cargo/config.toml file, the use of "println!" statements for debugging purposes, and the presence of commented-out sections in the code.

In the fifth pull request, issues arise from the use of "println!" statements, modifications to the fread and fwrite functions, and the lack of support for Rust async or HTTPS client as mentioned in the title.

Finally, the sixth pull request sees potential problems due to the absence of tests for the new HTTPS functionality, changes to multiple files, the removal of src/event_loop/poll.rs, changes to the Cargo.toml and Cargo.lock files, and conditional changes related to the "tls" feature.

Overall, there are common themes among the pull requests, such as the lack of sufficient explanations, potential issues with dependencies, and the need for proper testing. It would be beneficial to address these issues and provide more context to improve the review process and ensure the quality of the code changes.

Details

Commit 7547923419e957a8a5ad0b2f6207b1f467d0252f

Key changes:

• Renamed the "socket" function to "wasi_socket".
• Updated dependencies in the Cargo.toml file.
• Updated the examples and modules to use the new "wasi_sock" module for networking.

Potential problems:

• The patch includes a lot of line deletions and insertions, which might make it difficult to review the specific changes.
• It is unclear why the "socket" function was renamed to "wasi_socket". The Pull Request description or commit message should provide more context on this change.
• The patch modifies the Cargo.toml file, adding new dependencies and updating existing dependencies. These changes should be reviewed to ensure they are necessary and do not introduce any compatibility or security issues.
• The examples and modules have been updated to use the new "wasi_ssh" module for networking. This change should be reviewed to ensure that it is implemented correctly and does not introduce any regressions or compatibility issues.
• The patch includes changes to multiple files. It would be helpful if the Pull Request description or commit message provided a high-level overview of the changes and their purpose.
• The patch includes changes to example files, but the commented out code in the examples suggests that the examples may not be updated correctly. This should be confirmed and fixed if necessary.
• The patch includes changes to the "src/event_loop/mod.rs" file, but the specific changes to this file are not clear from the diff. Code review should be done on this file to ensure that the modifications are correct and do not introduce any bugs or issues.
• The patch includes changes to the Cargo.lock file, which suggests that dependencies have been updated or added. These changes should be reviewed to ensure that they are necessary and do not introduce any compatibility or security issues.

Commit 6309458867ca7357999ae58417fda8c95c07ace8

Key Changes:

• Bumped libquickjs.a version
• Removed a large number of constants from the binding.rs file

Potential Problems:

• It is not clear why the libquickjs.a version was bumped. Some information explaining the reason for this change would be helpful.
• There is a significant amount of code removal in the binding.rs file. It is important to verify that these removed constants are no longer needed and will not cause any issues in the codebase.

Overall, it would be beneficial to have more context and explanation for the changes made in this patch.

Commit 8590011a7c945f951d22b1216eba656b8b51a9e4

Key changes:

• Added support for rust async
• Added support for HTTPS client
• Updated several dependencies to newer versions

Potential problems:

• The patch includes changes to multiple files. It would be helpful to have more detailed commit messages or comments explaining the changes in each file.
• The large number of insertions and deletions suggests significant changes to the codebase. It's important to ensure that these changes are properly tested and reviewed for any potential bugs or regressions.
• It's unclear if the changes have been tested and if there are any new test cases to cover the added functionality.
• The patch introduces a new file src/quickjs_sys/js_promise.rs. It would be helpful to review the implementation of this file to ensure it meets the project's coding standards and requirements.
• The patch updates several dependencies. It's important to verify that these updates are compatible with the existing codebase and do not introduce any compatibility issues or breaking changes.
• The patch introduces a new dependency aho-corasick. It's important to review the purpose and usage of this dependency to ensure it aligns with the project's requirements.

Additional Review Comments:

• It would be helpful to have a more detailed commit message or PR description explaining the motivation behind these changes and how they improve the project. This will provide better context for reviewers and future maintainers.
• Consider breaking the changes into smaller, more focused PRs if possible. This will make the review process more manageable and focused on specific areas of the codebase.

Commit 41c92700ee5450d5f1f0f53cea1ae700d986732c

Key changes:

• Added println! statements for debugging purposes.
• Changed the target in the .cargo/config.toml file from wasm32-wasi to /home/csh/WasmEdge/build/tools/wasmedge/wasmedge --dir=.:..

Potential problems:

1. The .cargo/config.toml file changes the target to a specific path. It is unusual to include an absolute path in the configuration file. This might make the code harder to run or use in different environments.
2. The addition of println! statements is helpful for debugging, but they should not be included in a production-ready codebase. It's important to remove or comment out these statements before merging the code.
3. There are commented-out sections in the code, such as the main function and parts of the async_run_with_context implementation. These sections should be reviewed to ensure they are not relevant to the changes being made.

Overall, the patch contains changes that are mainly related to debugging and target configuration. It would be beneficial to address the potential problems mentioned above before merging the code.

Commit ebad9e7487507f9d7e05676772db98c46c265afc

Key Changes:

• In src/internal_module/core.rs, the println! statements have been commented out.
• In src/internal_module/fs.rs, the fread and fwrite functions have been modified to run asynchronously using the event loop.
• The calls to println! in the fread function have been added.

Potential Problems:

• The println! statements in src/internal_module/fs.rs should not be present in the production code. They should be removed or replaced with proper logging statements.
• The use of println! statements for debugging purposes is a potential source of performance issues and slowdowns. These statements should not be present in the production code.
• The modified fread and fwrite functions introduce asynchronous behavior, which may introduce new bugs or race conditions. These changes should be thoroughly tested to ensure correctness.
• The code does not appear to have any new support for Rust async or HTTPS client as mentioned in the PR title.

Commit e0c237ec7379e49d8c3240cdd2f3ab16876f18bb

Key Changes:

• Added support for HTTPS in http.fetch module.
• Added a new JavaScript module example_js/wasi_https_fetch.js that demonstrates fetching HTTPS resources.
• Added a new Rust crate dependency wasmedge_rustls_api for handling TLS connections.
• Modified the http.fetch module to use net.WasiTlsConn for HTTPS connections.

Potential Problems:

• The patch does not include any tests for the new HTTPS functionality. It would be good to add tests to ensure proper functionality and prevent regressions.
• The patch includes changes to multiple files, which could make it more difficult to review and understand the complete impact of the changes.
• The removal of src/event_loop/poll.rs could potentially cause issues if it is still being used elsewhere in the codebase. It would be good to double-check if this file is no longer needed or if its functionality has been replaced with something else.
• The patch includes changes to the Cargo.toml and Cargo.lock files, adding new dependencies. It would be good to verify if these dependencies are necessary and if they are compatible with the existing codebase.
• It seems that the patch is adding support for TLS connections but is only conditional on the tls feature being enabled. It would be good to verify if this feature is properly enabled and tested.
• The use of net.WasiTlsConn in the http.fetch module is not directly explained in the patch description. It would be good to provide more context and explanation for the changes made.

Overall, the patch introduces HTTPS support and adds a new dependency for handling TLS connections. However, it would be good to address the potential problems mentioned above before merging the changes.

Commit e724d981ad3ebe14425d7ca3eb8bd1d8c69cf726

Key Changes:

• Updated the version of WasmEdge from 0.11.2 to 0.13.3.
• Updated the download links for WasmEdge and the Rustls plugin.
• Added a step to unzip the Rustls plugin and move it to the appropriate directory.
• Added a new job named "Https fetch example" to build and run a Wasmedge example using the wasi_https_fetch.js script.

Potential Problems:

1. The patch updates the version of WasmEdge without providing any information about why this change is necessary. It's unclear if this change might introduce compatibility issues or other problems.
2. There is a mismatch between the version mentioned in the download link for the WasmEdge plugin (0.12.0-alpha.1) and the updated version of WasmEdge (0.13.3). This might be an error or oversight.
3. It's unclear why the Rustls plugin is being downloaded and installed. The purpose and impact of this change should be documented.
4. The patch moves the Rustls plugin to /usr/local/lib/wasmedge/, but it's unclear if this is the correct location or if any additional configuration steps are required.
5. The Https fetch example job is added without any explanation of its purpose or expected behavior. Additional details should be provided.
6. The timeout-minutes for the Node fs module test job is changed from 10 to 15, but there is no explanation for this change. The reason for the increased timeout should be clarified.

Commit b38cf2cae2db76d60831dd7926d8c0dfe40b6e36

Key changes:

• The runner command in the .cargo/config.toml file has been modified. The absolute path to the wasmedge executable has been replaced with a relative path.

Potential problems:

• It is unclear whether the relative path wasmedge will resolve correctly when the command is executed. This may cause issues if the executable is not in the expected location or if the working directory is different.
• The change does not provide any explanation as to why the absolute path was replaced with a relative one. It would be helpful to include this information for better understanding.

Overall, the change seems minor, but it is important to ensure that the relative path resolves correctly and verify the reason behind the change.

Commit 74d2016ac8abeda156c0696ec7d19b90b96469db

Key changes:

• Updated the URLs for downloading WasmEdge and plugins to use version 0.13.3.
• Added a new URL to download the WasmEdge TensorFlow dependencies.
• Added commands to extract and remove the downloaded tar.gz files.
• Added commands to move the extracted files to the appropriate locations.
• Added sudo before the mkdir command.

Potential problems:

• The patch includes changes to download and install WasmEdge and its plugins. The URLs for downloading the plugins and TensorFlow dependencies have been updated, but it's important to verify if these URLs are correct and up-to-date.
• The patch uses the sudo command to perform some operations. It is recommended to avoid using sudo in GitHub Actions workflows because it runs all actions as the root user. This can have unintended consequences and is generally not recommended.
• There is a commented out section related to building and running a TensorFlow example. It is unclear why it is commented out and whether it should be included or removed. This should be clarified and addressed.

Commit 8e3920075a11b4d5e2fe0d90a0763fba394b10f1

Key Changes:

• Increased the timeout for cargo test in the CI workflow from 15 minutes to 60 minutes.
• Updated the WasmEdge version to 0.13.4 in the CI workflow installation script.
• Updated the download link for the WasmEdge Rustls plugin in the CI workflow installation script.
• Removed unnecessary file movements in the CI workflow installation script.

Potential Problems:

• The code change in src/internal_module/fs.rs seems unrelated to the pull request title "support rust async && support https client". It might be accidentally included in this patch. It should be reviewed separately.
• The CI workflow installation script could be improved by removing unnecessary curl and wget commands and simplifying the installation process.
• The use of sudo in the installation script could be a security concern. It should be reviewed if it's necessary or if there's a better alternative.
• The increased timeout for cargo test suggests that the tests might be resource-intensive or require more time to complete. It could be worth investigating if there's a way to optimize or improve the test suite to reduce the need for a longer timeout.

Commit 4afed4460a0db22be1b16d2bbfc83d328e3cefae

Key Changes:

• The patch updates the CI workflow file .github/workflows/examples.yml.
• It replaces the hardcoded version number 0.13.4 with the variable $VERSION.
• The change is made in the wget command, where the version number is used to download a specific file.

Potential problems:

• The patch does not provide any context regarding the reason for the change or any potential issues that were addressed.
• The patch assumes that the $VERSION environment variable is already defined and contains a valid version number.
• It is not clear how the $VERSION variable is intended to be set or where it should be defined.
• There is a risk of introducing errors if the $VERSION variable is not properly handled or if it contains an incorrect value.

Suggestions:

• Add a clear description in the patch or the accompanying PR explaining the motivation behind the change and any problems it aims to solve.
• Ensure that the $VERSION variable is consistently defined and set properly before using it in the CI workflow.
• Consider adding validation or error handling to ensure that the wget command only runs if $VERSION contains a valid version number.
• Update any relevant documentation or instructions to reflect the change in using the $VERSION variable.

[juntao]

Thanks. Only a few minor comments in the code.

[juntao]

Can we use $VERSION instead of the hardcoded version string here?

[chenhdc]

@L-jasmine hi，I have a question, how to compile QuickJs to libquickjs.a? use wasi-sdk-20? Looking forward to your reply, thanks