Remove Ubuntu 20.04 job from GitHub Actions [circle skip]#790
Conversation
WalkthroughThis change updates the GitHub Actions CI configuration by expanding the build matrix in Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Job Matrix
participant Build Steps
GitHub Actions->>Job Matrix: Expand with Clang 11, 12, GCC 10 (Release/Debug, Sanitizers)
Job Matrix->>Build Steps: For each configuration, run checkout, setup, build, and test
Build Steps->>Build Steps: Install dependencies (LLVM/GCC) as per version
Build Steps->>Build Steps: Configure and build with CMake/make
Build Steps->>Build Steps: If CMake fails, print CMakeError.log and CMakeOutput.log, then exit
Build Steps->>Build Steps: Run tests and benchmarks
Possibly related PRs
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/old-compilers.yml(4 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
.github/workflows/old-compilers.yml (1)
644-656:Details
✅ Verification successful
Verify numeric comparisons and package coverage.
- GitHub Actions treats env vars as strings;
env.VERSION <= 13may perform lexicographical comparison. Confirm this works for all version values.- The
llvm-toolchainrepo step installsclang-${VERSION}(C compiler) but doesn’t guaranteeclang++-${VERSION}. Make sure C++ compiler is available for CMake.
🌐 Web query:
Does GitHub Actions compare env.VERSION <= 13 numerically or lexicographically?💡 Result:
When you compare values like (\text{env.VERSION} <= 13) in GitHub Actions, the evaluation depends on the data type of (\text{env.VERSION}).
By default, all environment variables in GitHub Actions (such as (\text{env.VERSION})) are strings. If you write a comparison like:
if: env.VERSION <= 13GitHub Actions will generally attempt to coerce both sides to numbers if possible. According to the documentation, when you use relational operators ((<, >, <=, >=)), GitHub Actions first tries to interpret string values as numbers using a loose conversion (via
fromJSON()internally). This means:
- If (\text{env.VERSION}) is the string "12", it will be parsed as the number 12 and compared numerically.
- If (\text{env.VERSION}) is the string "13", it will become the number 13, and "13" <= 13 evaluates to true.
- If (\text{env.VERSION}) is "13.1", "14", or "12.9", it is compared as a number, so "12.9" <= 13 is true, but "14" <= 13 is false.
- If (\text{env.VERSION}) is a non-numeric string (like "beta"), it will be converted to NaN, and any comparison with NaN is false[7].
If the string cannot be converted to a number (for example, "v1.2.3"), the value becomes NaN, and comparisons like (\text{env.VERSION} <= 13) always result in false[7].
Lexicographical vs. Numerical Comparison:
- Numerical: GitHub Actions attempts numerical comparison if both operands can be parsed as numbers[7].
- Lexicographical: If either value is not a numeric string (e.g., version strings like "1.10.0" vs "1.2.0"), coercion fails, resulting in NaN, so relational comparisons do not work as expected in those cases[7][10].
For comparing actual semantic version numbers (like "1.10.0" vs "1.2.0"), you should use a dedicated version comparison tool or action (e.g., actions that handle semantic versioning), because numeric coercion will not yield correct results for multi-part versions[1][3][6].
Summary Table
env.VERSION Value Comparison Result type Example outcome "13" <= 13 Numeric (13 <= 13) true "12" <= 13 Numeric (12 <= 13) true "13.1" <= 13 Numeric (13.1 <= 13) false "beta" <= 13 NaN comparison false "1.10.0" <= 13 NaN comparison false Best Practice
If you need to compare semantic versions or dotted strings, use a dedicated action or script that correctly parses and compares version components, since simple comparisons in GitHub Actions do not handle versions lexicographically or semantically[1][3][6].
In summary: GitHub Actions compares (\text{env.VERSION} <= 13) numerically if possible. If (\text{env.VERSION}) cannot be coerced to a number, the result is always false, and lexicographical comparison is not performed by default[7][10].
Citations:
- 1: https://github.com/marketplace/actions/semantic-version-comparator
- 2: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/viewing-and-comparing-commits/comparing-commits
- 3: https://github.com/marketplace/actions/check-version
- 4: https://docs.github.com/enterprise-cloud@latest/repositories/releasing-projects-on-github/comparing-releases
- 5: https://github.com/orgs/community/discussions/48058
- 6: https://github.com/Rombecchi/version-compare
- 7: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions
- 8: https://github.com/marketplace/actions/version-check
- 9: https://www.updatecli.io/docs/plugins/resource/github_release/
- 10: https://github.com/orgs/community/discussions/57480
🏁 Script executed:
#!/bin/bash set -e # Locate matrix definition and VERSION usages in the workflow rg -n "matrix" .github/workflows/old-compilers.yml || true rg -n "VERSION" .github/workflows/old-compilers.ymlLength of output: 4236
Both numeric comparisons and C++ compiler are already covered
- GitHub Actions relational operators coerce string values like
"10"–"19"to numbers, so(env.VERSION <= 13 || env.VERSION >= 15)works as intended.- The
clang‑${VERSION}meta‐package from apt.llvm.org provides bothclang-${VERSION}and theclang++-${VERSION}driver, so you get out‑of‑the‑box C++ support for CMake.No changes required here.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #790 +/- ##
==========================================
- Coverage 98.38% 98.23% -0.15%
==========================================
Files 35 35
Lines 7040 7040
==========================================
- Hits 6926 6916 -10
- Misses 114 124 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
from
1921180 to
a66f5c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
.github/workflows/old-compilers.yml (4)
38-83: Matrix enumeration for Clang 11 is excessively verbose
Manual listing of Release/Debug + ASan/TSan/UBSan combinations for Clang 11 is error‑prone and hard to maintain. Consider using YAML anchors or generating the matrix dynamically to DRY this up.
84-129: Matrix enumeration for Clang 12 is excessively verbose
Similar to Clang 11, the Clang 12 combinations could be consolidated via YAML anchors or a script-generated matrix to reduce duplication and maintenance overhead.
472-517: GCC 10 matrix entries look correct but are verbose
As noted previously, the manual enumeration for GCC 10 (all build types and sanitizers) can be DRYed with anchors or a generated matrix. This will improve readability and reduce the risk of drift when adding new variants.
659-665: Install matching C++ compiler for versioned GCC
Installing onlygcc-${VERSION}omits theg++-${VERSION}package, which is required forCXX="g++-${VERSION}".Apply this diff:
- sudo apt-get install -y "gcc-${VERSION}" + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/old-compilers.yml(3 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
⏰ Context from checks skipped due to timeout of 90000ms (30)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 14 Debug with UBSan, Debug, ON, clang, 14, OFF, OFF)
- GitHub Check: build (MSVC Debug without AVX2, msvc-debug-no-avx2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 14 Debug with UBSan, Debug, ON, clang, 14, OFF, OFF)
- GitHub Check: build (MSVC Debug without AVX2, msvc-debug-no-avx2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 14 Debug with UBSan, Debug, ON, clang, 14, OFF, OFF)
- GitHub Check: build (MSVC Debug without AVX2, msvc-debug-no-avx2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 14 Debug with UBSan, Debug, ON, clang, 14, OFF, OFF)
- GitHub Check: build (MSVC Debug without AVX2, msvc-debug-no-avx2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 14 Debug with UBSan, Debug, ON, clang, 14, OFF, OFF)
- GitHub Check: build (MSVC Debug without AVX2, msvc-debug-no-avx2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 14 Debug with UBSan, Debug, ON, clang, 14, OFF, OFF)
- GitHub Check: build (MSVC Debug without AVX2, msvc-debug-no-avx2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 14 Debug with UBSan, Debug, ON, clang, 14, OFF, OFF)
- GitHub Check: build (MSVC Debug without AVX2, msvc-debug-no-avx2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 14 Debug with UBSan, Debug, ON, clang, 14, OFF, OFF)
- GitHub Check: build (MSVC Debug without AVX2, msvc-debug-no-avx2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 14 Debug with UBSan, Debug, ON, clang, 14, OFF, OFF)
- GitHub Check: build (MSVC Debug without AVX2, msvc-debug-no-avx2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 14 Debug with UBSan, Debug, ON, clang, 14, OFF, OFF)
- GitHub Check: build (MSVC Debug without AVX2, msvc-debug-no-avx2)
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
from
a66f5c7 to
8959c3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
.github/workflows/old-compilers.yml (5)
659-665: Installg++-${VERSION}alongsidegcc-${VERSION}
CMake will invokeg++-${VERSION}and fail if it’s not present. Add the C++ package to the install.- sudo apt-get install -y "gcc-${VERSION}" + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}"
38-83: 🧹 Nitpick (assertive)Matrix enumeration for clang 11 is overly verbose
You’ve manually listed all Release/Debug + sanitizers for Clang 11. This is error‑prone and hard to maintain—consider using YAML anchors or a generated matrix to DRY up the entries.
84-129: 🧹 Nitpick (assertive)Matrix enumeration for clang 12 is overly verbose
Same pattern as Clang 11: you can collapse these entries with anchors or a dynamic matrix generation approach.
472-517: 🧹 Nitpick (assertive)Matrix enumeration for GCC 10 is overly verbose
All Release/Debug + sanitizers for GCC 10 are hand‑listed. To reduce duplication and simplify updates, switch to YAML anchors or a script‑generated matrix.
644-652: 🧹 Nitpick (assertive)Use non‑deprecated key import for LLVM repo
apt-key addis deprecated on newer Ubuntu releases. Import the GPG key withgpg --dearmorinto/etc/apt/trusted.gpg.d/and reference it viasigned-byin your source list.- curl 'https://apt.llvm.org/llvm-snapshot.gpg.key' | sudo apt-key add - - sudo add-apt-repository -y "deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${VERSION} main" + curl 'https://apt.llvm.org/llvm-snapshot.gpg.key' \ + | gpg --dearmor \ + | sudo tee /usr/share/keyrings/llvm-archive-keyring.gpg > /dev/null + echo "deb [signed-by=/usr/share/keyrings/llvm-archive-keyring.gpg] \ + http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${VERSION} main" \ + | sudo tee /etc/apt/sources.list.d/llvm.list
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(3 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
.github/workflows/old-compilers.yml (1)
653-658: No changes needed here
LLVM package install step looks correct for all Clang versions.
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
from
8959c3f to
e51ea2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
.github/workflows/old-compilers.yml (4)
38-129: 🧹 Nitpick (assertive)Matrix enumeration is excessively verbose
The manual listing of every combination for Clang 11 and Clang 12 (Release/Debug + ASan/TSan/UBSan) is error‑prone and hard to maintain. Consider DRYing this up using YAML anchors or dynamically generating the matrix.
472-517: 🧹 Nitpick (assertive)Verbose GCC 10 matrix entries; consider DRYing up
Similar to Clang, the GCC 10 variants are explicitly enumerated. You could leverage YAML anchors or a small script to generate these entries, reducing duplication and easing future updates.
644-652: 🛠️ Refactor suggestionAvoid deprecated
apt-key
apt-key addis deprecated on Ubuntu 22.04. For better security and forward compatibility, import the key withgpg --dearmorinto/etc/apt/trusted.gpg.d:- curl 'https://apt.llvm.org/llvm-snapshot.gpg.key' \ - | sudo apt-key add - + curl 'https://apt.llvm.org/llvm-snapshot.gpg.key' \ + | gpg --dearmor \ + | sudo tee /etc/apt/trusted.gpg.d/llvm-snapshot.gpg > /dev/null
659-665:⚠️ Potential issueMissing
g++package for GCC 10 & 12
Onlygcc-${VERSION}is installed, but CMake setsCXX=g++-${VERSION}. The absence ofg++-${VERSION}will break the C++ build. Please install both:- sudo apt-get install -y "gcc-${VERSION}" + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(3 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
⏰ Context from checks skipped due to timeout of 90000ms (40)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Debug with TSan, Debug, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Debug with TSan, Debug, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Debug with TSan, Debug, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Debug with TSan, Debug, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Debug with TSan, Debug, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Debug with TSan, Debug, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Debug with TSan, Debug, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Debug with TSan, Debug, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Debug with TSan, Debug, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Debug with TSan, Debug, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
from
e51ea2d to
17ac3de
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
.github/workflows/build.yml (1)
416-424: Typo in CMakeOutput log group name
The group label is"CMakeOutput.Log", but the file isCMakeOutput.log. This mismatch hides the output in the Actions UI.- echo "::group::CMakeOutput.Log" + echo "::group::CMakeOutput.log".github/workflows/old-compilers.yml (5)
38-83: 🧹 Nitpick (assertive)Verbose matrix for Clang 11
You've manually enumerated every Release/Debug + sanitizer combination for Clang 11, which is error‑prone and hard to maintain. Consider using YAML anchors or generating this matrix dynamically to reduce duplication.
84-129: 🧹 Nitpick (assertive)Verbose matrix for Clang 12
The Clang 12 section repeats the same patterns as Clang 11. Applying YAML anchors across compilers/sanitizers will DRY up the configuration and simplify future updates.
472-517: 🧹 Nitpick (assertive)Verbose matrix for GCC 10
Similarly, the GCC 10 entries enumerate each sanitizer + build type manually. Using YAML anchors or a template-based script can dramatically reduce repetition and maintenance overhead.
644-652: 🧹 Nitpick (assertive)Avoid deprecated
apt-keyusage
You import the LLVM GPG key withapt-key add, which is deprecated on Ubuntu 22.04+. To future‑proof and harden security, usegpg --dearmorinto/etc/apt/trusted.gpg.d/and mark the source entry withsigned-by.
659-665:⚠️ Potential issueInstall missing C++ compiler for GCC 10 & 12
The step only installsgcc-${VERSION}, but CMake setsCXX="g++-${VERSION}". Withoutg++-${VERSION}, builds will fail.- sudo apt-get install -y "gcc-${VERSION}" + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(3 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
4 times, most recently
from
67725ee to
593b393
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/old-compilers.yml (2)
38-129: Matrix enumeration still excessively verbose. You've manually duplicated every combination for Clang 11 and 12, which is error‑prone and hard to maintain. Consider using YAML anchors or generating the matrix dynamically to DRY up this section.
472-517:⚠️ Potential issueCritical: Missing C++ compiler packages for GCC 10 & 12. You install only
gcc-${VERSION}, but later setCXX=g++-${VERSION}. This will break C++ builds.
Apply this diff:- sudo apt-get install -y "gcc-${VERSION}" + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(4 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
🔇 Additional comments (2)
.github/workflows/build.yml (1)
406-428: CMake error-handling improvement is solid. The snippet correctly disablesset -e, capturesCMAKE_EXIT_CODE, re-enables fail‑fast, and printsbuild/CMakeFileslogs with accurate group labels..github/workflows/old-compilers.yml (1)
700-722: CMake configuration error handling aligns with the main build. The block correctly mirrors thebuild.ymlimprovements: disabling exit, capturing the code, printing logs, and restoringset -e. Looks good.
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
from
593b393 to
439a577
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/old-compilers.yml (2)
38-129: Matrix enumeration is excessively verbose; DRY it up.
There’s a lot of repetitive blocks for Clang 11 and 12 (Release/Debug with sanitizers). Consider using YAML anchors, reusable snippets, or generating the matrix dynamically to reduce duplication and maintenance overhead.
659-665:⚠️ Potential issueMissing C++ compiler for GCC 10 & 12.
The step installs onlygcc-${VERSION}, but CMake will invokeg++-${VERSION}. Please includeg++-${VERSION}to ensure the C++ compiler is available:- sudo apt-get install -y "gcc-${VERSION}" + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(4 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
🔇 Additional comments (1)
.github/workflows/build.yml (1)
406-428: CMake error-handling enhancement looks solid.
The new block correctly disablesset -e, capturesCMAKE_EXIT_CODE, restoresset -e, and prints bothbuild/CMakeFiles/CMakeError.logandbuild/CMakeFiles/CMakeOutput.logwithin log groups on failure. This greatly improves diagnostic output.
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
from
439a577 to
5f43f51
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/old-compilers.yml (3)
38-129: 🧹 Nitpick (assertive)Matrix enumeration for Clang 11/12 is excessively verbose
Manually listing all variants (Release/Debug + sanitizers) for Clang 11/12 bloats this file and is error‑prone. Consider using YAML anchors or generating the matrix dynamically to DRY this up.
644-652: 🧹 Nitpick (assertive)APT key import is deprecated
apt-key addis deprecated on Ubuntu 22.04. Switch togpg --dearmorand place the key in/etc/apt/trusted.gpg.d/, then reference it withsigned-byin your APT source entry.
659-665:⚠️ Potential issueInstall C++ compiler for GCC 10/12
Onlygcc-${VERSION}is installed, but CMake usesg++-${VERSION}for CXX. Addg++-${VERSION}or the C++ toolchain step will fail.Apply this diff:
- sudo apt-get install -y "gcc-${VERSION}" + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(4 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
🔇 Additional comments (5)
.github/workflows/build.yml (2)
406-415: Well done: Capture and restore shell failure settings around CMake
CapturingCMAKE_EXIT_CODEwithset +eand re-enablingset -eis solid for error diagnostics.
416-428: Nice: Printing CMake logs on failure
Failing build now dumpsbuild/CMakeFiles/CMakeError.logandCMakeOutput.logfor easier debugging..github/workflows/old-compilers.yml (3)
653-658: Confirm LLVM package set includes linker tools and tidy
Installingllvm-${VERSION}-linker-tools,clang-${VERSION}, andclang-tidy-${VERSION}covers the needed toolchain components.
700-708: Capture CMake exit code for old compilers
Disablingset -ebeforecmakeand capturing its exit code enhances diagnostics for legacy builds.
709-722: Dump CMake logs on configuration failures
Printingbuild/CMakeFiles/CMakeError.logandCMakeOutput.logwhen configuration fails boosts troubleshooting efficiency.
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
3 times, most recently
from
c96a818 to
f8bec76
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/old-compilers.yml (3)
472-517: Matrix enumeration for GCC 10 is inconsistent and verbose
The GCC 10 variant matrix (lines 472–517) mirrors the Clang blocks and bloats the workflow. Reuse anchors or a dynamic generation approach to streamline these entries and ensure consistency across compilers.
644-652: Avoid deprecatedapt-key addfor LLVM key import
Usingapt-key addis deprecated on Ubuntu 22.04+. Replace this with agpg --dearmorimport into/etc/apt/trusted.gpg.d/and reference the key viasigned-byin the APT source entry.
659-665: Install missing C++ packages for GCC 10 & 12
Onlygcc-${VERSION}is installed for versions 10 and 12, but CMake later invokesg++-${VERSION}. Addg++-${VERSION}to the install line to prevent missing compiler errors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(5 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
🔇 Additional comments (2)
.github/workflows/old-compilers.yml (2)
676-676: Enable verbose tracing in CMake setup
The addition ofset -xat line 676 will help trace all configuration commands. This is a useful change for debugging setup issues.
701-723: Enhanced CMake configuration diagnostics for old compilers
Disabling fail‑fast (set +e), capturing the exit code, listing the entirebuilddirectory, and grouping theCMakeError.logandCMakeOutput.logoutputs will significantly improve investigation speed for configuration failures across older compiler versions.
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
2 times, most recently
from
b83d182 to
88e2b19
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
.github/workflows/old-compilers.yml (1)
676-720: 🧹 Nitpick (assertive)Enhance CMake failure diagnostics with grouping
Capturing the exit code and printingCMakeConfigureLog.yamlis solid. Consider wrapping thels -alR build/output in its own::group::build directory::endgroup::block for consistency and easier navigation in the logs.
♻️ Duplicate comments (2)
.github/workflows/build.yml (1)
406-427: Consider listing the build directory when CMake fails
Addingls -alR build/before the log groups can help surface missing files or unexpected structure if errors don’t generate logs..github/workflows/old-compilers.yml (1)
659-665:⚠️ Potential issueInstall
g++-${VERSION}alongsidegcc-${VERSION}
Currently only the C compiler is installed for versioned GCC (10/12), but CMake setsCXX="g++-${VERSION}". This will break C++ builds.Apply this diff:
- sudo apt-get install -y "gcc-${VERSION}" + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(5 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)CMakeLists.txt(2 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
🔇 Additional comments (5)
.github/workflows/build.yml (1)
406-427: Improved CMake diagnostic handling
The newset +e/exit code capture and grouped log printing greatly enhance failure visibility. The log paths and group labels correctly referencebuild/CMakeFiles/....CMakeLists.txt (3)
31-33: Addis_clang11helper for version-specific logic
Introducingis_clang11simplifies conditional checks for the Clang 11 workaround. Well-scoped and consistent with existing patterns.
304-308: Apply UBSan workaround for Clang 11 when enabled
Appending:use_sigaltstack=falseunderif(is_clang11)addresses issue #55785 cleanly. Good to see the workaround scoped to the relevant branch.
311-316: Ensure UBSan workaround even when sanitizer flag is off
Theelseif(is_clang11)branch ensures DeepState-enabled UBSan still gets the workaround. Please verify theif/elseif/endifchain nests correctly and doesn’t inadvertently skip other branches..github/workflows/old-compilers.yml (1)
653-658: Verify the comprehensive LLVM setup step
The "Setup dependencies for LLVM" step installsclang-${VERSION}andclang-tidy-${VERSION}for all versions. Confirm that these packages exist and match the expected toolchain versions on Ubuntu 22.04.
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
from
88e2b19 to
4ef12fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/build.yml (1)
406-427: 🛠️ Refactor suggestionMissing
set -eto restore fail‑fast after CMake configuration
You disable exit-on-error (set +e) to capture the CMake exit code, but you never re-enable it. This means subsequent shell failures won’t abort the workflow as intended. Addset -eright after you capture the exit code to restore fail-fast semantics.Proposed diff:
--- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -414,6 +414,7 @@ CMAKE_EXIT_CODE=$? + set -e if [ $CMAKE_EXIT_CODE -ne 0 ]; then if [ -f build/CMakeFiles/CMakeError.log ]; then echo "::group::CMakeError.log".github/workflows/old-compilers.yml (2)
659-665:⚠️ Potential issueInstall both GCC and G++ for versioned packages
You’re only installinggcc-${VERSION}for GCC 10 & 12, but CMake setsCXX="g++-${VERSION}". That will break the build.Proposed fix:
--- a/.github/workflows/old-compilers.yml +++ b/.github/workflows/old-compilers.yml @@ -659,3 +659,4 @@ - name: Setup dependencies for GCC 10 & 12 (versioned packages) run: | - sudo apt-get install -y "gcc-${VERSION}" + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}" if: > env.COMPILER == 'gcc' && (env.VERSION == '10' || env.VERSION == '12')
644-652: 🧹 Nitpick (assertive)Deprecation: avoid
apt-key addfor LLVM GPG key
Ubuntu 22.04 deprecatesapt-key add. Switch to importing the key into/etc/apt/trusted.gpg.d/and addsigned-byin your source entry for better future compatibility.Example diff:
- run: | - curl 'https://apt.llvm.org/llvm-snapshot.gpg.key' \ - | sudo apt-key add - - sudo add-apt-repository -y \ - "deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${VERSION} main" + run: | + curl 'https://apt.llvm.org/llvm-snapshot.gpg.key' \ + | gpg --dearmor \ + | sudo tee /etc/apt/trusted.gpg.d/llvm-snapshot.gpg >/dev/null + sudo add-apt-repository -y \ + "deb [signed-by=/etc/apt/trusted.gpg.d/llvm-snapshot.gpg] http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${VERSION} main"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(5 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)CMakeLists.txt(3 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
from
4ef12fc to
a6d59d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
.github/workflows/build.yml (2)
406-427: Restore fail‑fast behavior after capturing CMake exit code
You disableset -eto capture the CMake exit code but never re‑enable it. Even if no further commands follow in this block, it’s safer to restoreset -eimmediately after handling errors to prevent silent failures if someone later adds commands here.
416-425: 🧹 Nitpick (assertive)Consider adding directory listing for richer diagnostics
Before dumping the CMake logs, printing the build directory contents (ls -alR build/) helps surface missing files or odd directory structures—just like inold-compilers.yml.- if [ -f build/CMakeFiles/CMakeError.log ]; then + ls -alR build/ + if [ -f build/CMakeFiles/CMakeError.log ]; thenCMakeLists.txt (1)
818-824:⚠️ Potential issueFix undefined variable in sanitizer test condition
Theadd_sanitized_testfunction checks(TARGET deepstate AND clang11), butclang11is undefined. It should referenceis_clang11.- if(SANITIZE_ADDRESS OR SANITIZE_THREAD OR SANITIZE_UB - OR (TARGET deepstate AND clang11)) + if(SANITIZE_ADDRESS OR SANITIZE_THREAD OR SANITIZE_UB + OR (TARGET deepstate AND is_clang11)).github/workflows/old-compilers.yml (2)
659-665:⚠️ Potential issueInstall both GCC and G++ for versioned packages
The step for GCC 10 & 12 only installsgcc-${VERSION}, but CMake will invokeg++-${VERSION}. Without the C++ compiler, builds on these versions will fail.- sudo apt-get install -y "gcc-${VERSION}" + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}"
644-652: 🧹 Nitpick (assertive)Replace deprecated
apt-key addusage
Ubuntu 22.04 deprecatesapt-key add. Switch to importing viagpg --dearmorinto/etc/apt/trusted.gpg.d/and usesigned-byin thedebentry for future compatibility.- curl 'https://apt.llvm.org/llvm-snapshot.gpg.key' \ - | sudo apt-key add - + curl 'https://apt.llvm.org/llvm-snapshot.gpg.key' \ + | gpg --dearmor \ + | sudo tee /etc/apt/trusted.gpg.d/llvm-apt-keyring.gpg > /dev/null sudo add-apt-repository -y \ "deb [signed-by=/etc/apt/trusted.gpg.d/llvm-apt-keyring.gpg] http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${VERSION} main"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(5 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)CMakeLists.txt(3 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
🔇 Additional comments (5)
CMakeLists.txt (3)
31-33: Newis_clang11helper for Clang 11 detection is clear and precise.
This boolean simplifies targeting Clang 11‑specific logic.
304-308: Appropriate UBSan workaround for Clang 11.
Appending:use_sigaltstack=falseaddresses LLVM #55785 when UBSan is enabled.
312-317: Apply workaround even when UBSan is disabled.
Ensuring DeepState’s self‑enabled UBSan run gets the same fix is essential..github/workflows/old-compilers.yml (2)
676-683: Enable verbose output for diagnostics
Addingset -xbefore the CMake invocation will echo every command, making CI failures easier to trace.
701-717: Improve CMake failure diagnostics end‑to‑end
Capturing the exit code, re‑enablingset -e, listingbuild/, and dumpingCMakeConfigureLog.yamlcreates a robust debug trail for configuration failures.
| - name: clang 11 Release | ||
| BUILD_TYPE: Release | ||
| COMPILER: clang | ||
| VERSION: 11 | ||
|
|
||
| - name: clang 11 Release with ASan | ||
| BUILD_TYPE: Release | ||
| SANITIZE_ADDRESS: ON | ||
| COMPILER: clang | ||
| VERSION: 11 | ||
|
|
||
| - name: clang 11 Release with TSan | ||
| BUILD_TYPE: Release | ||
| SANITIZE_THREAD: ON | ||
| COMPILER: clang | ||
| VERSION: 11 | ||
|
|
||
| - name: clang 11 Release with UBSan | ||
| BUILD_TYPE: Release | ||
| SANITIZE_UB: ON | ||
| COMPILER: clang | ||
| VERSION: 11 | ||
|
|
||
| - name: clang 11 Debug | ||
| BUILD_TYPE: Debug | ||
| COMPILER: clang | ||
| VERSION: 11 | ||
|
|
||
| - name: clang 11 Debug with ASan | ||
| BUILD_TYPE: Debug | ||
| SANITIZE_ADDRESS: ON | ||
| COMPILER: clang | ||
| VERSION: 11 | ||
|
|
||
| - name: clang 11 Debug with TSan | ||
| BUILD_TYPE: Debug | ||
| SANITIZE_THREAD: ON | ||
| COMPILER: clang | ||
| VERSION: 11 | ||
|
|
||
| - name: clang 11 Debug with UBSan | ||
| BUILD_TYPE: Debug | ||
| SANITIZE_UB: ON | ||
| COMPILER: clang | ||
| VERSION: 11 | ||
|
|
||
| - name: clang 12 Release | ||
| BUILD_TYPE: Release | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Release with ASan | ||
| BUILD_TYPE: Release | ||
| SANITIZE_ADDRESS: ON | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Release with TSan | ||
| BUILD_TYPE: Release | ||
| SANITIZE_THREAD: ON | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Release with UBSan | ||
| BUILD_TYPE: Release | ||
| SANITIZE_UB: ON | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Debug | ||
| BUILD_TYPE: Debug | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Debug with ASan | ||
| BUILD_TYPE: Debug | ||
| SANITIZE_ADDRESS: ON | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Debug with TSan | ||
| BUILD_TYPE: Debug | ||
| SANITIZE_THREAD: ON | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Debug with UBSan | ||
| BUILD_TYPE: Debug | ||
| SANITIZE_UB: ON | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Matrix duplication for Clang 11/12 variants is error‑prone
Listing all Release/Debug × ASan/TSan/UBSan combinations for Clang 11 and 12 manually makes the file huge and risks drift. Use YAML anchors or a generated matrix to DRY this up and improve maintainability.
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
3 times, most recently
from
e9f617a to
2874c42
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
CMakeLists.txt (1)
818-833:⚠️ Potential issueFix variable name in
add_sanitized_testconditionYou reference
${clang11}but the variable is namedis_clang11. This typo means the workaround won’t trigger. Update all occurrences ofclang11tois_clang11.Suggested diff:
- message(STATUS "clang11: ${clang11}") - if(SANITIZE_ADDRESS OR SANITIZE_THREAD OR SANITIZE_UB - OR (TARGET deepstate AND clang11)) + message(STATUS "is_clang11: ${is_clang11}") + if(SANITIZE_ADDRESS OR SANITIZE_THREAD OR SANITIZE_UB + OR (TARGET deepstate AND is_clang11))
♻️ Duplicate comments (4)
.github/workflows/build.yml (2)
415-427: 🧹 Nitpick (assertive)Consider adding build directory listing for better diagnostics
Before printing the error logs, a recursive
ls -alR build/helps identify missing or mislocated log files. This can be wrapped in a group for clarity.Suggested diff:
if [ $CMAKE_EXIT_CODE -ne 0 ]; then + echo "::group::Build directory contents" + ls -alR build/ + echo "::endgroup::" if [ -f build/CMakeFiles/CMakeError.log ]; then echo "::group::CMakeError.log" cat build/CMakeFiles/CMakeError.log echo "::endgroup::" fi ...
406-414:⚠️ Potential issueRestore fail‑fast behavior after CMake invocation
After disabling
set -ebefore runningcmaketo capture its exit code, you never re‑enable it. Subsequent steps won’t abort on failures as intended. Re-addset -eimmediately after capturing the exit code to restore fail‑fast behavior.Suggested diff:
CMAKE_EXIT_CODE=$? + set -e if [ $CMAKE_EXIT_CODE -ne 0 ]; then.github/workflows/old-compilers.yml (2)
644-652: Replace deprecatedapt-key addusage
Ubuntu 22.04 deprecatesapt-key. Import the GPG key viagpg --dearmorinto/etc/apt/trusted.gpg.d/and addsigned-byto the source entry.
659-665:⚠️ Potential issueInstall missing
g++-${VERSION}packageOnly
gcc-${VERSION}is installed for GCC 10/12. CMake expectsg++-${VERSION}, so add it to prevent linker failures.Suggested diff:
- sudo apt-get install -y "gcc-${VERSION}" + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(5 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)CMakeLists.txt(3 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
CMakeLists.txt (4)
31-33: Definition ofis_clang11looks correct
The boolean helper macro correctly detects Clang versions ≥11 and <12. This variable supports the UBSan workaround.
294-296: Enable-fsanitize=implicit-bitfield-conversionfor newer Clangs
The conditional for Clang ≥19 is properly scoped. No issues found.
304-309: Workaround for UBSan on Clang 11 packaged in Ubuntu 22.04
Appending:use_sigaltstack=falsetoUBSAN_OPTIONSunderis_clang11is correctly placed within the UBSan-enabled block.
312-317: DeepState UBSan workaround in non-sanitizer build
Theelseif(is_clang11)block correctly adds theUBSAN_OPTIONSworkaround for DeepState.
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
from
2874c42 to
5af62d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
.github/workflows/old-compilers.yml (5)
38-83: Matrix duplication for Clang 11 variants
Listing eight nearly identical Clang 11 configurations inflates the workflow and increases maintenance risk. Please DRY this up using YAML anchors/aliases or a generated matrix.
84-129: Matrix duplication for Clang 12 variants
Similarly, the Clang 12 entries repeat the same pattern. Consolidate these with anchors or a dynamic strategy matrix to reduce repetition.
472-517: Matrix duplication for GCC 10 variants
The GCC 10 section also lists Release/Debug × ASan/TSan/UBSan verbatim. Consider templating or anchors to keep the matrix DRY.
644-652: Deprecatedapt-key addusage
Ubuntu 22.04 deprecatesapt-key add. Usegpg --dearmorinto/etc/apt/trusted.gpg.d/and addsigned-byin the repo entry to future‑proof this step.
659-665: Install missing C++ compiler package for GCC 10 & 12
CMake setsCXX="g++-${VERSION}"later, but onlygcc-${VERSION}is installed here. Update the install line to include"g++-${VERSION}"as well.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(5 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)CMakeLists.txt(3 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
.github/workflows/build.yml (1)
406-427: Great enhancement to CMake diagnostics
Capturing the exit code and grouping bothbuild/CMakeFiles/CMakeError.logandCMakeOutput.logmakes debugging CI failures much easier..github/workflows/old-compilers.yml (2)
653-658: Consolidated LLVM dependency setup
Good job unifying the LLVM setup for all Clang versions. Installingllvm-${VERSION}-linker-tools,clang-${VERSION}, andclang-tidy-${VERSION}under one step simplifies the workflow.
676-720: Verbose CMake configuration logging
Enablingset -x, capturing the exit code, listingbuild/contents, and printingCMakeConfigureLog.yamlon failure provides excellent diagnostics for old compiler builds.CMakeLists.txt (4)
31-33: Defineis_clang11for targeted workarounds
The booleanis_clang11correctly captures Clang ≥11.0 and <12.0. This cleanly scopes the Ubuntu 22.04 workaround.
294-297: Add implicit-bitfield sanitizer for Clang ≥19
Checking forCMAKE_CXX_COMPILER_VERSION ≥ 19.0before appending-fsanitize=implicit-bitfield-conversionensures compatibility. Nice forward‑looking addition.
304-309: UBSan workaround for Clang 11 in UBSan block
Appending:use_sigaltstack=falsetoUBSAN_ENVwhenis_clang11fixes the known LLVM #55785 issue. Correctly placed within the UBSan-enabled branch.
312-318: UBSan workaround for Clang 11 when sanitizer disabled
Including the sameuse_sigaltstack=falsesetting in the non-UBSan branch covers DeepState’s implicit sanitizer enabling. Well handled.
| # Set Sanitizer environment on a sanitizer-less build too, if using clang | ||
| # 11 and DeepState is built, because it enables sanitizers itself. | ||
| message(STATUS "add_sanitizer_test for ${AST_NAME}") | ||
| if(TARGET deepstate) | ||
| message(STATUS "TARGET deepstate is TRUE") | ||
| else() | ||
| message(STATUS "TARGET deepstate is FALSE") | ||
| endif() | ||
| message(STATUS "is_clang11: ${is_clang11}") | ||
| if(SANITIZE_ADDRESS OR SANITIZE_THREAD OR SANITIZE_UB | ||
| OR (TARGET deepstate AND is_clang11)) | ||
| message(STATUS "Setting sanitizer environment for ${AST_NAME}") | ||
| set_property(TEST "${AST_NAME}" APPEND PROPERTY ENVIRONMENT | ||
| "${SANITIZER_ENV}") | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhanced add_sanitized_test for Clang 11 and DeepState
Great to extend sanitizer environment to DeepState tests under Clang 11. For less verbosity in normal runs, consider switching message(STATUS ...) calls to message(VERBOSE ...).
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
2 times, most recently
from
ec0c5a6 to
9bb3672
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
.github/workflows/old-compilers.yml (2)
644-652: 🧹 Nitpick (assertive)Update deprecated apt-key usage for LLVM repository.
Ubuntu 22.04+ deprecatesapt-key add. Import the GPG key viagpg --dearmorinto/etc/apt/trusted.gpg.d/and reference it withsigned-byin your source entry to future‑proof the workflow.
Proposed diff:- curl 'https://apt.llvm.org/llvm-snapshot.gpg.key' | sudo apt-key add - - sudo add-apt-repository -y "deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${VERSION} main" + curl -s https://apt.llvm.org/llvm-snapshot.gpg.key \ + | sudo gpg --dearmor -o /etc/apt/trusted.gpg.d/llvm-snapshot.gpg + echo "deb [signed-by=/etc/apt/trusted.gpg.d/llvm-snapshot.gpg] http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${VERSION} main" \ + | sudo tee /etc/apt/sources.list.d/llvm.list
659-665:⚠️ Potential issueInstall missing C++ compiler package for GCC 10 & 12.
Onlygcc-${VERSION}is installed, but CMake setsCXX=g++-${VERSION}, which will fail ifg++-${VERSION}is absent. Please install both packages.- sudo apt-get install -y "gcc-${VERSION}" + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(5 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)CMakeLists.txt(4 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 11 Debug with UBSan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 11 Debug with UBSan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 11 Debug with UBSan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 11 Debug with UBSan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 11 Debug with UBSan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 11 Debug with UBSan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 11 Debug with UBSan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (8)
.github/workflows/old-compilers.yml (2)
676-683: Enable shell tracing for CMake configuration.
Addingset -xbefore the configuration step logs each command, improving visibility when diagnosing failures in the old‑compilers workflow.
701-720: Enhanced diagnostics for CMake failures in old‑compilers workflow.
Disabling fail-fast (set +e), capturingCMAKE_EXIT_CODE, re-enabling fail-fast, listing the build directory, and printingCMakeConfigureLog.yamlwithin grouped annotations will greatly aid troubleshooting for older compiler builds.CMakeLists.txt (6)
31-33: Introduceis_clang11helper for Clang 11 detection.
The new boolean accurately captures when the compiler is Clang ≥ 11 and < 12. This simplifies conditional logic elsewhere.
294-296: Add-fsanitize=implicit-bitfield-conversionfor Clang 19+.
Gating this sanitizer flag on compiler version ≥ 19.0 leverages newer Clang features while avoiding errors on older toolchains.
306-308: Workaround for UBSan on Clang 11.
Appending:use_sigaltstack=falsetoUBSAN_OPTIONSwhenis_clang11addresses LLVM issue #55785 on Ubuntu 22.04. Well scoped.
312-317: Apply UBSan workaround for DeepState on Clang 11.
Theelseif(is_clang11)branch ensures DeepState’s internal sanitizer usage also gets the same workaround. This maintains consistency across test types.
574-578: Enable UBSan workaround in benchmarking code.
SettingENV{SANITIZER_ENV}for Clang 11 benchmarks matches the workaround used elsewhere, preventing spurious failures during performance testing.
832-837: Extend sanitizer environment for DeepState on Clang 11.
The updated condition (OR (TARGET deepstate AND is_clang11)) correctly applies the workaround when DeepState enables sanitizers internally. This resolves known Clang 11 issues in test runs.
| set +e | ||
| cmake -B build "$GITHUB_WORKSPACE" "$CBT" -DSTANDALONE=ON \ | ||
| -DMAINTAINER_MODE=ON -DIWYU=ON \ | ||
| "-DSANITIZE_ADDRESS=${SANITIZE_ADDRESS}" \ | ||
| "-DSANITIZE_THREAD=${SANITIZE_THREAD}" \ | ||
| "-DSANITIZE_UB=${SANITIZE_UB}" \ | ||
| "-DSTATIC_ANALYSIS=${STATIC_ANALYSIS}" "-DCOVERAGE=${COVERAGE}" \ | ||
| "-DAVX2=${AVX2}" "${EXTRA_CMAKE_ARGS[@]}" | ||
| CMAKE_EXIT_CODE=$? | ||
| if [ $CMAKE_EXIT_CODE -ne 0 ]; then | ||
| if [ -f build/CMakeFiles/CMakeError.log ]; then | ||
| echo "::group::CMakeError.log" | ||
| cat build/CMakeFiles/CMakeError.log | ||
| echo "::endgroup::" | ||
| fi | ||
| if [ -f build/CMakeFiles/CMakeOutput.log ]; then | ||
| echo "::group::CMakeOutput.log" | ||
| cat build/CMakeFiles/CMakeOutput.log | ||
| echo "::endgroup::" | ||
| fi | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve CMake configuration diagnostics further.
You've correctly disabled immediate exit (set +e), captured CMAKE_EXIT_CODE, and printed logs under grouped annotations.
It might also be beneficial to include a ls -alR build/ before the log checks to surface unexpected directory structures or missing log files when troubleshooting CI failures.
| - name: clang 12 Release | ||
| BUILD_TYPE: Release | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Release with ASan | ||
| BUILD_TYPE: Release | ||
| SANITIZE_ADDRESS: ON | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Release with TSan | ||
| BUILD_TYPE: Release | ||
| SANITIZE_THREAD: ON | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Release with UBSan | ||
| BUILD_TYPE: Release | ||
| SANITIZE_UB: ON | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Debug | ||
| BUILD_TYPE: Debug | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Debug with ASan | ||
| BUILD_TYPE: Debug | ||
| SANITIZE_ADDRESS: ON | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Debug with TSan | ||
| BUILD_TYPE: Debug | ||
| SANITIZE_THREAD: ON | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
||
| - name: clang 12 Debug with UBSan | ||
| BUILD_TYPE: Debug | ||
| SANITIZE_UB: ON | ||
| COMPILER: clang | ||
| VERSION: 12 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
DRY up Clang 12 matrix entries.
Similarly, the Clang 12 matrix repeats eight manual entries. Using anchors or a templated matrix will reduce duplication and minimize drift.
| # Set Sanitizer environment on a sanitizer-less build too, if using clang | ||
| # 11 and DeepState is built, because it enables sanitizers itself. | ||
| message(STATUS "add_sanitizer_test for ${AST_NAME}") | ||
| if(TARGET deepstate) | ||
| message(STATUS "TARGET deepstate is TRUE") | ||
| else() | ||
| message(STATUS "TARGET deepstate is FALSE") | ||
| endif() | ||
| message(STATUS "is_clang11: ${is_clang11}") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Refine debug logging for add_sanitized_test.
The added message(STATUS) calls improve visibility of why the sanitizer environment is applied, but may clutter normal output. Consider using message(VERBOSE) for these lines to reduce noise unless verbose mode is enabled.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/build.yml (1)
414-415: Re-enable fail‑fast after capturing exit code.
You captureCMAKE_EXIT_CODEbut never restoreset -e, so if any subsequent commands in this step fail, they won’t abort. Restoreset -eimmediately after capturing the exit code.CMakeLists.txt (1)
823-831: 🧹 Nitpick (assertive)Switch sanitizer‑test debug messages to verbose mode.
The addedmessage(STATUS)calls help diagnoseadd_sanitized_test, but they will clutter normal CI logs. Consider usingmessage(VERBOSE)for these to limit noise unless verbose mode is explicitly enabled..github/workflows/old-compilers.yml (1)
659-665:⚠️ Potential issueInstall g++-${VERSION} alongside gcc.
Onlygcc-${VERSION}is being installed, but CMake setsCXX="g++-${VERSION}", which will cause failures ifg++-${VERSION}is missing. Add"g++-${VERSION}"to the install command.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(5 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)CMakeLists.txt(4 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
.github/workflows/build.yml (2)
406-407: Allow capturing CMake exit code for diagnostics.
Disablingset -ebefore invoking CMake is correct to let the script proceed and capture a non‑zero exit code for later inspection.
416-427: Print CMake error and output logs on failure.
Grouping and printingbuild/CMakeFiles/CMakeError.logandCMakeOutput.logunder log groups is an effective enhancement for debugging configuration failures. Paths are correct.CMakeLists.txt (4)
31-33: Detect Clang 11 specifically.
Introducingis_clang11viaset_boolaccurately captures Clang ID with version ≥ 11.0 and < 12.0 for targeted workarounds.
304-309: Apply UBSan workaround for Clang 11.
Appending:use_sigaltstack=falsetoUBSAN_OPTIONSunderis_clang11addresses LLVM #55785 on Ubuntu 22.04. This is correctly scoped and ensures UBSan runs smoothly on Clang 11.
312-317: Enable DeepState workaround when UBSan isn’t enabled.
Appending"UBSAN_OPTIONS=use_sigaltstack=false"toSANITIZER_ENVin theelseif(is_clang11)branch covers DeepState’s self‑enabled sanitizers. Well handled.
574-578: Workaround Clang 11 in benchmark setup.
In the benchmarks block, you correctly setENV{SANITIZER_ENV}when any sanitizer or DeepState on Clang 11 is used. This mirrors the general UBSan fix and is spot‑on..github/workflows/old-compilers.yml (3)
676-676: Enable command echo for debugging.
Addingset -xwill trace shell commands in the CI logs, which is helpful when diagnosing failures.
701-710: Capture and restore shell error behavior around CMake.
Disabling exit‑on‑error withset +e, running CMake, capturingCMAKE_EXIT_CODE, and then restoring withset -eis the correct pattern to both gather diagnostics and maintain fail‑fast semantics for the remainder of the step.
711-719: Surface CMake configuration failures with directory listing and logs.
Recursively listingbuild/and printingbuild/CMakeFiles/CMakeConfigureLog.yamlunder a group block greatly improves CI troubleshooting.
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
from
9bb3672 to
727e4c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
.github/workflows/old-compilers.yml (3)
38-83: 🧹 Nitpick (assertive)DRY up Clang 11 matrix entries
The eight nearly identical entries for Clang 11 (Release/Debug × ASan/TSan/UBSan/OFF) bloat the workflow and risk drift. Using YAML anchors or matrix generation would simplify maintenance.
84-129: 🧹 Nitpick (assertive)DRY up Clang 12 matrix entries
Similarly, Clang 12 entries repeat the same pattern. Consolidating with anchors or aliases will reduce duplication and improve readability.
472-517: 🧹 Nitpick (assertive)DRY up GCC 10 matrix entries
Listing all eight GCC 10 combinations manually inflates the file. Consider templating this pattern or using anchors to collapse these entries into a concise matrix.CMakeLists.txt (1)
823-832: 🧹 Nitpick (assertive)Use
message(VERBOSE)for sanitizer logs
Switching the diagnosticmessagetoVERBOSEwill reduce noise in normal CI runs, showing these logs only when--verboseis enabled.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(4 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)CMakeLists.txt(4 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
⏰ Context from checks skipped due to timeout of 90000ms (26)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 11 Debug with ASan, Debug, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
🔇 Additional comments (4)
.github/workflows/old-compilers.yml (1)
700-718: CMake configuration error handling looks solid
CapturingCMAKE_EXIT_CODE, grouping and printingCMakeConfigureLog.yaml, then exiting with that code improves diagnosability.CMakeLists.txt (3)
31-33: Clang 11 detection logic looks correct
Theis_clang11variable accurately identifies Clang 11.x by checking>=11.0and<12.0. Ensure CI compilers align with this version range.
304-309: Validate UBSan workaround application
The workaround foruse_sigaltstack=falseis applied both when UBSan is enabled and when DeepState auto-enables it. Please confirm this covers all relevant build and test scenarios.
312-317: Workaround on non-sanitized builds also seems correct
AppendingUBSAN_OPTIONS=use_sigaltstack=falsefor DeepState on Clang 11 without explicit UBSan ensures the same workaround. Good.
| if [ $CMAKE_EXIT_CODE -ne 0 ]; then | ||
| if [ -f build/CMakeFiles/CMakeConfigureLog.yaml ]; then | ||
| echo "::group::CMakeConfigureLog.yaml" | ||
| cat build/CMakeFiles/CMakeConfigureLog.yaml | ||
| echo "::endgroup::" | ||
| fi | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: List build directory on failure
Before printing logs, adding ls -alR build/ can help uncover unexpected directory structures or misplaced log files, improving diagnostics.
| - name: Setup LLVM 16+ repo | ||
| run: | | ||
| curl 'https://apt.llvm.org/llvm-snapshot.gpg.key' \ | ||
| | sudo apt-key add - | ||
| sudo add-apt-repository -y \ | ||
| "deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${VERSION} main" | ||
| sudo apt-get update | ||
| if: env.COMPILER == 'clang' && env.VERSION >= 16 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace deprecated apt-key add usage
Ubuntu 22.04 deprecates apt-key add. To future‑proof package installation, import the GPG key with gpg --dearmor into /etc/apt/trusted.gpg.d/ and reference it with a signed-by option in the source entry.
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
from
727e4c3 to
77fd09b
Compare
Move the tested compiler versions there to old-compilers.yml. Add a sanitizer workaround for Ubuntu 22.04-packaged clang 11. Fix a typo that prevented enabling -fsanitize=implicit-bitfield-conversion for clang 19 or later. Add extra CMake diagnostics in the case of its failure to build.yml and old-compilers.yml scripts.
laurynas-biveinis
force-pushed
the
rm-ubuntu-20.04
branch
from
77fd09b to
c2d98d9
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
.github/workflows/build.yml (1)
406-423: 🧹 Nitpick (assertive)Enhance diagnostics by listing the build directory
Before dumpingCMakeConfigureLog.yaml, include a recursive directory listing ofbuild/to expose misplaced or missing files during CI failures.Apply this diff:
@@ 406,423 - if [ $CMAKE_EXIT_CODE -ne 0 ]; then + echo "::group::Build directory contents" + ls -alR build/ + echo "::endgroup::" + if [ $CMAKE_EXIT_CODE -ne 0 ]; then.github/workflows/old-compilers.yml (5)
38-83: DRY up Clang 11 matrix entries
The eight nearly identical Clang 11 configurations bloat the matrix and risk drift. Please reuse YAML anchors/aliases or a generated strategy matrix here.
84-129: DRY up Clang 12 matrix entries
Similarly, Clang 12 repeats eight manual entries. Use anchors or templating to collapse these into a concise definition.
472-517: DRY up GCC 10 matrix entries
The GCC 10 Release/Debug × ASan/TSan/UBSan entries repeat eight times. Anchors or a parameterized matrix would greatly improve maintainability.
659-665:⚠️ Potential issueInstall matching C++ compiler package
Onlygcc-${VERSION}is installed for versions 10/12, butg++-${VERSION}is required by CMake’sCXXsetting.Apply this diff:
@@ 659,665 - - name: Setup dependencies for GCC 10 & 12 (versioned packages) - run: | - sudo apt-get install -y "gcc-${VERSION}" + - name: Setup dependencies for GCC 10 & 12 (versioned packages) + run: | + sudo apt-get install -y "gcc-${VERSION}" "g++-${VERSION}" if: > env.COMPILER == 'gcc' && (env.VERSION == '10' || env.VERSION == '12')
644-652: 🧹 Nitpick (assertive)Replace deprecated
apt-key addusage
apt-key addis deprecated on modern Ubuntu. Switch to importing the key viagpg --dearmorinto/usr/share/keyringsand reference it withsigned-byin the APT source.Example diff:
@@ 646,649 - curl 'https://apt.llvm.org/llvm-snapshot.gpg.key' \ - | sudo apt-key add - + curl 'https://apt.llvm.org/llvm-snapshot.gpg.key' \ + | gpg --dearmor \ + | sudo tee /usr/share/keyrings/llvm-archive-keyring.gpg >/dev/null - sudo add-apt-repository -y \ - "deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${VERSION} main" + echo "deb [signed-by=/usr/share/keyrings/llvm-archive-keyring.gpg] \ + http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${VERSION} main" \ + | sudo tee /etc/apt/sources.list.d/llvm-toolchain.list
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(1 hunks).github/workflows/old-compilers.yml(4 hunks).github/workflows/ubuntu-20.04.yml(0 hunks)CMakeLists.txt(4 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ubuntu-20.04.yml
⏰ Context from checks skipped due to timeout of 90000ms (60)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release, Release, clang, 12)
- GitHub Check: build (clang 11 Debug with TSan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: build (clang 11 Release with ASan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release, Release, clang, 12)
- GitHub Check: build (clang 11 Debug with TSan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: build (clang 11 Release with ASan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release, Release, clang, 12)
- GitHub Check: build (clang 11 Debug with TSan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: build (clang 11 Release with ASan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release, Release, clang, 12)
- GitHub Check: build (clang 11 Debug with TSan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: build (clang 11 Release with ASan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release, Release, clang, 12)
- GitHub Check: build (clang 11 Debug with TSan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: build (clang 11 Release with ASan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release, Release, clang, 12)
- GitHub Check: build (clang 11 Debug with TSan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: build (clang 11 Release with ASan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release, Release, clang, 12)
- GitHub Check: build (clang 11 Debug with TSan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: build (clang 11 Release with ASan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release, Release, clang, 12)
- GitHub Check: build (clang 11 Debug with TSan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: build (clang 11 Release with ASan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release, Release, clang, 12)
- GitHub Check: build (clang 11 Debug with TSan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: build (clang 11 Release with ASan, Release, ON, clang, 11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release, Release, clang, 12)
- GitHub Check: build (clang 11 Debug with TSan, Debug, ON, clang, 11)
- GitHub Check: build (clang 11 Release with UBSan, Release, ON, clang, 11)
- GitHub Check: build (clang 11 Release with ASan, Release, ON, clang, 11)
🔇 Additional comments (4)
CMakeLists.txt (4)
31-33: Approve newis_clang11helper
The boolean helper correctly detects Clang 11.x. Givencmake_minimum_required(VERSION 3.16), multi-argumentif()expressions work as expected.
304-309: Approve UBSan workaround in main sanitizer block
Appending:use_sigaltstack=falseunderis_clang11correctly addresses the LLVM #55785 issue.
312-317: Approve DeepState UBSan workaround
EnsuresUBSAN_OPTIONSis set even when DeepState self‑enables Sanitizer on Clang 11. Good catch.
833-840: Approve sanitized test environment injection
The condition correctly appendsSANITIZER_ENVwhen any sanitizer is on or when DeepState under Clang 11 needs the workaround.
1 participant
Move the tested compiler versions there to old-compilers.yml.
Summary by CodeRabbit