Skip to content

feat: add clang support to llvm-core Conan recipe#513

Merged
zhangxffff merged 8 commits into
bytedance:mainfrom
zhangxffff:feat/add_clang_to_llvm_recipe
Apr 19, 2026
Merged

feat: add clang support to llvm-core Conan recipe#513
zhangxffff merged 8 commits into
bytedance:mainfrom
zhangxffff:feat/add_clang_to_llvm_recipe

Conversation

@zhangxffff
Copy link
Copy Markdown
Collaborator

@zhangxffff zhangxffff commented Apr 17, 2026

Add with_clang option to llvm-core recipe that builds clang alongside LLVM from the same source. This ensures exact version match between clang and LLVM for bitcode compatibility in the pre-built JIT IR framework.

Changes:

  • scripts/conan/patches/llvm-core-add-clang-support.patch: adds with_clang option, clang source download, LLVM_ENABLE_PROJECTS, and clang library exclusion from components
  • conanfile.py: enables with_clang=True when JIT is enabled

What problem does this PR solve?

Issue Number: related #502

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 🚀 Performance improvement (optimization)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🔨 Refactoring (no logic changes)
  • 🔧 Build/CI or Infrastructure changes
  • 📝 Documentation only

Description

Describe your changes in detail.
For complex logic, explain the "Why" and "How".

Performance Impact

  • No Impact: This change does not affect the critical path (e.g., build system, doc, error handling).

  • Positive Impact: I have run benchmarks.

    Click to view Benchmark Results
    Paste your google-benchmark or TPC-H results here.
    Before: 10.5s
    After:   8.2s  (+20%)
    
  • Negative Impact: Explained below (e.g., trade-off for correctness).

Release Note

Please describe the changes in this PR

Release Note:

Release Note:
- Fixed a crash in `substr` when input is null.
- optimized `group by` performance by 20%.

Checklist (For Author)

  • I have added/updated unit tests (ctest).
  • I have verified the code with local build (Release/Debug).
  • I have run clang-format / linters.
  • (Optional) I have run Sanitizers (ASAN/TSAN) locally for complex C++ changes.
  • No need to test or manual test.

Breaking Changes

  • No

  • Yes (Description: ...)

    Click to view Breaking Changes
    Breaking Changes:
    - Description of the breaking change.
    - Possible solutions or workarounds.
    - Any other relevant information.
    

Add with_clang option to llvm-core recipe that builds clang alongside
LLVM from the same source. This ensures exact version match between
clang and LLVM for bitcode compatibility in the pre-built JIT IR
framework.

Changes:
- scripts/conan/patches/llvm-core-add-clang-support.patch: adds
  with_clang option, clang source download, LLVM_ENABLE_PROJECTS,
  and clang library exclusion from components
- conanfile.py: enables with_clang=True when JIT is enabled

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

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

Just one question

Comment on lines +70 to +72
+ if self.options.with_clang:
+ bindir = os.path.join(self.package_folder, "bin")
+ self.buildenv_info.prepend_path("PATH", bindir)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

From the conan documentation at https://docs.conan.io/2/reference/conanfile/methods/package_info.html

It is not necessary to add bindirs to the PATH environment variable, this will be automatically done by the consumer VirtualBuildEnv and VirtualRunEnv generators. Likewise, it is not necessary to add includedirs, libdirs or any other dirs to environment variables, as this information will be typically managed by other generators.

It looks like this is not necessary?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Actually, I am mistaken as bindir here is just the variable. But maybe LLVM should export it's bindir to the conan package so that the binaries are propagated?

On the same page, the example is

self.cpp_info.bindirs = ['bin']  # Directories where executables and shared libs can be found

Conan's default cpp_info.bindirs=['bin'] already propagates the
bin directory to consumers via VirtualBuildEnv. No need to manually
prepend PATH.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ZacBlanco
Copy link
Copy Markdown
Collaborator

@zhangxffff Have you tested that the new recipe works for PR #501 ? Also, I think something is not correct with the patch, because I would expect that adding this patch would cause the CI to rebuild the llvm-core recipe, but it seems like it's not rebuilding it. Can you look into that?

@zhangxffff
Copy link
Copy Markdown
Collaborator Author

zhangxffff commented Apr 17, 2026

@zhangxffff Have you tested that the new recipe works for PR #501 ? Also, I think something is not correct with the patch, because I would expect that adding this patch would cause the CI to rebuild the llvm-core recipe, but it seems like it's not rebuilding it. Can you look into that?

For the question before, we use LLVM_TOOLS_BINARY_DIR in CMakeLists.txt to locate clang, so you should be right, adding it to PATH is not necessary.
https://github.com/conan-io/conan-center-index/blob/f7e534b5a37d22627b73f7a5e009a5eb20a1d4ed/recipes/llvm-core/all/conanfile.py#L468

The reason llvm-core does not rebuild is that Conan appears to treat the remote version and the newly patched version as having the same recipe ID, even though we added a with_clang option. So in #501, I renamed the llvm-core version to 19.1.7-clang to force a rebuild. I will test in #501 to make sure the latest changes work. After this patch is merged, I will rename it back to 19.1.7.

