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 support for standalone static libraries #22

Merged
merged 2 commits into from
Jul 19, 2023

Conversation

jprendes
Copy link
Contributor

WasmEdge started releasing static libraries WasmEdge/WasmEdge#2660
The wasmedge-sys crate support a standalone feature when using shared libraries.
This PR adds support for the standalone feature with static libraries.

Copy link
Member

juntao commented Jul 18, 2023

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 adds support for standalone static libraries and fixes Clippy issues introduced with a new nightly version. There are potential problems that need to be addressed, including unclear conditional compilation directives, unused extracted files, strange syntax for variable interpolation, and platform dependency on invoking the wget command. The changes also introduce changes to files in the crates/async-wasi and crates/wasmedge-sys directories, which may have unintended consequences. Further clarification and testing are needed to ensure the correctness and robustness of the implementation.

Details

Commit 68dbfb6da0f850f530ace2186caac787af02c52c

Key changes:

  1. Added support for standalone static libraries in wasmedge-sys/build.rs.
  2. Added conditional compilation based on the features standalone and target_family.
  3. Updated the WASMEDGE_RELEASE_VERSION constant to "0.13.1".

Potential problems:

  1. The conditional compilation directives for the install_libwasmedge function are inside the find_wasmedge function. It may be better to move them outside or separate them into separate functions for better organization.
  2. The install_libwasmedge function seems to download and extract a tarball, but the extracted files are not used anywhere in the code. It is unclear how these extracted files are related to the support for standalone static libraries.
  3. The println! statements in the install_libwasmedge function are using a strange syntax for variable interpolation ({arch}, {out_dir}, {url}). This could be a formatting error and should be fixed.
  4. In the install_libwasmedge function, the wget command is invoked using /bin/bash directly. This assumes that the wget command is available and on the system's $PATH. This might not be the case on all systems, and a more platform-independent solution should be used.

Overall, the changes need further clarification and testing. The review of this patch should focus on resolving these issues and ensuring that the implementation of support for standalone static libraries is correct and robust.

Commit 12b6397a72a1c83994cb937fcbd22fe733bb7d18

Key Changes:

  • The patch fixes Clippy issues that were introduced with a new nightly version.
  • Changes were made to several files in the crates/async-wasi and crates/wasmedge-sys directories.

Potential Problems:

  • It is unclear what specific Clippy issues were fixed with this patch.
  • The changes made to the files in the crates/async-wasi and crates/wasmedge-sys directories may introduce new bugs or break existing functionality.
  • It would be helpful to have more information about the rationale behind these changes and any associated tests.

@hydai hydai requested a review from apepkuss July 19, 2023 06:49
@hydai
Copy link
Member

hydai commented Jul 19, 2023

Hi @apepkuss
Could you please take a look at this PR?

@apepkuss
Copy link
Collaborator

@jprendes Please check the results of the CI workflows to fix the Clippy issues. Thanks!

@apepkuss
Copy link
Collaborator

@jprendes You can ignore the failure of the CI workflow for Windows.

@jprendes
Copy link
Contributor Author

Looks like the clippy issues were intruduced by a new nighly version of clippy.
Should be fixed now.

@apepkuss
Copy link
Collaborator

@jprendes There are still some failures in the CI workflows, but they are not caused by this PR. We'll fix them in another PR. So I'll approve this PR first. Thanks a lot!

@apepkuss apepkuss merged commit 598a700 into WasmEdge:main Jul 19, 2023
@jprendes jprendes deleted the static-standaline branch July 20, 2023 15:24
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.

4 participants