Skip to content

Conversation

luoliwoshang
Copy link
Member

fixed #1292

Copy link

niupilot bot commented Sep 11, 2025

PR Review Analysis

Todo List:

Review Summary

This PR updates the LLVM/ESP Clang toolchain from version 19.1.2 to 20.x to fix issue #1292, which was causing ESP32 compilation failures due to undefined temporary symbols.

Code Analysis

Positive Aspects

  1. Consistent Version Updates: All three files are properly updated with matching version numbers:

    • .github/workflows/download_esp_clang.sh:4-5 - Updated ESP_CLANG_VERSION and BASE_URL
    • internal/crosscompile/crosscompile.go:52-53 - Updated espClangBaseUrl and espClangVersion
    • .goreleaser.yaml:30,46,63,81 - Updated LLVM library references from -lLLVM-19 to -lLLVM-20
  2. Repository Change: Switched from goplus/espressif-llvm-project-prebuilt to luoliwoshang/espressif-llvm-project-prebuilt repository, suggesting this may be a temporary location for the LLVM 20 builds.

  3. Addresses Root Cause: Based on issue llgo/embed: Undefined temporary symbol .L� error code #1292, this upgrade specifically fixes the "undefined temporary symbol" problem that occurred with ESP32 compilation using LLVM 19.

⚠️ Areas of Concern

  1. Dependency Source Change:

    • File: .github/workflows/download_esp_clang.sh:5, internal/crosscompile/crosscompile.go:52
    • Issue: The base URL switched from the official goplus organization to luoliwoshang user repository
    • Risk: This introduces a dependency on a personal repository rather than the official one
    • Recommendation: Consider if this is temporary and document the plan to migrate back to official releases
  2. Quote Style Change:

    • File: .goreleaser.yaml:58,76
    • Change: -extldflags quotes changed from single quotes '-extldflags=...' to double quotes "-extldflags=..."
    • Impact: While functionally equivalent, this is an unrelated cosmetic change that should be mentioned in commit message

🔍 Technical Validation

  1. Version Consistency: ✅ All references to LLVM 19 have been properly updated to LLVM 20
  2. URL Pattern: ✅ The version string format changed from 19.1.2_20250905-3 to 20.x_20250911-1, which follows a logical pattern
  3. Build Configuration: ✅ GoReleaser configurations are correctly updated for all platforms (darwin-amd64, darwin-arm64, linux-amd64, linux-arm64)

📚 Documentation

Recommendations

  1. Consider adding to commit message: Mention the repository change from goplus to luoliwoshang and whether this is temporary
  2. Future planning: Document migration plan back to official repository when LLVM 20 builds become available there
  3. Testing: Ensure ESP32 compilation that previously failed in issue llgo/embed: Undefined temporary symbol .L� error code #1292 now works with these changes

Final Assessment

This is a well-executed upgrade that systematically updates all necessary components to resolve a critical compilation issue. The changes are minimal, focused, and address the specific problem described in issue #1292.

Approval Status: ✅ Recommended for approval - The changes properly fix the reported issue and maintain consistency across the codebase.


👉 Want me to re-run this review? Just type /review in the comments! For more usage examples, visit CodeAgent GitHub Repository.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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 focuses on upgrading the underlying LLVM compiler infrastructure used by the project. The changes involve updating build configurations to leverage the newer LLVM 20 libraries and adjusting the cross-compilation toolchain to a compatible version. This update addresses a reported issue and ensures the project remains current with its compiler dependencies.

Highlights

  • LLVM Version Upgrade: The project's build configuration has been updated to link against LLVM 20 libraries instead of LLVM 19 across all supported platforms (macOS AMD64/ARM64, Linux AMD64/ARM64).
  • Cross-Compilation Toolchain Update: The ESP-IDF Clang cross-compilation toolchain references have been updated to a newer version (20.x) and point to a different prebuilt repository, addressing issue #1292.
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

  1. 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.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to upgrade the compiler to use LLVM 20. The changes involve updating LLVM library versions in the GoReleaser configuration and modifying the source URL for ESP-Clang prebuilt binaries. While the LLVM version update seems correct, the change to use a personal repository for ESP-Clang introduces a potential risk that should be addressed.

Comment on lines +52 to +53
espClangBaseUrl = "https://github.com/luoliwoshang/espressif-llvm-project-prebuilt/releases/download/20.x_20250911-1"
espClangVersion = "20.x_20250911-1"

Choose a reason for hiding this comment

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

high

The base URL for esp-clang has been changed to a personal GitHub repository (luoliwoshang). While this might be for development purposes, as the PR is a work in progress, relying on a personal fork for dependencies introduces potential stability and security risks. For production-readiness, it's highly recommended to use pre-built binaries from an official source or host them under the goplus organization. If this is a temporary measure, please add a comment explaining the situation and a TODO to track switching to an official source.

Copy link

codecov bot commented Sep 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.22%. Comparing base (d5ad4d9) to head (f11f7bc).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1294   +/-   ##
=======================================
  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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@luoliwoshang luoliwoshang marked this pull request as draft September 12, 2025 02:33
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.

llgo/embed: Undefined temporary symbol .L� error code
1 participant