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 more details for Windows MSVC build process. #156

Merged
merged 2 commits into from
Sep 15, 2023
Merged

Add more details for Windows MSVC build process. #156

merged 2 commits into from
Sep 15, 2023

Conversation

am009
Copy link
Contributor

@am009 am009 commented Aug 21, 2023

Explanation

I'm working on adding support of MSVC for WasmEdge. This adds related docs for this new feature.
See this issue WasmEdge/WasmEdge#2629
See also this PR WasmEdge/WasmEdge#2751

What type of PR is this

/kind documentation

Copy link
Collaborator

alabulei1 commented Aug 21, 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 building WasmEdge using MSVC on Windows. The key changes include adding instructions, PowerShell scripts, and test instructions specific to MSVC.

There are several potential problems identified in the patches. Firstly, the assumption of the presence of vswhere and choco tools should be documented as prerequisites. Secondly, instructions for obtaining and installing LLVM 16 and the Windows SDK should be included. Thirdly, the use of relative paths for the LLVM directory may not work in all scenarios, and instructions for specifying the absolute path should be provided. Lastly, there is duplicated code that needs to be refactored to avoid code duplication.

In addition, another set of key changes was identified in a subsequent summary. These changes include updating the required LLVM version to 16, updating the download links, and adding a note for users encountering errors with the community version of Visual Studio.

A potential problem identified in this set of changes is that the file paths for the LLVM directories are hard-coded in the script, which may cause issues if the directory structure changes in the future. The patch also lacks information about other dependencies or requirements that may have changed alongside the LLVM version.

In summary, the pull request brings valuable additions for Windows MSVC build support, but there are potential issues and errors such as missing prerequisites, hard-coded paths, missing dependency update instructions, and duplicated code that need to be addressed.

Details

Commit 57af89b42df08e336ff90987c52b7a3d20ae4488

Key changes in the patch:

  1. Added instructions for Windows MSVC build in the windows.md file.
  2. Added PowerShell scripts for setting up the environment for building WasmEdge using MSVC.
  3. Added instructions to comment out the environment variable settings for Clang-cl and uncomment them for MSVC.
  4. Added instructions for running tests.

Potential problems:

  1. The patch assumes the presence of the vswhere and choco tools, which may not be available on all systems. This should be documented as a prerequisite.
  2. The patch assumes the use of LLVM 13 and a specific version of the Windows SDK. It would be helpful to include instructions for obtaining and installing these dependencies.
  3. The patch uses relative paths for the LLVM directory, which may not work in all scenarios. It would be better to provide instructions for specifying the absolute path.
  4. There is duplicated code in the patch. The instructions in windows.md are repeated in the Chinese language version of the file. This should be refactored to avoid code duplication.

Commit cc7a1c32982108c00af4c04edb2ae3ddda9c4a83

Key changes:

  • Updated the required LLVM version from 13 to 16.
  • Updated the download links for the pre-built LLVM 16 binary.
  • Added a note for users encountering errors when using the community version of Visual Studio.

Potential problems:

  • It seems that the file paths for the LLVM directories are hard-coded in the script. If the directory structure of the downloaded files changes in the future, the script may fail to locate the required files.
  • The patch does not mention whether any other dependencies or requirements have changed apart from the LLVM version. It would be helpful to include this information if there are other dependencies that need to be updated as well.

@alabulei1
Copy link
Collaborator

Hi @hydai

Please help review this PR. Thanks.

@hydai
Copy link
Member

hydai commented Aug 21, 2023

I am going to review this after the major PR of this feature gets merged in the wasmedge repo.

@hydai
Copy link
Member

hydai commented Sep 12, 2023

The main PR is merged. We can process this now.

@am009 am009 changed the title Add process for Windows MSVC build. Add more details for Windows MSVC build process. Sep 12, 2023
@hydai hydai merged commit 80b2a5f into WasmEdge:main Sep 15, 2023
5 checks passed
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.

3 participants