fix(ci): align JNI LTO compiler - #5
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChanges基准日志工件发布
JNI 构建与 mmap 初始化
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BenchmarkWorkflow
participant LogPageScript
participant ActionsArtifact
participant GitHubPages
BenchmarkWorkflow->>ActionsArtifact: 上传 LOG-* 工件
BenchmarkWorkflow->>LogPageScript: 传入 Actions 运行 URL
LogPageScript->>LogPageScript: 收集工件名称
LogPageScript->>GitHubPages: 生成外部工件链接
GitHubPages->>ActionsArtifact: 链接至运行工件
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the Makefile to conditionally clone the 'topling-rocks' and 'cspp-memtable' repositories only when 'UPDATE_REPO' is not set to '0'. Additionally, it excludes the prebuilt source file 'cspp_memtable.cc' from 'ALL_SOURCES' to prevent dependency generation issues. The reviewer feedback suggests also excluding 'top_zip_table_builder.cc' from 'ALL_SOURCES' to avoid similar build dependency failures, and wrapping the cloning of the 'cspp-wbwi' repository in the same 'UPDATE_REPO' conditional check for consistency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # Exclude build_version.cc -- a generated source file -- from all sources. Not needed for dependencies | ||
| ALL_SOURCES = $(filter-out util/build_version.cc, $(LIB_SOURCES)) $(TEST_LIB_SOURCES) $(MOCK_LIB_SOURCES) $(GTEST_DIR)/gtest/gtest-all.cc | ||
| # Exclude generated and prebuilt sources that do not need dependencies. | ||
| ALL_SOURCES = $(filter-out util/build_version.cc sideplugin/topling-zip_table_reader/cspp_memtable.cc, $(LIB_SOURCES)) $(TEST_LIB_SOURCES) $(MOCK_LIB_SOURCES) $(GTEST_DIR)/gtest/gtest-all.cc |
There was a problem hiding this comment.
Just like cspp_memtable.cc, the synthetic source file sideplugin/topling-zip_table_reader/top_zip_table_builder.cc (added to EXTRA_LIB_SOURCES at line 653 when TOPLING_ZIP_TABLE_TRIAL_DAYS is set and topling-rocks is missing) does not exist on disk and has no rule to be generated in this scenario.\n\nIf TOPLING_ZIP_TABLE_TRIAL_DAYS is set, make depend will fail with a similar error:\ntext\nNo rule to make target 'sideplugin/topling-zip_table_reader/top_zip_table_builder.cc',\nneeded by '.../sideplugin/topling-zip_table_reader/top_zip_table_builder.cc.d'.\n\n\nTo prevent this, we should also exclude sideplugin/topling-zip_table_reader/top_zip_table_builder.cc from ALL_SOURCES so that its dependency file is not generated.
ALL_SOURCES = $(filter-out util/build_version.cc sideplugin/topling-zip_table_reader/cspp_memtable.cc sideplugin/topling-zip_table_reader/top_zip_table_builder.cc, $(LIB_SOURCES)) $(TEST_LIB_SOURCES) $(MOCK_LIB_SOURCES) $(GTEST_DIR)/gtest/gtest-all.cc
| endif | ||
|
|
||
| ifeq (,$(wildcard sideplugin/cspp-memtable)) | ||
| ifneq (${UPDATE_REPO},0) |
There was a problem hiding this comment.
While you have successfully protected the cspp-memtable and topling-rocks clones when UPDATE_REPO=0, the private repository cspp-wbwi (cloned around line 491) is still cloned unconditionally when it is missing.\n\nTo avoid unnecessary clone attempts and potential failures/delays in CI, you should also wrap the cspp-wbwi clone in a similar ifneq (${UPDATE_REPO},0) check.
There was a problem hiding this comment.
Pull request overview
This PR fixes JNI CI dependency-generation failures when the private CSPP MemTable sources are unavailable by ensuring Make’s depend step no longer tries to generate .d files for a synthetic, intentionally-nonexistent cspp_memtable.cc path, while keeping the prebuilt cspp_memtable.o download rule usable for linking.
Changes:
- Guard private-repository clone probes (
topling-rocks,cspp-memtable) behindUPDATE_REPO!=0soUPDATE_REPO=0builds don’t attempt them. - Exclude
sideplugin/topling-zip_table_reader/cspp_memtable.ccfromALL_SOURCESso it doesn’t produce a*.cc.ddependency target duringmake depend.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2b3b321 to
b70bb33
Compare
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
475-482: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value移除多余的
cd命令在这个
shell调用中，cd cspp-memtable;是最后一条执行的命令。由于它位于执行完毕后即被销毁的子 Shell 中，且其后没有任何操作，这个目录切换动作实际上是无效的，可以安全地移除以保持代码整洁。💡 建议的修改
ifneq (${UPDATE_REPO},0) # topling specific: just for people who has permission to cspp-memtable dummy := $(shell set -e -x; \ cd sideplugin; \ - git clone https://github.com/topling/cspp-memtable; \ - cd cspp-memtable; \ + git clone https://github.com/topling/cspp-memtable \ ) endif🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 475 - 482, Remove the trailing `cd cspp-memtable;` command from the `dummy` shell invocation guarded by `UPDATE_REPO`, leaving the repository clone operation and shell structure unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Makefile`:
- Around line 475-482: Remove the trailing `cd cspp-memtable;` command from the
`dummy` shell invocation guarded by `UPDATE_REPO`, leaving the repository clone
operation and shell structure unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd9d5db2-226d-4721-9bde-3f6cf4f3bbda
📒 Files selected for processing (13)
.github/workflows/sanity_check.yml.github/workflows/topling-jni-release.yml.github/workflows/topling-jni.ymlMakefileTARGETSdb/db_memtable_test.ccenv/fs_cat.ccenv/fs_cat.hfile/writable_file_writer.ccjava/rocksjni/side_plugin_repo_jni.ccutil/thread_local.ccutilities/transactions/lock/point/point_lock_manager.ccutilities/write_batch_with_index/write_batch_with_index_test.cc
🚧 Files skipped from review as they are similar to previous changes (11)
- util/thread_local.cc
- .github/workflows/topling-jni-release.yml
- .github/workflows/topling-jni.yml
- TARGETS
- file/writable_file_writer.cc
- utilities/write_batch_with_index/write_batch_with_index_test.cc
- env/fs_cat.h
- env/fs_cat.cc
- java/rocksjni/side_plugin_repo_jni.cc
- db/db_memtable_test.cc
- utilities/transactions/lock/point/point_lock_manager.cc
LOG files are too large for the pages repo; link to the run's Artifacts instead.
…eaks Under C++17, std::atomic's default constructor does not zero-initialize. intrusive_ptr refcounts can start from heap garbage and never reach zero, so .blob mappings remain in Shared RSS after compact deletes the files.
Trial synthetic CSPP sources can fail DEPFILES generation; keep the build going like the implicit -include path used by db_bench.
- pin both JNI workflows to Ubuntu 24.04 - select GCC 13 for prebuilt trial LTO objects - remove the stale GCC 11 version hint
imbajin
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/db_bench-dcompact-run.yml:
- Line 166: Allow artifact replacement on reruns by updating both
upload-artifact steps: .github/workflows/db_bench-dcompact-run.yml lines 164-169
and .github/workflows/db_bench-run.yml lines 442-448. Set overwrite: true for
each upload, or include github.run_attempt in both db-info-logs-${{ env.VARIANT
}} artifact names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a99b2954-8e7c-4095-b54b-aaeb108147f0
📒 Files selected for processing (7)
.github/scripts/bench_dcompact_pages.py.github/scripts/bench_logs_to_pages.py.github/workflows/db_bench-dcompact-run.yml.github/workflows/db_bench-run.yml.github/workflows/topling-jni-release.yml.github/workflows/topling-jni.ymlenv/file_system.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/topling-jni.yml
- .github/workflows/topling-jni-release.yml
| - name: Upload DB INFO LOGs (Actions artifact) | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: db-info-logs-${{ env.VARIANT }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow references =="
rg -n "upload-artifact|db-info-logs|mantisbt" .github/workflows/db_bench*.yml || true
echo
echo "== relevant slices =="
for f in .github/workflows/db_bench-dcompact-run.yml .github/workflows/db_bench-run.yml; do
echo "--- $f ---"
nl -ba "$f" | sed -n '140,180p;430,455p'
done
echo
echo "== all upload-artifact configs in workflows =="
rg -n "upload-artifact|overwrite:|github.run_attempt|name:" .github/workflows/db_bench*.ymlRepository: hugegraph/toplingdb
Length of output: 822
🌐 Web query:
actions/upload-artifact v4 overwrite true run attempts artifact names same workflow run
💡 Result:
In GitHub Actions, actions/upload-artifact@v4 enforces immutability for artifacts [1][2]. Unlike previous versions, you cannot modify an existing artifact or upload to the same artifact name multiple times within the same workflow run without explicit configuration [3][2]. By default, attempting to upload an artifact with a name that already exists in the same workflow run will cause the action to fail [4][5]. To manage scenarios where you want to replace an existing artifact, you can use the overwrite: true input [4][6]. When overwrite: true is set, the action will delete the existing artifact with the matching name before uploading the new one [4][5]. It is important to note the following: 1. New ID: The new artifact will be assigned an entirely new ID, and the previous artifact will no longer exist [4][5]. 2. Not for concurrency: overwrite: true is intended for replacing an artifact, not for enabling concurrent uploads from multiple jobs to the same artifact name [7]. Uploading to the same artifact name across multiple jobs is not supported in v4 [2][5]. If you need to combine outputs from multiple jobs, the recommended approach is to give each artifact a unique name (e.g., using matrix variables) and then use the merge-multiple functionality when downloading them [1][3][7].
Citations:
- 1: https://github.com/actions/upload-artifact/blob/main/docs/MIGRATION.md
- 2: https://github.com/actions/upload-artifact/tree/v4.0.0
- 3: https://github.blog/news-insights/product-news/get-started-with-v4-of-github-actions-artifacts/
- 4: https://github.com/actions/upload-artifact/blob/v4/README.md
- 5: https://github.com/actions/upload-artifact
- 6: Ability to overwrite an Artifact actions/upload-artifact#501
- 7: [bug] (v4) Unable to upload to same artifact name from multiple jobs actions/upload-artifact#478
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant slices =="
for f in .github/workflows/db_bench-dcompact-run.yml .github/workflows/db_bench-run.yml; do
echo "--- $f ---"
awk 'NR>=140 && NR<=180 {printf "%5d %s\n", NR, $0}' "$f"
printf '\n'
awk 'NR>=430 && NR<=455 {printf "%5d %s\n", NR, $0}' "$f"
printf '\n--- all upload-artifact config in $f ---\n'
awk '/upload-artifact|overwrite:|name:|persist-credentials:|retention-days:/{start=NR-5; end=NR+10} start && NR>=start && NR<=end+5 {printf "%5d %s\n", NR, $0}' "$f" | tail -50
done
echo
echo "== dependency on these artifacts =="
rg -n "db-info-logs|download-artifact|github.run_attempt" .github/workflows/db_bench*.ymlRepository: hugegraph/toplingdb
Length of output: 10158
重跑时允许覆盖上传的 artifact
在 upload-artifact@v4 中，同一 workflow run 使用同一名 artifact 会失败；请在两处上传步骤中设置 overwrite: true，或在 db-info-logs-${{ env.VARIANT }} 中纳入 github.run_attempt。
.github/workflows/db_bench-dcompact-run.yml#L164-L169.github/workflows/db_bench-run.yml#L442-L448
📍 Affects 2 files
.github/workflows/db_bench-dcompact-run.yml#L166-L166(this comment).github/workflows/db_bench-run.yml#L445-L445
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/db_bench-dcompact-run.yml at line 166, Allow artifact
replacement on reruns by updating both upload-artifact steps:
.github/workflows/db_bench-dcompact-run.yml lines 164-169 and
.github/workflows/db_bench-run.yml lines 442-448. Set overwrite: true for each
upload, or include github.run_attempt in both db-info-logs-${{ env.VARIANT }}
artifact names.
Source: MCP tools
0x00 Summary
Align both JNI workflows with the GCC 13 LTO toolchain used by the prebuilt CSPP trial objects.
gcc-13/g++-130x01 Root cause
The prebuilt
cspp_memtable.ocontains GCC 13.1 LTO bytecode, while the previous JNI workflow linked it with GCC 11.3:0x02 Scope
topling-jni.ymltopling-jni-release.ymlMakefileunchangedThe non-fatal
make dependerrors are handled upstream by635f082(ci(jni): ignore explicit make depend failures).0x03 Validation
635f082git diff --checkSummary by CodeRabbit
新增功能
构建与发布
Bug 修复