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 options to allow generating statically linked binaries #46

Closed
wants to merge 1 commit into from

Conversation

jprendes
Copy link
Contributor

@jprendes jprendes commented Aug 1, 2023

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.

This PR add options to specify the type of linking required for the different dependencies using environment variables and removes the runtime feature from bindgen.

Copy link
Member

juntao commented Aug 1, 2023

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


Commit 48e2225083d7c74b9ece7cad504699a4cecab070

Key changes:

  • Added options to allow generating statically linked binaries.
  • Changed the way dependency libraries are linked.

Potential problems:

  • The patch modifies the main function in build.rs, which is the main build script for the project. This function is responsible for configuring the build process and generating bindings. Any changes in this function could affect the build process and generate incorrect bindings.

  • The patch introduces a new function link_lib which is used to link the dependency libraries. The function seems to handle the sanitization of dependency names for environment variables but it is not clear whether this sanitization is necessary or correct.

  • The patch modifies the Env struct in build_paths.rs to print a rerun-if-env-changed directive for all environment variables except OUT_DIR and those starting with CARGO_. This directive is used to signal Cargo to re-run the build process if any of the specified environment variables change. While this can help ensure a correct build process, it is not clear why only specific environment variables are excluded.

  • The patch adds a new macro Env! for creating an Env struct with a given environment variable name. It is not clear why this macro is necessary or why it is used in some places but not in others.

  • The patch changes the way the Rust bindgen tool is invoked. Instead of using the bindgen crate directly, it tries to run a custom bindgen command specified by the WASMEDGE_RUST_BINDGEN_PATH environment variable. If the variable is not set or the command fails, it falls back to using the bindgen crate. This change introduces an additional dependency on an external executable and may break the build process if the command specified by the WASMEDGE_RUST_BINDGEN_PATH variable is not valid.

Overall, the patch introduces several changes that could potentially affect the build process and the correctness of the generated bindings. It is important to thoroughly test these changes and ensure they do not introduce any regressions or unexpected behavior.

@jprendes jprendes force-pushed the non-runtime-bindgen branch 5 times, most recently from dd28a4f to e6d89a7 Compare August 1, 2023 22:05
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.

2 participants