feat: add Linux Clang+libc++ build configuration#1633
Conversation
Up to standards ✅🟢 Issues
|
ce4ed2a to
38db10d
Compare
|
Hey @hk2166 👋 thanks for the PR! This comment updates automatically as you push changes -- think of it as your PR's live scoreboard! PR Checks✅ DCO Sign-off -- All commits have valid sign-offs. Nice work! ✅ GPG Signature -- All commits have verified GPG signatures. Locked and loaded! ✅ Merge Conflicts -- No merge conflicts detected. Smooth sailing! ✅ Issue Link -- Linked to #1620 (assigned to you). 🎉 All checks passed! Your PR is ready for review. Great job! |
github-actions
Bot
added
the
status: needs review
|
Hi @gsstoykov @leninmehedy could you please review my PR when you get a chance? I’d also appreciate any feedback or suggestions for improvement. If there’s anything that should be changed or refined, I’m happy to work on it. |
ab05113 to
63e499d
Compare
rwalworth
left a comment
rwalworth
left a comment
There was a problem hiding this comment.
Thanks for picking this one up @hk2166! Solid skeleton - the custom triplet, presets, and nightly wiring all match the shape we discussed in #1620. A few things need to be fixed before this can merge - detailed comments below. Let me know if anything's unclear and I'll take another pass once these are addressed.
| "description": "Configured via the vcpkg toolchain", | ||
| "binaryDir": "${sourceDir}/build/${presetName}", | ||
| "installDir": "${sourceDir}/package", | ||
| "generator": "Ninja Multi-Config", |
There was a problem hiding this comment.
issue (blocking): This change removes "generator": "Ninja Multi-Config" from the vcpkg-base preset. Since every existing Linux and macOS preset inherits from vcpkg-base, this drops them down to CMake's default generator on each platform (usually Unix Makefiles on Linux/macOS), which is a behavior change for the whole project - not just the new libc++ presets.
Beyond the runtime change, gtest_discover_tests is wired up with WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_BUILD_TYPE} (see src/sdk/tests/unit/CMakeLists.txt:169 and the integration equivalent). That ${CMAKE_BUILD_TYPE} subdirectory is something multi-config generators produce; single-config generators don't, so removing this will likely break test discovery on the existing presets too.
Please restore the "generator": "Ninja Multi-Config" line in vcpkg-base. If you want the new libc++ presets to use a different generator, set it explicitly on those two presets only.
The display-name / cacheVariables shorthand cleanups in this file are fine - just the generator removal needs to be reverted.
| For more detailed information about the libc++ build configuration, see [CLAUDE.md](CLAUDE.md). | ||
|
|
||
| For more detailed information about the libc++ build configuration, see [CLAUDE.md](CLAUDE.md). |
There was a problem hiding this comment.
issue (blocking): As we agreed on #1620, CLAUDE.md isn't a file we want in this repo - the mention in the issue's implementation steps was a copy-paste artifact from my template. README is the right home for all of this. Please remove both of these lines:
For more detailed information about the libc++ build configuration, see [CLAUDE.md](CLAUDE.md).
For more detailed information about the libc++ build configuration, see [CLAUDE.md](CLAUDE.md).(Also note they're duplicated - the same line appears twice in a row.)
Could you also update the PR description? It still says "Create CLAUDE.md with comprehensive developer documentation," which isn't accurate anymore.
| - name: Run Unit Tests (Debug) | ||
| run: ctest -j 6 -C Debug --test-dir build/linux-x64-clang-libcxx-debug --output-on-failure | ||
| continue-on-error: false | ||
|
|
||
| - name: Configure CMake (Release) | ||
| run: cmake --preset linux-x64-clang-libcxx-release -DBUILD_TESTS=ON | ||
|
|
||
| - name: Build (Release) | ||
| run: cmake --build --preset linux-x64-clang-libcxx-release -j 6 | ||
|
|
||
| - name: Run Unit Tests (Release) | ||
| run: ctest -j 6 -C Release --test-dir build/linux-x64-clang-libcxx-release --output-on-failure | ||
| continue-on-error: false |
There was a problem hiding this comment.
issue (blocking): The step is named "Run Unit Tests" but the ctest invocation here runs every test discovered for the preset - both unit and integration tests. Integration tests require a live Hiero network (see config/local_node.json and the Prepare Hiero Solo step in the existing build job above at zxc-build-library.yaml:211). Since this new job doesn't stand up Solo, the integration tests will fail on every nightly run as soon as the cache warms up.
The repo's README.md already shows the right pattern for unit-only runs:
ctest -j 6 -C <CONFIG> --test-dir build/<PRESET> -R "TestVectors|UnitTests" --output-on-failureYou have two reasonable options:
- (Recommended for now) Restrict the new job to unit tests by adding
-R "TestVectors|UnitTests"to bothctestinvocations. This matches the spirit of [Intermediate]: Addlinux-x64-clang-libcxxpreset and run it as a nightly build #1620 - the goal is libc++ stdlib portability for the SDK, and unit tests exercise that surface area without needing a network. - Wire in a
Prepare Hiero Solostep before the test runs (matching the existing Linux job at lines 197-205) and use-E NodeUpdateTransactionIntegrationTestslike the existing job does. This is a heavier nightly run but gives full coverage.
If you go with option 1, also rename the steps from "Run Unit Tests" to something accurate (the name is already correct in that case 🙂 - just keep it consistent).
| .setNodeAccountIds({ AccountId(3) }) | ||
| .setTransactionId(getTestTransactionIdMock()) | ||
| .setKey(privateKey->getPublicKey()) | ||
| .setKeyWithoutAlias(privateKey->getPublicKey()) |
There was a problem hiding this comment.
question (blocking): What's the reason for this setKey --> setKeyWithoutAlias change? It looks unrelated to the build-configuration scope of this PR.
I checked, and:
AccountCreateTransaction::setKey()is[[deprecated]], but the project doesn't enable-Werror=deprecated-declarations, so the deprecation alone shouldn't break the new libc++ build.- There are still ~40 other
.setKey(...)call sites across the unit and integration test suites (e.g.AccountCreateTransactionUnitTests.cc:131,AccountUpdateTransactionUnitTests.cc:154, etc.). If this fix were genuinely needed for the libc++ build, you'd expect all of them to need it.
If something about the libc++ toolchain is actually erroring on this specific line, please share the error so I can understand the underlying issue (it would likely point at a real problem we'd want to fix more broadly). Otherwise, please revert this line - we can do a deprecated-setKey sweep as its own follow-up PR.
| required: false | ||
| default: true | ||
| default: true | ||
|
|
There was a problem hiding this comment.
nitpick: Several of the new/changed lines in this file have trailing whitespace (e.g. default: true on this line, default: false on line 53, and a few of the blank "spacer" lines around the new run-clang-libcxx-build input). Also, there's a blank line just under on: near the top of the file that wasn't there before. None of this affects function - just an editor / pre-commit cleanup pass before next push.
| run-windows-builds: true | ||
| run-macos-builds: true | ||
| run-clang-libcxx-build: true | ||
| upload-artifacts: false No newline at end of file |
There was a problem hiding this comment.
nitpick: This file lost its trailing newline (upload-artifacts: false is now the last byte). Easy fix - add the newline back.
There was a problem hiding this comment.
Thanks for the detailed review. I’ve addressed the requested changes:
- Restored
Ninja Multi-Configon the sharedvcpkg-basepreset. - Removed the remaining
CLAUDE.mdreferences from the README and updated the PR description. - Restricted the new libc++ CI job to unit/test-vector tests with
-R "TestVectors|UnitTests". - Reverted the unrelated
setKeyWithoutAliastest change back tosetKey. - Cleaned up trailing whitespace and restored the newline in
on-schedule-builds.yaml.
github-actions
Bot
added
status: needs revision
Signed-off-by: hk2166 <9610hemant@gmail.com>
57af833 to
5bfa227
Compare
Added an architecture overview section detailing the SDK's components. Signed-off-by: Hemant <133942800+hk2166@users.noreply.github.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Hi @rwalworth , thanks again for the detailed review — I’ve addressed the requested changes and pushed the updates. Whenever you get a chance, could you please take another pass on the PR? Also curious to hear your thoughts on the overall Clang + libc++ setup approach (custom triplet + presets + nightly-only validation) and whether the structure looks reasonable to you going forward. |
2 participants
Description:
Add Linux Clang+libc++ build configuration to test stdlib portability on self-hosted Linux runners, providing a faster and cheaper alternative to macOS builds for catching libc++-specific issues.
x64-linux-clang-libcxxinvcpkg-triplets/linux-x64-clang-libcxx-debugandlinux-x64-clang-libcxx-releasebuild-linux-clang-libcxxjob tozxc-build-library.yamlworkflowrun-clang-libcxx-buildinput flag to workflow (defaults to false)on-schedule-builds.yamlREADME.mdRelated issue(s):
Fixes #1620
Technical Implementation
Custom vcpkg Triplet (
vcpkg-triplets/x64-linux-clang-libcxx.cmake):-stdlib=libc++-stdlib=libc++ -lc++abiCMake Presets:
vcpkg-basefor consistencyVCPKG_OVERLAY_TRIPLETSCI Integration:
run-clang-libcxx-build: trueis setlibc++-devandlibc++abi-devpackages