8000 [Utils] Install script: Fix #1080 by SAtacker · Pull Request #1084 · WasmEdge/WasmEdge · GitHub
[go: up one dir, main page]
Skip to content

[Utils] Install script: Fix #1080 #1084

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jan 30, 2022
Merged

[Utils] Install script: Fix #1080 #1084

merged 11 commits into from
Jan 30, 2022

Conversation

SAtacker
Copy link
Collaborator

Fix #1080

Testing
Install:

curl -sSf https://raw.githubusercontent.com/SAtacker/WasmEdge/install_script/utils/install.sh | bash

@SAtacker
[Utils] Install Script:Add Linux aarch64 support for extensions WasmE…
8c8cc2c 
…dge#1080

Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
@SAtacker
[Utils] Install Script: Cases for arch in linux packages
7db8c7e 
Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
@codecov
Copy link
codecov bot commented Jan 29, 2022 •
edited
Loading

Codecov Report

Merging #1084 (6be6962) into master (153a082) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1084      +/-   ##
==========================================
- Coverage   70.34%   70.33%   -0.02%     
==========================================
  Files         116      116              
  Lines       15929    15929              
  Branches     3729     3729              
==========================================
- Hits        11206    11204       -2     
- Misses       2605     2612       +7     
+ Partials     2118     2113       -5
Impacted Files Coverage Δ
lib/executor/helper.cpp 56.15% <0.00%> (-1.54%) ⬇️
lib/host/wasmedge_process/processfunc.cpp 45.98% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec00362...6be6962. Read the comment docs.

@SAtacker
8000
Copy link
Collaborator Author

@juntao It seems that the support for aarch64 is there for the beta versions and not for the stable one.

@juntao
Copy link
Member
juntao commented Jan 29, 2022

Yes. So I expect if I pass the beta version number to the install script, I should be able to install the extensions. Is that correct?

@SAtacker
Copy link
Collaborator Author
SAtacker commented Jan 29, 2022 •
edited
Loading

Yes. So I expect if I pass the beta version number to the install script, I should be able to install the extensions. Is that correct?

Yes, that should be the case.
However what do I do about the failing test case? Should I change the GitHub CI by passing in the version?

@SAtacker
Copy link
Collaborator Author
SAtacker commented Jan 29, 2022 •
edited
Loading

Another could be the addition to install script to check whether the image-deps have a version greater than or equal to 0.9.1-beta.1 for aarch64

@juntao
Copy link
Member
juntao commented Jan 29, 2022

Another could be the addition to install script to check whether the image-deps have a version greater than or equal to 0.9.1-beta.1 for aarch64

That is probably a better solution. If the user wants to install an earlier version, we cannot install extensions anyway.

@SAtacker
[Utils] Install Script: Check availability of extension support for a…
641e4c7 
…arch

Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
@SAtacker
[Utils] Uninstall Script: Fix shell configuration not containing sour…
eab791b 
…cing

Issue: `Sourcing not found in /github/home/.bashrc `
Expected: Script should not exit

Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
@SAtacker
Copy link
Collaborator Author 
curl -sSf https://raw.githubusercontent.com/SAtacker/WasmEdge/install_script/utils/install.sh | bash -s -- -e all --version=0.9.1-beta.1 --tf-version=0.9.1-beta.1 --tf-deps-version=0.9.1-beta.1  --tf-tools-version=0.9.1-beta.1  --image-version=0.9.1-beta.1 --image-deps-version=0.9.1-beta.1

Uh oh!

There was an error while loading. Please reload this page.
@SAtacker
Copy link
Collaborator Author

https://github.com/second-state/WasmEdge-tensorflow-deps/releases/tag/0.9.1-beta.1 does not have TF deps for aarch64 while beta-2 has. CC @hydai

@SAtacker
[CI] GitHub CI: Add installation for 0.9.1-beta.1
744afc8 
* Extensions for image and tensorflow are available

Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
@hydai
Copy link
Member
hydai commented Jan 29, 2022

Hi @SAtacker
TF only supports x86_64, and does not support on aarch64.
We only provides TF-lite on aarch64.

@SAtacker
Copy link
Collaborator Author

