Add options to allow generating statically linked binaries

[jprendes]

It is currently not possible to generate statically linked binaries since dependent libraries are dynamically linked (this is the default behaviour of rustc-link-lib).

Moreover, rust-bindgen fails to run in alpine when the runtime feature is enabled, and fails to run with static or dynamic linking due to issues with crt-static. See rust-lang/rust-bindgen#2333 (comment).

This PR add:

• Options to specify the type of linking required for the different dependencies using environment variables.
• Adds an option to use an external rust-bindgen using environment variables.
• Adds support for musl libc following Upload prebuilt static libraries on Alpine in the 0.13.x release series. WasmEdge#2711.

[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 introduces several key changes, including adding options to generate statically linked binaries, adding a function to handle linking dependencies, adding a struct to handle environment variables, adding support for musl libc, and fixing clippy checks failing with a nightly version.

There are several potential problems and errors to address. In the link_lib() function, there is no error handling for missing or incorrect environment variables. In the Env struct, the read() method has an incorrect rerun-if-env-changed directive. In the do_http_request() function, there is a mistake in checking the WASMEDGE_STANDALONE_PROXY environment variable name. Additionally, there are assumptions made about the presence and validity of certain environment variables related to the build process without error handling or validation. The handling of unsupported combinations of OS, architecture, and libc is not clear. Lastly, there are inconsistent formatting styles for string concatenation.

The addition of support for musl libc lacks context or explanation, and it would be valuable to provide more information or comments. The changes related to clippy checks also lack context and do not include tests.

The changes related to documenting build environment variables seem to be helpful, but the removal of versioning information in the wasmedge-sys crate's README.md file may require further investigation.

In summary, the pull request provides the requested functionality but should address the potential problems and errors. The changes could benefit from more context, explanation, tests, and consistent formatting.

Details

Commit 48e2225083d7c74b9ece7cad504699a4cecab070

Key Changes:

• Added options to allow generating statically linked binaries.
• Added a new function link_lib() to handle linking dependencies.
• Added a new struct Env to handle environment variables and added a method read() to read the value of the environment variable.

Potential Problems:

• In the link_lib() function, there is no error handling for getting the link type and library path from environment variables. If any of these variables are not set, the program will panic.
• In the Env struct, in the read() method, a rerun-if-env-changed directive is added to the build script. However, this directive should only be added for non-build related environment variables, but it is currently added for all variables except OUT_DIR and those starting with CARGO_.
• In the do_http_request() function, the check for the WASMEDGE_STANDALONE_PROXY environment variable is done using Env("STANDALONE_PROXY"). This won't work as it should be Env("WASMEDGE_STANDALONE_PROXY").

Commit ede4670c5dd776a5b2b9cdcd633b43b7fd0d22ca

Key changes:

• Added support for musl libc.
• Refactored the code to accommodate the new libc option in the build process.

Potential problems:

• The patch introduces support for musl libc, but there is no mention of why this is necessary or any explanation for why it was added. It would be helpful to have more context or an explanation in the commit message or in the code comments.
• It is assumed that the environment variables "CARGO_CFG_TARGET_OS", "CARGO_CFG_TARGET_ARCH", and "CARGO_CFG_TARGET_ENV" will always be present and have valid values. It would be good to add some error handling or validation in case these environment variables are missing or have unexpected values.
• It is not clear how the build process will handle unsupported combinations of OS, architecture, and libc. It would be helpful to have some validation or error handling for unsupported configurations.
• There is a mix of different formatting styles for string concatenation (target = target + ... vs target += ...). It would be good to use a consistent style throughout the code.

Overall, the code changes seem to implement the requested functionality, but it would be beneficial to address the potential problems mentioned above and provide more context or explanation for the introduced changes.

Commit 3e10dda1ed904c37dbd0a27867524afebddcfd19

Key Changes:

• Fixed clippy checks failing with nightly version 1.73

Potential Problems:

• The patch does not provide any additional context or explanation for the changes made.
• It is not clear what specific clippy checks were failing and how they were fixed.
• The changes do not include any tests or test cases to verify the correctness of the code changes.
• It is unclear why the changes were necessary or what problem they solve.

Overall, the patch seems to be a small fix for clippy checks, but it lacks sufficient information and context to fully understand its purpose and potential impact.

Commit ee6ed289b1b7394ebf89815f1da80b0f73b6ac7b

Key changes:

• Added explanation about build environment variables in the README.md file.
• Provided information on how to set the WASMEDGE_[INCLUDE|LIB]_DIR, WASMEDGE_DIR, WASMEDGE_BUILD_DIR, and WASMEDGE_STANDALONE_[PROXY|PROXY_USER|PROXY_PASS|ARCHIVE] environment variables.
• Added information on automatic downloads of the WasmEdge library based on different architectures.
• Added information on setting the WASMEDGE_RUST_BINDGEN_PATH environment variable.

Potential problems:

• The patch modifies the README.md file to include information about build environment variables, which can be helpful for users. There doesn't appear to be any immediate problems with the changes, but they may need to be reviewed for clarity and accuracy.
• The patch also removes information about WasmEdge versioning in the wasmedge-sys crate's README.md file. It's unclear why this change was made and if it will have any impact on users who rely on this information.

Overall, the changes seem to be helpful for users who want to build and use the WasmEdge Rust SDK. However, the removal of versioning information in the wasmedge-sys crate's README.md file may require further investigation.

[jprendes]

@apepkuss PTAL :-)

[jprendes]

flows summarize

