chore: upgrade to llgo v0.11.6 and simplify CI setup#590
Conversation
This commit addresses issue xgo-dev#586 by upgrading to llgo v0.11.6 and significantly simplifying the CI configuration. Changes: - Update go.mod to use llgo v0.11.6 release instead of pseudo-version - Create download-llgo.sh script to download llgo releases from GitHub - Remove LLVM installation from CI (now bundled with llgo release) - Remove llgo repository checkout from CI (use release binaries instead) - Update workflow files to use version tags instead of commit hashes - Simplify dependency installation by removing LLVM-related packages Benefits: - Faster CI builds (no need to compile llgo from source) - More reliable builds (using official releases) - Simpler maintenance (no LLVM version management) - Reduced CI complexity Closes xgo-dev#586 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @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 significantly enhances the efficiency, reliability, and maintainability of the project's Continuous Integration (CI) pipelines. By upgrading to the official Highlights
Ignored Files
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the CI process by upgrading llgo and switching to pre-built binaries instead of building from source. This is an excellent change that will make CI faster and more reliable. The new download script is a great addition. My review includes several suggestions to make this script more robust and idiomatic by improving error handling, using standard shell practices, and streamlining the download process. I've also included a minor suggestion for the action.yml file to improve clarity.
| curl -L -o "/tmp/${FILENAME}" "$URL" | ||
| tar -xzf "/tmp/${FILENAME}" -C "$INSTALL_DIR" --strip-components=1 | ||
| rm "/tmp/${FILENAME}" |
There was a problem hiding this comment.
The download and extraction process can be simplified and made more robust.
- Piping
curltotaravoids creating a temporary file on disk, which is more efficient. - Using
curl -fsSLprovides better error handling:-ffails on HTTP server errors,-ssilences progress output,-Sshows errors even with-s, and-Lfollows redirects.
This approach is cleaner and more idiomatic for shell scripting.
| curl -L -o "/tmp/${FILENAME}" "$URL" | |
| tar -xzf "/tmp/${FILENAME}" -C "$INSTALL_DIR" --strip-components=1 | |
| rm "/tmp/${FILENAME}" | |
| curl -fsSL "$URL" | tar -xz -C "$INSTALL_DIR" --strip-components=1 |
| export LLGO_ROOT=$GITHUB_WORKSPACE/.llgo | ||
| echo "LLGO_ROOT=$LLGO_ROOT" >> $GITHUB_ENV |
There was a problem hiding this comment.
The export command is not necessary here. A simple variable assignment is sufficient since $LLGO_ROOT is used in the next line within the same run step. Removing export makes the intent clearer that the variable is only used locally within this script block before being passed to the GitHub Actions environment.
LLGO_ROOT=$GITHUB_WORKSPACE/.llgo
echo "LLGO_ROOT=$LLGO_ROOT" >> $GITHUB_ENV| # Usage: ./download-llgo.sh <version> <install_dir> | ||
| # Example: ./download-llgo.sh v0.11.6 ./llgo | ||
|
|
||
| set -e |
There was a problem hiding this comment.
For more robust error handling in shell scripts, it's a good practice to use set -euo pipefail instead of just set -e.
e: Exit immediately if a command exits with a non-zero status.u: Treat unset variables as an error when substituting.o pipefail: The return value of a pipeline is the status of the last command to exit with a non-zero status, or zero if no command exited with a non-zero status. This is particularly useful when piping commands.
| set -e | |
| set -euo pipefail |
| if [ -z "$VERSION" ] || [ -z "$INSTALL_DIR" ]; then | ||
| echo "Usage: $0 <version> <install_dir>" | ||
| echo "Example: $0 v0.11.6 ./llgo" | ||
| exit 1 |
There was a problem hiding this comment.
It's a best practice to print usage and error messages to standard error (stderr) instead of standard output (stdout). This allows users to redirect script output without capturing error messages.
| if [ -z "$VERSION" ] || [ -z "$INSTALL_DIR" ]; then | |
| echo "Usage: $0 <version> <install_dir>" | |
| echo "Example: $0 v0.11.6 ./llgo" | |
| exit 1 | |
| if [ -z "$VERSION" ] || [ -z "$INSTALL_DIR" ]; then | |
| echo "Usage: $0 <version> <install_dir>" >&2 | |
| echo "Example: $0 v0.11.6 ./llgo" >&2 | |
| exit 1 | |
| fi |
| *) | ||
| echo "Unsupported architecture: $ARCH" | ||
| exit 1 | ||
| ;; |
There was a problem hiding this comment.
Error messages should be printed to standard error (stderr) to separate them from normal script output.
| *) | |
| echo "Unsupported architecture: $ARCH" | |
| exit 1 | |
| ;; | |
| *) | |
| echo "Unsupported architecture: $ARCH" >&2 | |
| exit 1 | |
| ;; |
| else | ||
| echo "Error: llgo binary not found at ${INSTALL_DIR}/bin/llgo" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Error messages should be printed to standard error (stderr) to separate them from normal script output.
| else | |
| echo "Error: llgo binary not found at ${INSTALL_DIR}/bin/llgo" | |
| exit 1 | |
| fi | |
| else | |
| echo "Error: llgo binary not found at ${INSTALL_DIR}/bin/llgo" >&2 | |
| exit 1 | |
| fi |
The llgo release files use the format llgo{version}.{os}-{arch}.tar.gz
(e.g., llgo0.11.6.darwin-arm64.tar.gz) instead of the assumed format
llgo-{version}-{os}-{arch}.tar.gz.
This commit fixes the download script to:
- Remove 'v' prefix from version number
- Use correct filename format: llgo0.11.6.darwin-arm64.tar.gz
- Update comments to reflect actual format
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
|
|
||
| # Create install directory | ||
| mkdir -p "$INSTALL_DIR" | ||
|
|
There was a problem hiding this comment.
CRITICAL: Missing checksum verification
The script downloads and executes prebuilt binaries without any integrity verification. This is a significant supply chain security risk.
Recommended fix:
# Add -f flag to fail on HTTP errors, and add retry logic
curl -fL --retry 3 --retry-delay 2 --max-time 300 -o "/tmp/${FILENAME}" "$URL" || {
echo "Error: Failed to download LLGo from $URL"
exit 1
}
# Download and verify checksum (if available)
curl -fL -o "/tmp/${FILENAME}.sha256" "${URL}.sha256" 2>/dev/null && {
cd /tmp
sha256sum -c "${FILENAME}.sha256" || {
echo "Error: Checksum verification failed"
exit 1
}
}Security concerns:
- Without checksums, there's no way to verify the binary hasn't been tampered with
- Missing
-fflag means curl won't fail on HTTP errors (404, 500) - No retry logic for transient network failures
See CWE-494: Download of Code Without Integrity Check
|
|
||
| set -e | ||
|
|
||
| VERSION=$1 |
There was a problem hiding this comment.
Add input validation for VERSION parameter
The VERSION parameter is interpolated into URLs without validation, creating potential for injection attacks if this script is used outside the controlled CI environment.
Recommended:
VERSION=$1
INSTALL_DIR=$2
if [ -z "$VERSION" ] || [ -z "$INSTALL_DIR" ]; then
echo "Usage: $0 <version> <install_dir>"
echo "Example: $0 v0.11.6 ./llgo"
exit 1
fi
# Validate VERSION format
if ! [[ "$VERSION" =~ ^v[0-9]+\.[0-9]+\.[0-9]+(-[a-zA-Z0-9]+)?$ ]]; then
echo "Error: Invalid version format: $VERSION"
echo "Expected format: vX.Y.Z (e.g., v0.11.6)"
exit 1
fi
|
|
||
| # Create install directory | ||
| mkdir -p "$INSTALL_DIR" | ||
|
|
There was a problem hiding this comment.
Use mktemp for secure temporary file handling
Using predictable filenames in /tmp/ can lead to race conditions and symlink attacks.
Recommended:
# Create secure temporary file
TEMP_FILE=$(mktemp /tmp/llgo-download.XXXXXXXXXX)
trap "rm -f '$TEMP_FILE'" EXIT
mkdir -p "$INSTALL_DIR"
echo "Downloading LLGo ${VERSION} for ${OS}-${ARCH}..."
echo "URL: $URL"
curl -fL --retry 3 --retry-delay 2 --max-time 300 -o "$TEMP_FILE" "$URL"
tar -xzf "$TEMP_FILE" -C "$INSTALL_DIR" --strip-components=1This provides atomic temporary file creation and automatic cleanup.
| case $ARCH in | ||
| x86_64) | ||
| ARCH="amd64" | ||
| ;; | ||
| aarch64|arm64) | ||
| ARCH="arm64" | ||
| ;; | ||
| *) |
There was a problem hiding this comment.
Add OS validation
Consider validating the OS as well to provide clear error messages for unsupported platforms:
# Map architecture names
case $ARCH in
x86_64)
ARCH="amd64"
;;
aarch64|arm64)
ARCH="arm64"
;;
*)
echo "Error: Unsupported architecture: $ARCH"
echo "Supported architectures: amd64, arm64"
echo "Detected: $(uname -m) on $(uname -s)"
exit 1
;;
esac
# Validate OS
case $OS in
darwin|linux)
# Supported
;;
*)
echo "Error: Unsupported operating system: $OS"
echo "Supported systems: darwin (macOS), linux"
exit 1
;;
esac
| echo "Binary location: ${INSTALL_DIR}/bin/llgo" | ||
|
|
There was a problem hiding this comment.
Enhance post-installation validation
The current check only verifies file existence. Consider adding:
# Verify installation
if [ ! -f "${INSTALL_DIR}/bin/llgo" ]; then
echo "Error: llgo binary not found at ${INSTALL_DIR}/bin/llgo"
exit 1
fi
# Verify it's executable
if [ ! -x "${INSTALL_DIR}/bin/llgo" ]; then
echo "Error: llgo binary is not executable"
exit 1
fi
# Verify it's a valid binary
if ! file "${INSTALL_DIR}/bin/llgo" | grep -qE "(executable|ELF|Mach-O)"; then
echo "Error: llgo is not a valid binary executable"
exit 1
fi
echo "Installation verified successfully"
ls -lh "${INSTALL_DIR}/bin/llgo"
Code Review SummaryThis PR successfully upgrades to llgo v0.11.6 and simplifies CI by using prebuilt releases instead of compiling from source. The approach will significantly improve CI performance (estimated 5-15 minutes faster per run). Critical Issues to Address:
Recommended Improvements:
Overall, this is a well-executed modernization of the dependency management. Once the checksum verification is added, this will be ready to merge. |
The llgo release tarball has files directly at the root level (bin/llgo, LICENSE, README.md) without a top-level directory, so we should not strip any path components during extraction. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Update the gentest workflow to use llgo v0.11.6 and remove llvm parameter, consistent with other workflow files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add the crosscompile/clang/bin directory from the llgo release to PATH to make LLVM tools like llvm-nm available. These tools are bundled with the llgo release and are required for building. Also added llvm-nm version check to verify installation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit updates the CI configuration to use prebuilt llgo v0.12.0 releases instead of building from source, while maintaining LLVM dependency for clang access. Changes: - Add download-llgo.sh script to fetch prebuilt llgo releases - Update action.yml to use download script instead of source checkout - Keep LLVM/clang dependency as requested in issue #609 - Remove unnecessary LLVM dev packages (llvm-dev, libclang-dev, lld, etc.) - Retain only clang-19 for compilation needs - Add verification step for llgo and clang installation Benefits: - Faster CI builds (no source compilation) - Simpler dependency management - More reliable with official releases - Reduced complexity while keeping clang access Related: #609, #590 Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: luoliwoshang <51194195+luoliwoshang@users.noreply.github.com>
This commit updates the CI configuration to use prebuilt llgo v0.12.0 releases instead of building from source, while maintaining LLVM dependency for clang access. Changes: - Add download-llgo.sh script to fetch prebuilt llgo releases - Update action.yml to use download script instead of source checkout - Keep LLVM/clang dependency as requested in issue #609 - Remove unnecessary LLVM dev packages (llvm-dev, libclang-dev, lld, etc.) - Retain only clang-19 for compilation needs - Add verification step for llgo and clang installation Benefits: - Faster CI builds (no source compilation) - Simpler dependency management - More reliable with official releases - Reduced complexity while keeping clang access Related: #609, #590 Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: xgopilot <noreply@goplus.org> Co-authored-by: luoliwoshang <51194195+luoliwoshang@users.noreply.github.com>
1 participant
Summary
This PR addresses issue #586 by upgrading to llgo v0.11.6 and significantly simplifying the CI configuration.
Changes Made
1. Upgraded llgo dependency
go.modfrom pseudo-version to official v0.11.6 releasego mod tidyto update dependencies2. Created llgo download script
.github/actions/setup-llcppg/download-llgo.sh3. Simplified CI configuration
4. Reduced CI dependencies
llvm@19andlld@19installationbdw-gc,openssl,libffi,libuv,zlibBenefits
Files Changed
.github/actions/setup-llcppg/action.yml- Simplified setup action.github/actions/setup-llcppg/download-llgo.sh- New download script (executable).github/workflows/go.yml- Updated to use v0.11.6.github/workflows/end2end.yml- Updated to use v0.11.6go.mod- Updated llgo dependency to v0.11.6go.sum- Updated checksumsTesting Plan
Related Issues
Closes #586
🤖 Generated with Claude Code