Hi @SAtacker TF only supports x86_64, and does not support on aarch64. We only provides TF-lite on aarch64.

Thanks, I'll then make it OS specific.

@SAtacker
[Utils] Install Script: Tensorflow not supported on aarch64
6a76dd7 
Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
@SAtacker
Copy link
Collaborator Author

It should work now. Thanks all!

@SAtacker
Copy link
Collaborator Author

The test-install-script should not fail after you merge the changes in the uninstall script.
The commit eab791b should fix it.

@juntao
Copy link
Member
juntao commented Jan 29, 2022

It should work now. Thanks all!

Can you update the test installer link?

@SAtacker
Copy link
Collaborator Author

It should work now. Thanks all!

Can you update the test installer link?

Yes, just to test right?

@SAtacker
[Utils] Test Uninstall Script
3342c00 
Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
@SAtacker
Copy link
Collaborator Author

Thanks, humble reminder that the last commit was only to test the CI please don't merge.

SAtacker and others added 3 commits January 29, 2022 15:18
@SAtacker
[Utils] Install Script: Skipping empty binary names
8eabd6d 
Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
@SAtacker
Revert "[Utils] Test Uninstall Script"
1a2512e 
This reverts commit 3342c00.

Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
@hydai
Merge branch 'master' into install_script
d0c7ed3
@SAtacker
8000 Copy link
Collaborator Author

It's ready, please review.
Thanks
CC @hydai @juntao

juntao
juntao approved these changes Jan 30, 2022
View reviewed changes
Copy link
Member
@juntao juntao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it on an AWS t4g instance. It works. Thanks.

@hydai hydai merged commit 8f0673a into WasmEdge:master Jan 30, 2022
@juntao
Copy link
Member
juntao commented Apr 2, 2023

flows summarize

@alabulei1 Flows.network Integration
Copy link
Contributor

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

The set of reviews includes multiple patches for different files in a GitHub repository. The changes introduced vary from adding support for new architectures or versions to resolving and preventing potential issues. However, some of the patches lack proper testing or an explanation for why changes were made, which could cause unforeseen issues. Additionally, some patches include conditional changes that require further examination, and it is essential to review or test the patches before merging them. Finally, one of the patches is a revert patch, which does not provide an explanation for why the change was made in the first place.

Details

Commit 1

This is a patch for a single commit titled "Install Script: Add Linux aarch64 support for extensions" which modifies the utils/install.sh file. In this commit, the IF_EXT_COMPAT and TF_EXT_COMPAT variables are removed for Linux aarch64 architecture.

Potential problems to consider during the review could include:

  • The reason why the IF_EXT_COMPAT and TF_EXT_COMPAT variables were removed is not clear. The reviewer should ask the author to provide an explanation for this change.
  • The code has not been tested yet, so the author should provide a testing plan and explain how they have tested the code changes locally.

Commit 2

This GitHub patch is for one specific commit in the utils/install.sh file. The patch makes changes to the script to install cases for arch in Linux packages.

The changes added a CASE statement to the detect_os_arch function to distinguish between different architectures on Linux. The current code is looking for aarch64, and if that ARCH is detected, then it uses the manylinux2014_aarch64.tar.gz package to install cases. The patch changes it to check for additional architectures like arm64, armv8*, and "amd64". It also adds a default case to print a message for unsupported architectures and exit.

As for potential problems, since the patch is only for this specific commit, we don't know what other changes are present in the codebase. Also, the script may contain other bugs which are not detected by the patch, so it would be best to perform testing after applying the patch.

Commit 3

This patch seems to add support for aarch64 architectures in the installation script. It is adding checks to verify the compatibility of Image and Tensorflow extensions versions with aarch64 architecture. If they are compatible, only then they get installed. The potential problem could be that the patch is not tested on other architectures, so it could break the installation process for other architectures.

Commit 4

This patch is for the uninstall.sh file in the "utils" folder of a GitHub repository. It resolves an issue where the shell configuration not containing sourcing results in the script exiting with an error. The patch adds a condition that if the sourcing is not found in the shell configuration, it prints a warning message instead of exiting the script.

