Replace deprecated Github runner ubuntu-20.04 #802
Conversation
Each spawned testcase is now exited to avoid running the tests multiple times. This fixes a race condition seen with a newer GNU libc version. Signed-off-by: Björn Svensson <[email protected]>
Signed-off-by: Björn Svensson <[email protected]>
WalkthroughThe CI workflow was updated to use Ubuntu 24.04, with explicit installation of OpenSSL 1.1.1f to maintain compatibility. In the test suite, process handling in fork-related tests was improved by adding exit status checks and explicit cleanup, ensuring proper verification of child process behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
26-32: Consider adding package integrity verification and error handling.The OpenSSL 1.1.1f installation approach is sound for maintaining compatibility, but could be improved for robustness and security.
Consider this enhanced version with integrity checks and error handling:
- # Replace installed OpenSSL with the supported version 1.1.1 - curl -O http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl1.1_1.1.1f-1ubuntu2.24_amd64.deb - curl -O http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl-dev_1.1.1f-1ubuntu2.24_amd64.deb - curl -O http://security.ubuntu.com/ubuntu/pool/main/o/openssl/openssl_1.1.1f-1ubuntu2.24_amd64.deb - sudo dpkg -i --force-confnew openssl_1.1.1f-1ubuntu2.24_amd64.deb \ - libssl1.1_1.1.1f-1ubuntu2.24_amd64.deb \ - libssl-dev_1.1.1f-1ubuntu2.24_amd64.deb + # Replace installed OpenSSL with the supported version 1.1.1 + set -e + curl -fsSL -O http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl1.1_1.1.1f-1ubuntu2.24_amd64.deb + curl -fsSL -O http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl-dev_1.1.1f-1ubuntu2.24_amd64.deb + curl -fsSL -O http://security.ubuntu.com/ubuntu/pool/main/o/openssl/openssl_1.1.1f-1ubuntu2.24_amd64.deb + # Verify packages exist and are valid + dpkg --info openssl_1.1.1f-1ubuntu2.24_amd64.deb > /dev/null + dpkg --info libssl1.1_1.1.1f-1ubuntu2.24_amd64.deb > /dev/null + dpkg --info libssl-dev_1.1.1f-1ubuntu2.24_amd64.deb > /dev/null + sudo dpkg -i --force-confnew openssl_1.1.1f-1ubuntu2.24_amd64.deb \ + libssl1.1_1.1.1f-1ubuntu2.24_amd64.deb \ + libssl-dev_1.1.1f-1ubuntu2.24_amd64.debThis adds:
-fsSLflags to curl for better error handling and securityset -eto fail fast on errorsdpkg --infochecks to verify package validity before installation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(2 hunks)src/lib/test/ForkTests.cpp(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Linux (openssl)
- GitHub Check: macOS (botan)
- GitHub Check: macOS (openssl)
- GitHub Check: Linux (botan)
🔇 Additional comments (6)
.github/workflows/ci.yml (1)
13-13: LGTM: Ubuntu runner updated correctly.The migration from
ubuntu-20.04toubuntu-24.04is necessary as the former is deprecated. This change aligns with GitHub's recommended practices.src/lib/test/ForkTests.cpp (5)
43-43: LGTM: Proper header included for process synchronization.Adding
<sys/wait.h>is necessary for thewaitpid()functionality used in the improved fork tests.
68-68: LGTM: Status variables added for child process monitoring.The
int statusvariables are properly declared to capture child process exit information for verification.Also applies to: 108-108
83-88: Excellent: Proper child process cleanup and deterministic exit.The improvements address the GLIBC 2.34 fork issues mentioned in the PR objectives:
- Added explicit
C_Finalize(NULL_PTR)to ensure proper resource cleanup in child process- Using
_exit(EXIT_SUCCESS)prevents the child from running parent cleanup code- Clear comment explains the expected behavior
These changes eliminate race conditions and ensure deterministic test behavior.
94-96: Excellent: Robust parent-child synchronization implemented.The parent process now properly waits for and verifies child completion:
waitpid(pid, &status, 0)ensures synchronization and prevents race conditionsWIFEXITED(status) && WEXITSTATUS(status) == EXIT_SUCCESSverifies successful child termination- This addresses the Ubuntu 24.04/GLIBC 2.34 compatibility issues mentioned in the PR
This is exactly the type of fix needed for reliable fork testing.
132-142: LGTM: Consistent improvements applied to testResetOnFork.The same robust child process cleanup and parent synchronization patterns are correctly applied to the
testResetOnForkfunction, ensuring consistency across all fork-related tests.
|
I have already done this in #783 . Except the OpenSSL 1.1.1 part which I'm not sure is that useful nowadays (no one probably runs it anymore). |
|
Although it might be still good idea to have 1.1.1 in the pipeline. Maybe merging our approaches together would be best. In that case it could be added to Ubuntu 22.04 runner and 24.04 would stay to test OpenSSL 3.0. |
|
I think this is actually better as it's limited so it's better to merge this one and then I can split my changes and extend pipeline... |
|
@jschlyter Thanks for merging this. I created a new PR with updated changes for OpenSSL 3.0 runner in #806 so if you could check that one out, it would be awesome! |
4 participants
Replace deprecated Github runner
ubuntu-20.04withubuntu-24.04When running with a newer ubuntu a new issue surfaces with the fork test,
which probably is due to the lift of GLIBC, from v2.31 to v2.34.
Others have this related issue.
An fix for this is to wait for spawned fork-tests to avoid any race conditions.
Each spawned testcase is now exited to avoid running the tests multiple times.
Summary by CodeRabbit
Chores
Tests