@zhangxffff
Copy link
Copy Markdown
Collaborator Author

@zhangxffff Have you tested that the new recipe works for PR #501 ? Also, I think something is not correct with the patch, because I would expect that adding this patch would cause the CI to rebuild the llvm-core recipe, but it seems like it's not rebuilding it. Can you look into that?

OK, it seems that modify patch does not retigger CI, I will look at it.

zhangxffff and others added 5 commits April 17, 2026 04:52
Add --index=0 to local-recipes-index remotes so patched recipes
take precedence over pre-built packages on CI remotes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a script that detects which Conan recipes are changed in the
current PR (from CCI patches or bolt-local recipes), copies only
those into an override remote at index 0. This ensures modified
recipes rebuild from source while everything else reuses cached
binaries from the CI remote.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tilde does not expand inside ${VAR:-~/.conan2}, causing [ -d ] and
cp -r to receive a literal ~ path and fail silently.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fail early if clang binary is missing from the llvm-core package
when JIT is enabled. Version check is handled in CMakeLists.txt.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@zhangxffff
Copy link
Copy Markdown
Collaborator Author

@ZacBlanco The reason we cannot trigger a rebuild for llvm-core is that the remote Conan server was added with --index=0, giving it the highest priority when resolving llvm-core/19.1.7. Once Conan finds a matching version on the remote server, it uses that instead of the recipe we patched in bolt-cci-local.

  CI adds its remote at index 0 (highest priority):
  0: ci              ← pre-built binaries (stale, unpatched recipes)
  1: bolt-local      ← custom recipes
  2: bolt-cci-local  ← patched CCI checkout
  3: conancenter

I tried adding --index=0 to bolt-cci-local, but that would cause all recipes to be built from source, since bolt-cci-local contains no prebuilt binaries.

So what we do now is add a step in the GitHub Action to extract modified recipes from the changed code and move them into a separate local Conan server bolt-patched with the highest priority. This way, only the modified recipes will be rebuilt.

Result for a PR that modifies llvm-core:

  0: bolt-patched    ← only llvm-core (different revision → rebuild)
  1: ci              ← everything else (cached binaries → reused)
  2: bolt-local
  3: bolt-cci-local
  4: conancenter

Action output shows that llvm-core was successfully rebuilt.

Run ./.github/actions/bolt-build-base/override-changed-recipes.sh "${GITHUB_BASE_REF:-main}"
✅ Override recipes: llvm-core

@zhangxffff zhangxffff enabled auto-merge April 17, 2026 09:04
@zhangxffff zhangxffff requested a review from ZacBlanco April 17, 2026 09:04
Copy link
Copy Markdown
Collaborator

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

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

The recipe changes themselves LGTM, but I think the patch should just update the llvm-core recipe version as well so we don't conflict with the OSS conan recipe version.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this script is too complex to be worth committing and maintaining. I would rather have the patch update the LLVM-core recipe version to 19.1.7-bolt instead, and then just have bolt's conanfile reference 19.1.7-bolt. No external conan remote will have that version, so it will be forced to get picked from the CCI local recipe index.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I agree with updating the version for the llvm-core recipe. However, without this script, changes to scripts/conan/patches or scripts/conan/recipes would not trigger a rebuild in CI unless the version is also updated. Since we always upload used conan recipes to the ci server, and it is always searched first, this overrides the CCI local recipe index and prevents us from picking up recipes from CCI local even we have a version that differs from official conan server.

- name: Upload packages to conan remote
env:
CONAN_LOGIN_USERNAME: ${{ secrets.CONAN_ADMIN_USERNAME }}
CONAN_PASSWORD: ${{ secrets.CONAN_ADMIN_PASSWORD }}
run: |
conan remote auth ci
conan upload -r ci --confirm "*"

If we remove this script, we would need to establish a convention that any modification to a recipe must be accompanied by a corresponding version update to ensure CI rebuilds the changed recipe.

Copy link
Copy Markdown
Collaborator

@ZacBlanco ZacBlanco Apr 17, 2026

Choose a reason for hiding this comment

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

I think having a convention where we update the recipe version is not a bad, but is somewhat tedious. I guess I am ok with this script for now then. If it presents issues in the future, we might want to look at other solutions.

Anyways, we can merge this then. Can you test on your other PR to make sure that this recipe works before merging though?

Rename the patched CCI recipe to 19.1.7-bolt so the version itself
disambiguates from upstream conan-center-index, removing the need for
a separate CI script that detects and overrides modified recipes.
Recipe/patch changes now require a corresponding version bump to
trigger a rebuild.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ZacBlanco ZacBlanco disabled auto-merge April 17, 2026 17:35
@ZacBlanco
Copy link
Copy Markdown
Collaborator

@zhangxffff once you can confirm the recipe works for #501 we can merge this, upload the new recipe to CI conan server, and then you can rebase 501

@zhangxffff zhangxffff added this pull request to the merge queue Apr 19, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 19, 2026
@zhangxffff zhangxffff added this pull request to the merge queue Apr 19, 2026
Merged via the queue into bytedance:main with commit cab6ca5 Apr 19, 2026
7 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.

2 participants