Potential problems that can be identified are:

  • There may be a scenario where the sourcing is not found in the shell configurations, but the script still executes successfully. This scenario should be tested to ensure that the warning message is printed correctly and the script executes successfully without sourcing.
  • It may be helpful to add comments to the code to make it more readable and easier to understand.

Commit 5

This patch adds an installation step for version 0.9.1-beta.1 of WasmEdge and its extensions for image and TensorFlow. It also includes a test for arm extension installation.

A potential problem could arise if the installation step fails or if the new version and its extensions cause compatibility issues with the existing codebase. Additionally, it may be worthwhile to ensure that the installation process does not cause any conflicts with other dependencies or packages on the system.

Commit 6

This is a patch for the install.sh script in the "utils" directory of a project. The patch contains changes to support the installation of WasmEdge-tensorflow (a deep learning inference engine) on non-aarch64 platforms (i.e., machines with processors other than Arm-based ones). Specifically, this patch adds support for fetching and installing WasmEdge-tensorflow dependencies (named "WasmEdge-tensorflow-deps") and main packages. It also includes a conditional check that skips installation of WasmEdge-tensorflow on aarch64 platforms, as it is not currently supported there.

As for potential problems, it seems that this patch replaces some existing lines of code in the get_wasmedge_tensorflow_deps() and install_wasmedge_tensorflow() functions. The changes inside the if-statement in install_wasmedge_tensorflow() could introduce a problem if "$RELEASE_PKG" =~ "aarch64" returns unexpected output, which is something that should be tested thoroughly. The conditional check itself is also worth examining since it could potentially pose problems in the future when version compatibilities change or when new platforms are supported. Overall, it seems that these changes are fairly straight-forward, but some testing and review are necessary before merging them.

Commit 7

This patch modifies the utils/install.sh script to point to a new repository branch when invoking the uninstall script. Specifically, it updates the URL to the uninstall script on GitHub to point to the install_script branch of the SAtacker/WasmEdge repository.

A potential problem with this change is that it only updates the uninstall script URL if the REMOVE_OLD variable is set to 1 or if the user types "yes" when prompted. If the user chooses "no" or otherwise skips this prompt, the script will still use the old uninstall script URL. This could result in inconsistencies and incorrect behavior for users who choose to retain older versions of WasmEdge.

Commit 8

This patch is adding a condition in the wasmedge_checks function of the utils/install.sh script to skip installation of empty binary names. The condition checks if the binary name is empty and continues to the next iteration if it is. This change should prevent installation of binaries with empty names and improve code robustness.

Based on this patch, there do not appear to be any potential problems or issues.

Commit 9

This is a revert patch, reverting commit 3342c00. The key change is in the utils/install.sh file where the script now uses the WasmEdge uninstall script from the main repository instead of from the forked repository SAtacker/WasmEdge. The potential problem is that the motivation for the original commit - testing the uninstall script - is not mentioned in the revert message.

@juntao
Copy link
Member
juntao commented Apr 5, 2023

I will now say the magic words to summon the GPT4 review bot!

flows summarize

@alabulei1 Flows.network Integration
Copy link
Contributor

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

Overall Summary:

The provided patches introduce various changes to install.sh and uninstall.sh scripts under the utils directory, focusing on adding support for Linux aarch64 architecture and improving software compatibility. The most significant findings in these patches are:

Potential Issues and Errors:

  1. Disabling compatibility checks for IM_EXT and TF_EXT might cause issues during installation on aarch64.
  2. Modification of the utils/install.sh script might cause backward compatibility issues for some users.
  3. 'x86_64' might not be handled correctly in the architecture case statement.
  4. Compatibility checks rely on string comparisons which could fail if the version format changes.
  5. A proper message should be provided to indicate unsupported Tensorflow installation due to architecture incompatibility.
  6. The patch that points to the user's fork for uninstall.sh might lead to missing future updates or bug fixes.
  7. The conditional check introduced to skip empty binary names does not handle other non-desired inputs.

