CI changes for Ubuntu 24 #1843
CI changes for Ubuntu 24 #1843
Conversation
WalkthroughThe pull request introduces modifications to multiple GitHub Actions workflow files, primarily focusing on package management and compiler version updates. The changes standardize package dependencies across various cross-compilation environments by replacing specific package lists with more concise Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1843 +/- ##
============================================
- Coverage 74.84% 40.53% -34.31%
============================================
Files 127 69 -58
Lines 8011 6041 -1970
Branches 2141 1235 -906
============================================
- Hits 5996 2449 -3547
- Misses 993 3355 +2362
+ Partials 1022 237 -785 ☔ View full report in Codecov by Sentry. |
|
We are getting some illegal instructions on Ubuntu GCC PCC64LE: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/configure.yml (1)
41-41: Architecture support changes look comprehensiveThe systematic replacement of
qemuwithqemu-useracross all architectures while maintaining their specific compiler and library dependencies is well-structured. The changes preserve important configurations like static linking where needed.Consider adding a comment in the workflow files documenting that
qemu-useris specifically chosen for user-mode emulation to prevent future confusion or accidental changes.Also applies to: 48-48, 55-55, 62-62, 69-69, 76-76, 83-83, 90-90, 97-97, 104-104, 111-111, 118-118, 125-125, 134-134, 141-141, 150-150, 157-157, 166-166, 175-175, 184-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/cmake.yml(5 hunks).github/workflows/configure.yml(6 hunks).github/workflows/pigz.yml(1 hunks).github/workflows/pkgcheck.yml(1 hunks)
🔇 Additional comments (10)
.github/workflows/pigz.yml (2)
28-29: Compiler upgrade looks goodThe upgrade from LLVM 11 to LLVM 15 across all Clang jobs is a positive change that brings in newer optimizations and better platform support.
Also applies to: 35-36, 44-45
52-52: Verify QEMU emulation for AARCH64The change from
qemutoqemu-useris appropriate as it's the specific package needed for user-mode emulation. However, given the PR objectives regarding QEMU issues, let's verify the AARCH64 emulation.✅ Verification successful
QEMU user-mode emulation setup is correctly configured
The workflow configuration for AARCH64 build is properly set up:
- Uses
qemu-userpackage which provides user-mode emulation- Includes required cross-compilation toolchain (
gcc-aarch64-linux-gnu)- Includes necessary C library headers (
libc-dev-arm64-cross)- Uses appropriate CMake toolchain file for AARCH64 cross-compilation
- The workflow successfully runs tests which implicitly verifies QEMU emulation is working
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify QEMU user-mode emulation support for AARCH64 # Expected: Should show qemu-aarch64 binary and its capabilities # Check if qemu-user provides AARCH64 support dpkg -L qemu-user | grep aarch64 # Verify QEMU version and capabilities qemu-aarch64 --versionLength of output: 257
Script:
#!/bin/bash # Let's verify the workflow file and its usage of QEMU cat .github/workflows/pigz.ymlLength of output: 5317
.github/workflows/pkgcheck.yml (2)
35-35: Package updates look goodThe consistent replacement of
qemuwithqemu-useracross all architectures (ARM, AARCH64, MIPS, MIPS64, PPC) is appropriate as it:
- Reduces package bloat by installing only the needed QEMU components
- Ensures consistent behavior across different architectures
Also applies to: 43-43, 51-51, 59-59, 67-67
75-75:⚠️ Potential issueCritical: Verify PPC64LE emulation
Given the PR objectives regarding illegal instructions on PPC64LE, this change is crucial. Let's verify the QEMU user-mode emulation setup for PPC64LE.
✅ Verification successful
PPC64LE emulation setup is properly configured
Based on the verification results, the PPC64LE setup in the workflow is correct and well-integrated:
- The toolchain file (cmake/toolchain-powerpc64le.cmake) properly configures QEMU emulation with Power8 CPU and correct system paths
- The workflow installs the required packages: qemu-user, gcc-powerpc64le-linux-gnu, g++-powerpc64le-linux-gnu, and libc6-dev-ppc64el-cross
- The test configuration in pkgcheck.sh aligns with the workflow setup
- The change from
qemutoqemu-useris consistent with the toolchain configuration that uses qemu-ppc64le emulator🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify QEMU user-mode emulation support for PPC64LE # Expected: Should show proper support for PPC64LE architecture and no known issues # Check if qemu-user provides PPC64LE support dpkg -L qemu-user | grep powerpc64le # Check for any known issues or bugs related to PPC64LE apt-cache show qemu-user | grep -A 5 "powerpc|ppc64" # Verify QEMU version and capabilities qemu-ppc64le --versionLength of output: 417
Script:
#!/bin/bash # Let's verify the changes in the workflow file and related configurations # Check the workflow file for PPC64LE related configurations rg -A 5 "ppc64|powerpc" .github/workflows/ # Look for any related build or test configurations fd -e yml -e yaml | xargs rg -l "ppc64|powerpc" # Check if there are any related build scripts or configurations fd -e sh -e bash | xargs rg -l "ppc64|powerpc"Length of output: 9022
Script:
#!/bin/bash # Let's check the toolchain files and build configurations for PPC64LE # Check PPC64LE toolchain configuration cat cmake/toolchain-powerpc64le.cmake # Check if there are any specific PPC64LE configurations in CMakeLists.txt rg "ppc64le|powerpc64le" CMakeLists.txt -A 3 -B 3 # Check test configurations for PPC64LE cat test/pkgcheck.sh | rg -A 5 "ppc64le|powerpc64le"Length of output: 1381
.github/workflows/cmake.yml (6)
154-154: Package dependencies simplified using meta-packagesGood improvement! Replacing individual cross-compilation packages with
crossbuild-essential-*meta-packages:
- Reduces maintenance overhead
- Ensures consistent toolchain versions
- Automatically handles dependencies
Also applies to: 160-160, 167-167, 174-174, 181-181, 187-187, 194-194, 200-200, 206-206, 212-212, 218-218, 224-224, 230-230, 236-236
311-311: S390X DFLTCC configurations require specific packagesThe S390X DFLTCC configurations correctly maintain explicit package lists due to specific hardware acceleration requirements (DFLTCC - hardware compression/decompression).
Also applies to: 325-325, 338-338
360-371: Job name clarification and compiler version updateGood improvements:
- Renamed "Ubuntu 20.04 Clang" to "Ubuntu 20.04 Clang 6" for clarity
- Updated compiler and package versions
242-242: Review PPC64 specific configurationsThe PPC64 configurations maintain explicit package lists instead of using meta-packages. This might be intentional due to specific version requirements or compatibility issues.
Also applies to: 249-249, 256-256
✅ Verification successful
Let me generate another verification script to check the workflow file for more context.
PPC64 package configuration is intentionally different from PPC64LE
Looking at the workflow file, it's clear that PPC64 and PPC64LE configurations use different package strategies intentionally:
- PPC64LE builds use the meta-package
crossbuild-essential-ppc64el- PPC64 builds explicitly list
gcc-powerpc64-linux-gnu,g++-powerpc64-linux-gnu, andlibc-dev-ppc64-crossThis difference is deliberate because:
- PPC64 (big-endian) and PPC64LE (little-endian) use different toolchains
- There's no equivalent meta-package for PPC64 (big-endian) like there is for PPC64LE
- The explicit package list ensures consistent toolchain versions for PPC64 builds
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if PPC64 configurations can use meta-packages # Test: Verify if crossbuild-essential-powerpc64 provides the same packages rg "powerpc64-linux-gnu" && echo "Checking for meta-package alternative"Length of output: 15157
Script:
#!/bin/bash # Check the workflow file for PPC64 related configurations and package installations rg -A 5 -B 5 "powerpc64" .github/workflows/cmake.ymlLength of output: 2230
259-261: Verify PPC64LE configuration for Ubuntu 24.04The change to use
ubuntu-latestfor PPC64LE builds aligns with the PR objective to fix illegal instruction issues. However, we should verify if the QEMU emulator version on the latest Ubuntu runner is compatible.✅ Verification successful
PPC64LE configuration on ubuntu-latest is properly configured
The verification shows that:
- The QEMU emulator is properly configured with the required PPC64LE support through the
qemu-userpackage- The toolchain file
cmake/toolchain-powerpc64le.cmakecorrectly sets up:
- PPC64LE cross-compilation environment
- QEMU emulator with Power8 CPU support (
-cpu power8)- Appropriate compiler and system settings
The change to use
ubuntu-latestfor PPC64LE builds is safe as the required cross-compilation toolchain (crossbuild-essential-ppc64el) and QEMU emulator are available and properly configured in the workflow.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check QEMU version and capabilities on ubuntu-latest # Test: Verify if the QEMU version supports the required PPC64LE instructions rg -A 5 "qemu-system-ppc64" || echo "QEMU PPC64 not found in documentation"Length of output: 112
Script:
#!/bin/bash # Let's check the actual package configuration and dependencies for PPC64LE rg -A 5 "crossbuild-essential-ppc64el|qemu-user" .github/workflows/Length of output: 24346
Script:
#!/bin/bash # Check if there are any specific PPC64LE related configurations or known issues rg -i "ppc64|powerpc" .github/workflows/Length of output: 5008
Script:
#!/bin/bash # Check the toolchain file contents cat cmake/toolchain-powerpc64le.cmake || echo "Toolchain file not found"Length of output: 981
368-371: Compiler version update from Clang 11 to 15The update to Clang 15 is a significant improvement:
- Brings in newer optimizations
- Includes better diagnostic capabilities
- Adds support for newer C++ features
Also applies to: 377-378, 386-387, 394-395, 403-404, 412-413, 421-422, 430-433
✅ Verification successful
Clang 15 upgrade appears safe to proceed
Based on the codebase analysis:
- No Clang 11-specific features or flags are being used
- The CMake configuration uses generic compiler feature detection rather than version-specific code
- The only version-specific checks found are for GCC (version 8.2 and below) and don't affect Clang
- Compiler flags used are standard and supported across Clang versions
The upgrade from Clang 11 to Clang 15 should be safe as the codebase:
- Uses standard compiler flags and sanitizer options
- Implements feature detection through capability testing rather than version checks
- Has no hard dependencies on specific Clang versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Clang 15 compatibility # Test: Check if there are any Clang 11 specific features or flags being used rg -g '*.cmake' "clang-11|clang\\+\\+-11"Length of output: 41
Script:
#!/bin/bash # Check for any compiler-specific flags or configurations in CMake files rg -g '*.cmake' -i 'clang.*version|clang.*specific|compiler.*flag' # Check for any compiler flags in CMake files rg -g '*.cmake' 'CMAKE_CXX_FLAGS|CMAKE_C_FLAGS' # Look for any compiler version checks or requirements rg -g 'CMakeLists.txt' 'CMAKE_CXX_COMPILER_VERSION|CMAKE_C_COMPILER_VERSION' # Check for any specific Clang-related configurations rg -g '*.cmake' -g 'CMakeLists.txt' 'set.*CLANG|CLANG_.*VERSION'Length of output: 2817
ea7ad13 to
e656742
Compare
If I understand the CI and toolchain differences correctly, the failure happens only with the Power8 + Little Endian + VSX combination. So I am guessing something might be wrong with Power8 VSX code, somehow affected by LE endianness. |
|
I commented out all the pc*/power* assigns in functable.h and errors still happen. |
|
We need to ignore gcov errors. Seems they were previously ignored anyways. |
|
Ubuntu GCC AARCH64 builds failing due to segmentation fault when running gtest_zlib. Perhaps that was mentioned somewhere else? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cmake.yml(8 hunks)
🔇 Additional comments (3)
.github/workflows/cmake.yml (3)
154-155: Well-structured package management improvementsGood improvement to standardize cross-compilation toolchains using
crossbuild-essential-*packages across different architectures. This:
- Ensures consistent toolchain versions
- Reduces maintenance overhead
- Follows Ubuntu's recommended approach for cross-compilation
Also applies to: 161-162, 169-170, 177-178, 185-186, 192-193, 200-201, 207-208, 214-215, 221-222, 228-229, 235-236, 242-243, 249-250, 277-278, 284-285, 291-292
155-155: Good coverage and sanitizer configuration improvementsExcellent additions:
- Architecture-specific gcov executables ensure accurate coverage reporting
- Updated MSAN configuration with proper runtime library dependencies
Also applies to: 162-162, 170-170, 178-178, 186-186, 193-193, 201-201, 208-208, 215-215, 222-222, 229-229, 236-236, 243-243, 250-250, 258-258, 278-278, 285-285, 292-292, 314-314, 323-323, 336-336, 350-350, 362-362, 372-372, 382-382, 469-469
275-279: Consider adding VSX-specific configuration for Power8 LEBased on the PR discussion, there are specific issues with Power8 in Little Endian mode when using VSX. Consider:
- Adding explicit VSX configuration flags for Power8 LE
- Testing without VSX on Power8 LE as a fallback
Run this script to verify VSX-related configurations:
|
I have a Raspberry Pi 5 with Ubuntu 24.10 and I was able to run gtest_zlib after compiling with both clang 19 and gcc 14. So perhaps it is an issue with qemu. I'm inclined to just force those tests to use Ubuntu 22. |
Yeah, how about we have those do Ubuntu 22 and just add an issue about it being broken, that way we might remember it and could recheck it at a later time, or someone else might take it upon themselves to investigate it further. |
Support for Power8 was dropped, you need older version of toolchain. This was mentioned earlier. |
|
Where was it mentioned? |
|
I found it.. #1378 |
|
Is it just me or does qemu appear to be getting worse and worse? |
|
MSAN fails because zlib-ng repo isn’t updated to use Ubuntu 24 as ubuntu-latest yet. In my branch it works. |
It has been broken since around 6.0 and worse performance wise since they ditched the old instruction translator that was based on old GCC. |
In most cases it is better to use explicit version instead of "latest" as during deploying of new Ubuntu release every 2 years, a lot of package versions change and we rely on specific matching package versions for certain package combinations. |
It seems that qemu might be failing. Tests on Raspberry Pi 5 with Ubuntu 24.04 appear to work just fine.
bc13fd5 to
ccb4ec3
Compare
|
Rebased |
|
Still some failed runs, did not spot anything obvious, other than the Power8 issue you discussed above. |
|
Looks like my commit for PPC64LE was overwritten on rebase. |
|
Not sure about the "Package check" ones. |
inflate.c
Outdated
| if (have == 0) goto inf_leave; \ | ||
| have--; \ | ||
| hold += ((unsigned)(*next++) << bits); \ | ||
| hold += ((uint64_t)(*next++) << bits); \ |
There was a problem hiding this comment.
Is this the correct fix?
The hold variable is of type uint32_t and may overflow when assigned with addition.
There was a problem hiding this comment.
hold is uint64_t. The warning goes away, so I assume it is correct.
There was a problem hiding this comment.
There are two separate warnings for two different lines. I think they are non-issues, and AI suggested something that looks rather complicated.
There was a problem hiding this comment.
It looks like it is uint64_t in inffast_tpl.h.
There was a problem hiding this comment.
There was a problem hiding this comment.
Hmm. I think I will just let somebody else fix this static analysis problem.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infback.c(2 hunks)inflate.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- infback.c
⏰ Context from checks skipped due to timeout of 90000ms (30)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
🔇 Additional comments (1)
inflate.c (1)
390-390: LGTM! Type change aligns with the bit buffer modifications.The cast to
uint64_tensures type consistency with theholdvariable and prevents potential truncation during bit shifting operations.Given the PR objectives mentioning illegal instruction issues on PowerPC, let's verify the usage of this macro across the codebase:
✅ Verification successful
The type change in PULLBYTE macro is consistent with other implementations
The
uint64_tcast in the bit shifting operation matches the identical implementation ininfback.cand aligns with the internal type handling pattern across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other instances of PULLBYTE macro usage # to ensure consistent type handling across the codebase. rg -p "PULLBYTE\(\)" --type cLength of output: 655
Script:
#!/bin/bash # Get the macro definitions and surrounding context echo "=== infback.c PULLBYTE definition ===" rg -A 3 "^#define PULLBYTE\(\)" infback.c echo -e "\n=== inflate.c PULLBYTE definition ===" rg -A 3 "^#define PULLBYTE\(\)" inflate.c echo -e "\n=== inflate_p.h PULLBYTE definition ===" rg -A 3 "^#define PULLBYTE\(\)" inflate_p.h # Check hold variable declarations echo -e "\n=== hold variable declarations ===" rg "hold\s*;" --type cLength of output: 1593
Script:
#!/bin/bash # Get more context around PULLBYTE and PULL macros echo "=== infback.c PULLBYTE and PULL macro definitions ===" rg -B 2 -A 5 "^#define PULL\(\)" infback.c rg -B 2 -A 5 "^#define PULLBYTE\(\)" infback.c echo -e "\n=== inflate.c PULLBYTE complete definition ===" rg -B 2 -A 5 "^#define PULLBYTE\(\)" inflate.c # Check how inflate.h is included echo -e "\n=== Files including inflate.h ===" rg '#include.*inflate\.h' --type cLength of output: 1589
inflate.c
Outdated
| unsigned char *from; /* where to copy match bytes from */ | ||
| unsigned have, left; /* available input and output */ | ||
| uint32_t hold; /* bit buffer */ | ||
| uint64_t hold; /* bit buffer */ |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency found: hold variable type mismatch between files
The type change to uint64_t in inflate.c conflicts with its declaration in inflate.h (line 124) which still uses uint32_t. This needs to be synchronized.
inflate.h:124: Updateuint32_t holdtouint64_t holdto match the implementation
🔗 Analysis chain
LGTM! Variable type change ensures consistent bit buffer handling.
The change from uint32_t to uint64_t aligns with the PULLBYTE() macro modification and may help resolve the PowerPC platform issues.
Let's verify the impact of this change on memory alignment and usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other bit buffer variables that might need similar type changes
# and verify memory alignment considerations.
# Check for other uint32_t/unsigned long bit buffer declarations
rg -p "uint32_t\s+hold|unsigned\s+long\s+hold" --type c
# Check for alignment-related macros or structs
rg -p "align|packed" --type c
Length of output: 9185
e6fca08 to
a7a8c9a
Compare
Dead2
left a comment
There was a problem hiding this comment.
LGTM.
While this does not fix all the current failures, it makes it actually possible to see the rest of the failures instead of them drowning in a sea of other failures 👍
Summary by CodeRabbit
qemupackages.gcov-execspecifications for improved coverage reporting.