search

WasmEdge / docs

https://wasmedge.org/docs/
Apache License 2.0
17 stars 57 forks source link

Add more details for Windows MSVC build process. #156

Closed am009 closed 1 year ago

am009 commented 1 year ago

Explanation

I'm working on adding support of MSVC for WasmEdge. This adds related docs for this new feature. See this issue https://github.com/WasmEdge/WasmEdge/issues/2629 See also this PR https://github.com/WasmEdge/WasmEdge/pull/2751

What type of PR is this

/kind documentation

alabulei1 commented 1 year ago

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:

Potential problems:

alabulei1 commented 1 year ago

Hi @hydai

Please help review this PR. Thanks.

hydai commented 1 year ago

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

hydai commented 1 year ago

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