Important Findings:

  1. Support for Linux aarch64 architecture has been added through modifications to the install script.
  2. The uninstall script now continues executing even when the sourcing is not found in the .bashrc file.
  3. Changes in compatibility checks added for aarch64 in install_image_extensions() and install_tf_extensions() functions.
  4. A new GitHub CI installation step has been introduced for WasmEdge version 0.9.1-beta.1, along with its extensions for image and TensorFlow.
  5. The revert patch restores the URL for the uninstall.sh script back to the original repository instead of the user's fork.

It is essential to test these changes thoroughly with different system architectures to ensure the conditional checks are working as expected, and the user experience is smooth. Additional attention should be paid to the removal of compatibility checks and the implementation of proper error messages for unsupported installations. Furthermore, future-proofing the compatibility checks by using a semver library for version comparisons should also be considered.

Details

Commit 1

This patch is for updating the install.sh script in the utils directory. The key change in this patch is the addition of support for Linux aarch64 architecture for extensions.

Summary of changes:

  1. Removed two lines related to IM_EXT_COMPAT and TF_EXT_COMPAT, which originally set both values to 0.

Potential issues:

  1. Disabling the compatibility checks for IM_EXT and TF_EXT might cause issues during installation on aarch64. However, this change might have been made after ensuring proper compatibility on this architecture. It would be useful to verify the compatibility before accepting the pull request.

Commit 2

The key changes in this patch are:

  1. Modification of the utils/install.sh script.
  2. Enhancement of architecture detection in the install script for Linux systems.

The changes are summarized below:

  • The previous implementation only looked for "aarch64" architecture, and in case of a match, it would set the RELEASE_PKG variable to "manylinux2014_aarch64.tar.gz".
  • The new implementation utilizes a nested case statement. It checks for the following architectures and sets the corresponding RELEASE_PKG variable:
    • 'x86_64'
    • 'arm64', 'armv8*', or 'aarch64' setting RELEASE_PKG to "manylinux2014_aarch64.tar.gz"
    • 'amd64' setting RELEASE_PKG to "manylinux2014_amd64.tar.gz"
  • If none of the above architectures match, the script now displays a message stating that the detected architecture is unsupported and exits with error code 1.

Potential problems:

  1. The case handling for 'x86_64' does not set a value for the RELEASE_PKG variable; there's a possibility that 'x86_64' might not be handled correctly.
  2. Backward compatibility issues might arise for users that are expecting the previous behavior.

However, the changes improve the architecture detection and handling for Linux systems, making it more flexible and easier to manage.

Commit 3

Summary of key changes:

  1. Added an architecture check for aarch64 in the installation script.
  2. Updated the install_image_extensions() function to check for compatibility of extensions with aarch64.
  3. Updated the install_tf_extensions() function to check for compatibility of extensions with aarch64.

Potential problems:

  1. The check for aarch64 compatibility in both install_image_extensions() and install_tf_extensions() might not work as expected if the minimum required version changes.
  2. The logic for checking compatibility relies on string comparisons and might fail if the version format changes. The use of semver library will provide more reliable comparisons.
  3. The compatibility checks are done in a specific order. If more architecture types are added, the nested if statements will quickly become difficult to maintain. Refactoring the code with a separate function for compatibility checks would improve readability and maintainability.

Commit 4

Summary:
The patch modifies the uninstall.sh script in the utils folder. It addresses an issue where the script would exit if a specific sourcing is not found in the .bashrc file, which is undesired. The changes allow the script to continue executing instead of exiting when the sourcing is not found in the .bashrc file.

Key Changes:

  1. Removed the if statement that checked whether line_num was empty or not.
  2. Changed the if statement condition from if [ "$line_num" != "" ] to [ "$line_num" != "" ].
  3. Removed the else block that printed "Sourcing not found in .bashrc" and called exit 1.
  4. Updated the previous if statement conditions into one-liners using && and ||.

Potential Problems:
No major problems are found. However, the patch assumes that if the sourcing is not found in the .bashrc file, it might be located in .profile or .bash_profile files. The script does not show any warning or error message when the sourcing is missing from .profile and .bash_profile files, which might cause confusion while uninstalling.

Commit 5

This patch introduces the following key changes:

  1. Adds a new GitHub CI installation step for WasmEdge version 0.9.1-beta.1, along with its extensions for image and TensorFlow.
  2. Modifies the .github/workflows/test-install-script.yml file by adding a new job step for this installation.