[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 titled "Add options to allow generating statically linked binaries" introduces several changes to the codebase. While the additions are valuable, there are some potential issues and areas for improvement that need to be addressed.

Potential issues:

1. The function link_lib does not handle the case where the environment variable for the library path is not set, which may result in a runtime error if the library is not found.
2. The code assumes specific environment variable names (WASMEDGE_DEPS_LINK_TYPE, WASMEDGE_DEPS_LIB_PATH, etc.), which might not hold in all environments.
3. The code depends on a specific version of the phf dependency, potentially causing build or functionality issues if this version is not available.
4. The use of the expect method to handle errors could lead to panics, indicating a need for more graceful error handling.
5. The usage and configuration of the WASMEDGE_STANDALONE_PROXY environment variable are undocumented, making it unclear how to set it or what values it can take.
6. The message in the as_path code, which prompts users to rerun the build if an environment variable changes, is not consistently printed for all variables, potentially causing inconsistencies in the build process.
7. There is no handling for the case where the bindgen executable is not found or fails to execute, only checking for success.
8. The inclusion of a signed-off-by line in the commit message may not align with the project's conventions, necessitating its removal.

Important findings:

• The addition of options for generating statically linked binaries is a valuable enhancement.
• The modifications to the bindgen generation process to handle the bindgen executable's path in an environment variable improve flexibility.
• The function to link libraries based on dependencies is a valuable addition.

In summary, while the pull request adds useful options for generating statically linked binaries and includes enhancements for the bindgen generation process and linking libraries, there are several potential issues and areas for improvement that need to be addressed before merging the changes.

Details

Commit 48e2225083d7c74b9ece7cad504699a4cecab070

Key changes:

• Added options to allow generating statically linked binaries.
• Added a function to link libraries based on dependencies.
• Modified the bindgen generation process to handle the case where the path to the bindgen executable is provided in an environment variable.
• Added a macro to handle environment variables.

Potential problems:

1. The function link_lib does not handle the case where the environment variable for the library path is not set. It could result in a runtime error if the library is not found.
2. The code makes assumptions about the environment variable names (WASMEDGE_DEPS_LINK_TYPE, WASMEDGE_DEPS_LIB_PATH, etc.). These assumptions might not hold in all environments.
3. The code relies on a specific version of a dependency (phf). If this version is not available, the code may not build or function correctly.
4. The code uses the expect method to handle errors, which will cause the program to panic if an error occurs. It would be better to use a more graceful error handling strategy.
5. The do_http_request function uses an environment variable WASMEDGE_STANDALONE_PROXY to determine if a proxy should be used, but there is no documentation on how to set this variable or what values it can take.
6. The code in as_path prints a message to rerun the build if an environment variable changes. However, this message is not printed for all variables, leading to possible inconsistencies in the build process.
7. The code does not handle the case where the bindgen executable is not found or fails to execute. It only checks for success but does not handle failure.
8. The code includes a signed-off-by line in the commit message. If this is not a convention followed by the project, it should be removed.

Overall, the code changes add options for generating statically linked binaries, but there are several potential issues and areas for improvement.

Commit 6ab6c9bccc7873588273f4c83530926abe8a0cb8

Key changes:

• Added support for musl libc by modifying the REMOTE_ARCHIVES map and the get_remote_archive() function.
• Modified the logic in the get_remote_archive() function to handle the CARGO_CFG_TARGET_ENV variable, which contains the target environment.

Potential problems:

• The addition of support for musl libc may introduce compatibility issues with other platforms or build configurations. This should be thoroughly tested.
• The modification in the get_remote_archive() function could introduce bugs or logic errors. A careful code review is necessary to ensure the correctness of the changes.
• The change does not include any tests. It would be ideal to add tests to validate the functionality of the newly added support for musl libc and the modified logic in the get_remote_archive() function.

Commit c16b76b14fdb76d0e9a68246650f32b9126d313b

Key changes in this patch:

• There are changes in three files:
  1. crates/async-wasi/src/snapshots/preview_1/async_socket.rs
  2. crates/wasmedge-sys/src/async/module.rs
  3. crates/wasmedge-sys/src/instance/function.rs
• The changes involve replacing mutable references to a memory (&mut M) with immutable references (&M) in the sock_connect function in async_socket.rs and module.rs. This change fixes clippy checks failing with nightly version 1.73.
• In the function.rs file, the change involves replacing a mutable reference to an engine (&mut E) with an immutable reference (&E) in the call_async function.

Potential problems to investigate:

• The patch does not provide much context about the overall functionality or purpose of the code being modified. It would be helpful to review the pull request description or associated issue to understand the motivation behind these changes.
• It's unclear why the change from mutable to immutable references is necessary to fix the clippy checks failing. It would be good to understand the exact error and verify that this change is the correct solution.
• It's also important to investigate whether these changes introduce any potential performance or functionality regressions. Reviewing relevant tests and considering the impact on surrounding code would be beneficial.
• The patch does not provide any test cases or evidence that these changes were tested. It would be good to confirm that all affected code paths are properly covered by tests and that the changes pass those tests.

Commit de8c512f1e83856cbb74263684185dca42e67899

Key changes:

• Added explanations of build environment variables related to the WasmEdge library installation process.
• Updated the table showing the version compatibility between the crates.
• Updated the instructions on how to build the wasmedge-sys crate.

Potential problems:

• There are no major problems with the patch. It mainly adds documentation to explain how to use build environment variables and provides more detailed instructions for building the wasmedge-sys crate.
• However, the patch could benefit from more specific descriptions of the build environment variables and their purposes.
• The update to the compatibility table in both README files is important, as it provides users with crucial information about the compatibility between different versions of the crates.
• The patch makes several changes to the README files, so careful review should be conducted to ensure the new information is accurate and clear for users.

[apepkuss]

Thanks for the contributions!