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

Refactor build script #40

Merged
merged 1 commit into from
Jul 28, 2023
Merged

Conversation

jprendes
Copy link
Contributor

@jprendes jprendes commented Jul 26, 2023

This PR refactors the wasmedge-sys build script to remove the dependency on the external commands wget, tar and bash.
It also reduces the amount of duplicated code in the build script.

Fixes #30.
Fixes #34.

Copy link
Member

juntao commented Jul 26, 2023

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


Commit 5d7d6f878ddf0795ab9d2296c04bbc2bc0d362ca

Key Changes:

  • Added new dependencies: reqwest, flate2, tar, sha256, lazy_static, phf
  • Created new files: build_paths.rs, build_standalone.rs
  • Reorganized code structure by moving some code to separate modules
  • Improved search for libwasmedge by using a list of search locations

Potential Problems:

  • The function find_wasmedge() is now split into two separate functions based on the static feature. This could lead to confusion and potential bugs if not handled correctly.
  • The code now relies on external dependencies (reqwest, flate2, tar, sha256, lazy_static, phf). Make sure these dependencies are properly documented and versioned.
  • The changes made to the build script may introduce compatibility issues with existing code or build configurations. It is important to thoroughly test the changes in different environments.
  • The added search locations are hardcoded. Consider making them configurable or dynamically determined based on the system.
  • The patch does not include any information about the reasoning behind the changes or any additional documentation to help understand them. It would be helpful to have more context when reviewing the changes.

@jprendes jprendes marked this pull request as ready for review July 26, 2023 15:31
@jprendes
Copy link
Contributor Author

@apepkuss PTAL :-)

@apepkuss
Copy link
Collaborator

@jprendes This PR is a BIG STEP for the build script. Really thank you for your efforts. BTW, could this PR also fix #34? In this issue, @ipuustin mentions "proxy prison". Thanks.

@jprendes jprendes force-pushed the refactor-build-script branch from 45d4065 to a317f1c Compare July 27, 2023 14:23
@jprendes
Copy link
Contributor Author

I pushed a new version. To help with reproducible builds:

  • It now validates the checksum of the downloaded archives
  • Adds env-vars to configure a proxy for the downloads: WASMEDGE_STANDALONE_PROXY{_USER,PASS}.
  • Adds an env-var to use a local archive: WASMEDGE_STANDALONE_ARCHIVE

@ipuustin I think this should address the concerns in #34

@jprendes jprendes force-pushed the refactor-build-script branch 2 times, most recently from be7ae85 to 0a0c69b Compare July 27, 2023 14:43
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
@jprendes jprendes force-pushed the refactor-build-script branch from 0a0c69b to 5d7d6f8 Compare July 27, 2023 14:44
@apepkuss apepkuss merged commit 3dbaaff into WasmEdge:main Jul 28, 2023
@juntao juntao mentioned this pull request Aug 1, 2023
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.

Standalone mode support for reproducible builds Remove dependency on wget and tar for the standalone feature
3 participants