Potential problems:

  1. The installation step specifies multiple version flags for TensorFlow and image extensions, which may not be easily maintainable when updating. It is recommended to use a variable for the common version to avoid error-prone manual updates.
  2. There is no context on whether tests for this new version or dependencies have been added or updated. This should be investigated to ensure proper testing for the updated version and its extensions.

Commit 6

Summary of key changes:

  1. Added a condition to check if the RELEASE_PKG variable contains "aarch64", which indicates that Tensorflow is not supported. This condition is added in several key function calls in the install.sh script.
  2. If the condition is true, the functions involved return a "Tensorflow not supported" message, and some function calls are not executed.

Potential problems:

  1. There is a need for a proper message to indicate that the Tensorflow-related functions/installations will be skipped due to a specific architecture incompatibility. The current message "Tensorflow not supported" may not be clear enough for the users.
  2. It's essential to test these changes thoroughly with different system architectures to ensure the conditional checks are working as expected, and user experience is smooth.

Apart from these points, the changes seem to be in line with the intended behavior of disabling Tensorflow installation for aarch64 architecture.

Commit 7

This GitHub patch modifies the install.sh file in the Utils repository. The key change in this patch is an update to the URL used to fetch the uninstall.sh script. Below are the main finding and potential problems:

Key Changes:

  • The URL for the uninstall.sh script has been updated to fetch from the user's fork (SAtacker/WasmEdge) and from the branch install_script.

Potential Problems:

  • The URL is now pointing to the user's fork and not the original repository (WasmEdge/WasmEdge). This might not be ideal because future updates or bug fixes in the original repository's uninstall script might not be reflected in the user's fork. To avoid issues, it would be better to keep the URL pointing to the original repository, unless there's a specific reason for using the fork.

To sum up, there's only one significant change in the patch, but it could potentially cause problems by pointing to a fork instead of the original repository.

Commit 8

This GitHub patch, titled "[Utils] Install Script: Skipping empty binary names", is a single-commit change that modifies the install.sh script in the "utils" directory.

Key Changes:

  1. The patch adds a conditional statement to the wasmedge_checks function in the install.sh script. This function appears to check version numbers of various dependencies.
  2. The conditional statement checks if the variable $var is an empty string. If it is, the loop iteration is skipped using the continue statement.

Potential Problems:

  1. Although the change appears reasonable to skip empty strings, it does not handle other non-desired inputs such as invalid binary names or incorrectly formatted version strings.
  2. There are no test cases included in this patch, which makes it difficult to determine if the change is fully functional and does not inadvertently break any existing functionality.

Conclusion:
The patch skips empty binary names in the wasmedge_checks function in the utils/install.sh script. However, it does not account for other possible invalid inputs, and there are no test cases provided.

Commit 9

Summary of Key 9F73 Changes:

  • This patch reverts a previous commit (3342c00) titled "[Utils] Test Uninstall Script".
  • The change is in the file utils/install.sh.
  • The following line:
    • bash <(curl -sSf https://raw.githubusercontent.com/SAtacker/WasmEdge/install_script/utils/uninstall.sh) -p "$IPATH" -q
      has been replaced by:
    • bash <(curl -sSf https://raw.githubusercontent.com/WasmEdge/WasmEdge/master/utils/uninstall.sh) -p "$IPATH" -q

Potential Problems:

  1. Reliability: The install script is downloading and executing code from the "master" branch, which may not be always stable. It would be safer to point to a specific tagged version or commit hash.
  2. Network Dependency: The script relies on an internet connection to download the uninstall script. If there is any network issue or if the user is behind a firewall, this might not work.

@WasmEdge WasmEdge deleted a comment from alabulei1 Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juntao juntao juntao approved these changes

@hydai hydai Awaiting requested review from hydai

@ibmibmibm ibmibmibm Awaiting requested review from ibmibmibm

@q82419 q82419 Awaiting requested review from q82419

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Install] Add Linux aarch64 support for extensions
4 participants
@SAtacker @juntao @hydai @alabulei1
0