[wip] test with esp-clang #1295
Conversation
Code Review for PR #1295Todo List:
OverviewThis PR adds ESP Clang toolchain support to the llgo installation process. The changes include:
✅ StrengthsGood Architecture & Design:
Security & Robustness:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @luoliwoshang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant enhancement to the llgo installation process by adding support for building with the ESP Clang toolchain. This allows for a more specialized build environment, particularly beneficial for cross-compilation scenarios targeting ESP platforms, by bundling the necessary LLVM components directly.
Highlights
- New ESP Clang Integration: Introduced a new installation method (install_local_with_esp) in install.sh to build llgo using the ESP Clang toolchain.
- Cross-Compilation Support: The new method handles downloading the ESP Clang toolchain and configures CGO_CPPFLAGS, CGO_LDFLAGS, and rpath for macOS and Linux to enable building llgo with a bundled LLVM.
- Installation Script Enhancement: The install.sh script now accepts a --with-esp argument, allowing users to choose between the basic local installation and the new ESP Clang integrated build.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new installation method in install.sh to build llgo with the ESP-Clang toolchain, activated by a --with-esp flag. The new install_local_with_esp function manages downloading the toolchain and configuring the CGO environment. My review highlights a significant issue with the linker's runtime path (rpath) configuration, which would prevent the compiled binary from running correctly. I have provided a comprehensive code suggestion to fix this issue and refactor the script for better maintainability and robustness.
| if [ "$OS" = "darwin" ]; then | ||
| export CGO_CPPFLAGS="-I$(pwd)/crosscompile/clang/include -mmacosx-version-min=10.13 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS" | ||
| export CGO_LDFLAGS="-L$(pwd)/crosscompile/clang/lib -mmacosx-version-min=10.13 -lLLVM-19 -lz -lm -Wl,-rpath,@executable_path/../crosscompile/clang/lib" | ||
| go install -tags="${OS},${ARCH},byollvm" ./cmd/llgo | ||
| elif [ "$OS" = "linux" ]; then | ||
| export CGO_CPPFLAGS="-I$(pwd)/crosscompile/clang/include -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS" | ||
| export CGO_LDFLAGS="-L$(pwd)/crosscompile/clang/lib -lLLVM-19" | ||
| go install -tags="${OS},${ARCH},byollvm" -ldflags='-extldflags=-Wl,-rpath,$ORIGIN/../crosscompile/clang/lib' ./cmd/llgo | ||
| else | ||
| echo "Error: Unsupported OS: ${OS}" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This block for OS-specific build flags has a high-impact issue with rpath and could also be improved for maintainability.
-
Incorrect
rpath: Therpathset for thellgobinary is incorrect for bothdarwinandlinux.go installplaces the binary in$GOPATH/bin, but the relative path provided (../crosscompile/clang/lib) assumes a different file structure. This will cause dynamic linking errors at runtime as the LLVM shared libraries won't be found. A fix is to use an absolute path to the libraries for local development builds. -
Code Duplication: There's significant repetition in setting
CGO_CPPFLAGS,CGO_LDFLAGS, and thego installcommand. Extracting common parts into variables will make the script cleaner and easier to maintain. -
Brittle
$(pwd): The script relies on$(pwd), which assumes it's executed from the project root. It's more robust to calculate paths relative to the script's own location.
Here is a suggested refactoring that addresses all these points:
| if [ "$OS" = "darwin" ]; then | |
| export CGO_CPPFLAGS="-I$(pwd)/crosscompile/clang/include -mmacosx-version-min=10.13 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS" | |
| export CGO_LDFLAGS="-L$(pwd)/crosscompile/clang/lib -mmacosx-version-min=10.13 -lLLVM-19 -lz -lm -Wl,-rpath,@executable_path/../crosscompile/clang/lib" | |
| go install -tags="${OS},${ARCH},byollvm" ./cmd/llgo | |
| elif [ "$OS" = "linux" ]; then | |
| export CGO_CPPFLAGS="-I$(pwd)/crosscompile/clang/include -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS" | |
| export CGO_LDFLAGS="-L$(pwd)/crosscompile/clang/lib -lLLVM-19" | |
| go install -tags="${OS},${ARCH},byollvm" -ldflags='-extldflags=-Wl,-rpath,$ORIGIN/../crosscompile/clang/lib' ./cmd/llgo | |
| else | |
| echo "Error: Unsupported OS: ${OS}" | |
| exit 1 | |
| fi | |
| SCRIPT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) | |
| CLANG_DIR="${SCRIPT_DIR}/crosscompile/clang" | |
| # Common CGO flags | |
| COMMON_CPPFLAGS="-I${CLANG_DIR}/include -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS" | |
| COMMON_LDFLAGS="-L${CLANG_DIR}/lib -lLLVM-19" | |
| INSTALL_TAGS="${OS},${ARCH},byollvm" | |
| if [ "$OS" = "darwin" ]; then | |
| export CGO_CPPFLAGS="${COMMON_CPPFLAGS} -mmacosx-version-min=10.13" | |
| export CGO_LDFLAGS="${COMMON_LDFLAGS} -mmacosx-version-min=10.13 -lz -lm -Wl,-rpath,${CLANG_DIR}/lib" | |
| go install -tags="${INSTALL_TAGS}" ./cmd/llgo | |
| elif [ "$OS" = "linux" ]; then | |
| export CGO_CPPFLAGS="${COMMON_CPPFLAGS} -D_GNU_SOURCE" | |
| export CGO_LDFLAGS="${COMMON_LDFLAGS}" | |
| go install -tags="${INSTALL_TAGS}" -ldflags="-extldflags=-Wl,-rpath,${CLANG_DIR}/lib" ./cmd/llgo | |
| else | |
| echo "Error: Unsupported OS: ${OS}" | |
| exit 1 | |
| fi |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1295 +/- ##
=======================================
Coverage 90.22% 90.22%
=======================================
Files 42 42
Lines 11931 11931
=======================================
Hits 10765 10765
Misses 1032 1032
Partials 134 134 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
we are release the esp-clang,so we need func test with esp-clang