Skip to content

feat: build ROOT against external llvm - #288

Open
wdconinc wants to merge 8 commits into
masterfrom
root-external-llvm
Open

feat: build ROOT against external llvm#288
wdconinc wants to merge 8 commits into
masterfrom
root-external-llvm

Conversation

@wdconinc

@wdconinc wdconinc commented May 14, 2026

Copy link
Copy Markdown
Contributor

Briefly, what does this PR introduce? Please link to any relevant presentations or discussions.

ROOT includes its version of llvm, but it should be possible to build against an external llvm. With ROOT 6.36, that external llvm is at the same level as what ROOT requires. This PR enables the external llvm build.

Needs:

What is the urgency of this PR?

  • High (please describe reason below)
  • Medium
  • Low

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue: ROOT with external llvm)
  • Optimization (issue #__)
  • Updated documentation
  • other: __

Please check if any of the following apply

  • This PR introduces breaking changes. Please describe changes users need to make below.
  • This PR changes default behavior. Please describe changes below.
  • AI was used in preparing this PR. Please describe usage below.

Copilot AI review requested due to automatic review settings May 14, 2026 21:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Enables building ROOT against an external LLVM (instead of ROOT's bundled copy) by adding ~builtin_llvm to the ROOT require list, and pulls in the corresponding spack-packages cherry-pick that introduces the builtin_llvm variant.

Changes:

  • Add cherry-pick 8cd9959e to spack-packages.sh that adds the builtin_llvm variant to the ROOT package recipe.
  • Add ~builtin_llvm to the ROOT requirements in spack-environment/packages.yaml to use the external LLVM.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
spack-packages.sh Adds new spack-packages cherry-pick hash and matching descriptive comment for the ROOT builtin_llvm variant.
spack-environment/packages.yaml Adds ~builtin_llvm to ROOT's required variants, switching to external LLVM.

Copilot AI review requested due to automatic review settings May 15, 2026 04:06

This comment was marked as duplicate.

@wdconinc
wdconinc force-pushed the root-external-llvm branch from 901a1d3 to ac164f0 Compare May 15, 2026 20:58
Copilot AI review requested due to automatic review settings May 16, 2026 00:05

This comment was marked as resolved.

Copilot AI review requested due to automatic review settings May 16, 2026 15:43

This comment was marked as duplicate.

Copilot AI review requested due to automatic review settings May 18, 2026 20:27
@wdconinc
wdconinc force-pushed the root-external-llvm branch from 52caa62 to 2b36d4d Compare May 18, 2026 20:29

This comment was marked as duplicate.

Copilot AI review requested due to automatic review settings May 18, 2026 20:29

This comment was marked as duplicate.

This comment was marked as duplicate.

@wdconinc
wdconinc force-pushed the root-external-llvm branch from 4881ab5 to e25b423 Compare May 23, 2026 22:43
Copilot AI review requested due to automatic review settings July 4, 2026 16:40
@wdconinc
wdconinc force-pushed the root-external-llvm branch from e25b423 to 7e5e143 Compare July 4, 2026 16:40
@wdconinc
wdconinc removed the request for review from Copilot July 4, 2026 16:41
@wdconinc

wdconinc commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

This fails due to mixing of gcc libstdc++ and llvm libc++ symbols.

Copilot AI review requested due to automatic review settings August 1, 2026 03:18
@wdconinc
wdconinc force-pushed the root-external-llvm branch from 7e5e143 to 173f75b Compare August 1, 2026 03:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread spack-environment/packages.yaml
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 1, 2026 15:54

This comment was marked as duplicate.

Copilot AI review requested due to automatic review settings August 3, 2026 03:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

containers/debian/Dockerfile:313

  • The comment above says both gcc-ar and gcc-ranlib should be used for LTO-safe archives, but the yq mutation only sets GCC_AR. If ranlib is invoked during archive creation/indexing, builds can still hit the same LTO plugin issue. Set GCC_RANLIB alongside GCC_AR in the external GCC environment block.
      "set": {
        "GCC_AR": "gcc-ar"
      }

Copilot AI review requested due to automatic review settings August 3, 2026 14:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

spack-packages.sh:53

  • The new ROOT cherry-pick description line starts with $$ instead of the established ## comment prefix. Because this line is inside the SPACKPACKAGES_CHERRYPICKS_FILES heredoc, it will be treated as data (not a comment) and can break downstream parsing that expects only ## ... comment lines plus the optional hash: files mappings.
## 438abd1e09f0cea9cb0ccc7e0326adcd0408196b: herwig3: add CT14(?n)lo pdfsets as build-time resources
## 4dc856a13e9066ca3249a593d351af8cda521fa3: root: add v6.40.00, v6.40.02
$$ e41e172d579e30881556873fe32f8408f6647800: root: allow ~builtin_llvm with 6.40 and newer

Comment thread spack-packages.sh Outdated
Copilot AI review requested due to automatic review settings August 3, 2026 14:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

containers/debian/Dockerfile:325

  • The yq edit for LLVM externals assumes .packages.llvm.externals[] exists; if spack external find llvm doesn’t register a system LLVM (e.g., missing llvm-config/dev package or discovery changes), this fails with a low-signal jq error. Since root~builtin_llvm relies on a usable external LLVM, fail with an explicit diagnostic so build failures are easier to interpret.
yq -iy '.packages.llvm.externals[] |= (
  .spec += " targets=all" |
  .extra_attributes += {
    "environment": {
      "set": {

containers/debian/Dockerfile:316

  • The yq edit for GCC externals assumes .packages.gcc.externals[] exists; if spack external find --not-buildable gcc doesn’t populate an externals list (e.g., discovery failure or format change), this will fail with a cryptic jq error (Cannot iterate over null). Wrap the yq call to emit a clear diagnostic before failing the build, so troubleshooting is actionable.

This issue also appears on line 321 of the same file.

yq -iy '.packages.gcc.externals[] |= (
  .extra_attributes += {
    "environment": {
      "set": {
        "GCC_AR": "gcc-ar"

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