fix: enable CI-nightly run normally#924
Conversation
Signed-off-by: Jason <libevent@yeah.net>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #924 +/- ##
=========================================
Coverage 80.86% 80.86%
Complexity 615 615
=========================================
Files 94 94
Lines 10742 10742
Branches 1060 1060
=========================================
Hits 8687 8687
Misses 1815 1815
Partials 240 240 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Jason <libevent@yeah.net>
Signed-off-by: Jason <libevent@yeah.net>
Signed-off-by: Jason <libevent@yeah.net>
Signed-off-by: Jason <libevent@yeah.net>
SYaoJun
left a comment
There was a problem hiding this comment.
PR Review
Summary
This PR aims to fix CI nightly build failures (closes #919) by:
- Adding missing
add_dependenciesfor arrow static library targets inapache-arrow.cmake - Upgrading CMake, switching to Ninja, and modernizing the CI workflow scripts
- Changing
ci-nightly.ymltriggers from scheduled cron to push/PR on main - Disabling all other CI workflows via
if: false
Critical Issues
1. 🔴 All other CI workflows disabled (if: false) — must be reverted
Files affected: ci.yml, docs.yml, java-info.yml, java.yml, license.yml, pr-title.yml, pre-commit.yml, publish-docker.yml, pyspark.yml, python-wheel-workflow.yml, python.yml
This PR disables 11 CI workflow jobs across 7 workflow files by adding if: false. This means:
- No license checks
- No pre-commit checks (clang-format, cpplint)
- No PR title validation
- No Python/Java/Rust/pyspark builds
- No Docker publishing
- No docs builds
These should absolutely not be in the final merge. This looks like a local debugging shortcut that was committed accidentally. All if: false lines must be removed.
2. 🔴 ci-nightly.yml trigger change breaks its purpose
The workflow name is literally "GraphAr C++ CI Nightly" and the original trigger was a daily cron schedule. Changing it to push/pull_request on main makes it a regular CI — not nightly at all. If the intent is to also run on push/PR for testing, add those triggers in addition to the existing schedule, rather than replacing it.
3. 🔴 CI currently fails — add_dependencies called with incorrect number of arguments
CI log shows:
CMake Error at cmake/apache-arrow.cmake:201 (add_dependencies):
add_dependencies called with incorrect number of arguments
Line 201 in the main branch (after the merge of this PR) is:
add_dependencies( arrow_ep)This is called unconditionally — but GAR_ARROW_ACERO_LIBRARY_TARGET is only defined inside the if(ARROW_VERSION_TO_BUILD GREATER_EQUAL "12.0.0") block. Wait — looking more carefully, it is inside an if block in the PR diff. Let me re-check...
Actually, the PR adds the acero target definition before ExternalProject_Add (line 138-187), which is correct. But the real error at line 201 is the new block added at the end:
add_dependencies( arrow_ep)The error message says "incorrect number of arguments", which means one of the variables is empty. On CI (gcc 13, Arrow 24), ARROW_VERSION_TO_BUILD defaults to "24.0.0" which is >= "12.0.0", so the acero block should run. The issue might be that GAR_ARROW_ACERO_STATIC_LIB is a CACHE variable set inside the if-block, but GAR_ARROW_ACERO_LIBRARY_TARGET is a local variable set inside the same if-block — these are in the function scope so should be fine.
Root cause: The add_dependencies calls at lines 198-201 are added after the original endfunction(). Wait, no — looking at the diff more carefully, the original function ended at the old line 195. The new code adds dependencies inside the function before endfunction(). But the CI error is at line 201... Let me trace this.
The actual bug: In the PR diff, the acero library target is set up before externalproject_add, but add_dependencies for acero is at the end. The variable GAR_ARROW_ACERO_LIBRARY_TARGET should be in scope. However, the CI uses the base branch version of apache-arrow.cmake (line numbers differ). This error needs to be reproduced against the PR branch.
4. 🟡 ctest -C build-debug is incorrect usage
- ctest --output-on-failure
+ ctest --output-on-failure -C build-debug-C is a CTest configuration type flag (e.g., Debug, Release), not a build directory path. The correct way is:
ctest --output-on-failure --test-dir build-debugSince the workflow already does ninja -C build-debug which puts you in the correct directory context, the original ctest --output-on-failure (run from within the build dir) or ctest --output-on-failure --test-dir build-debug would work. The current \-C build-debug will silently pass but won't actually find any tests.
Moderate Issues
5. 🟡 Dockerfile: ubuntu:latest is a moving target
-FROM ubuntu:22.04
+FROM ubuntu:latestPin to a specific version (e.g., ubuntu:24.04) to ensure reproducible builds. latest can break unexpectedly when a new Ubuntu release drops.
6. 🟡 Dockerfile: CMake 3.28.0 is outdated
The Dockerfile installs CMake 3.28.0 via manual download, but the CI workflow installs CMake from the Kitware apt repo (which would give a newer version). These should be consistent. Consider using the same Kitware approach in the Dockerfile, or pinning the same version in CI.
7. 🟡 Duplicate CMake in Dockerfile
The Dockerfile removes cmake from apt-get install but then downloads CMake 3.28.0 separately. It also removes cmake but still installs ninja-build. The manual CMake download tarball should use a checksum (sha256sum) to verify integrity.
Minor / Style
8. 🟢 .gitignore: adding build-debug is fine but incomplete
Better to use build*/ pattern or build-*/ to also cover build-release, build-relwithdebinfo, etc.
9. 🟢 CI style: pushd/popd removal is good
The switch from pushd/popd to -C flags and -B/-S cmake args is cleaner and less error-prone. Good change.
Recommendation
Request Changes — This PR mixes genuine fixes with CI-disabling hacks. Please:
- Remove all
if: falsefrom other workflow files (or split into a separate PR) - Fix the
ctest -Cusage toctest --test-diror justctestinside the build dir - Keep the
ci-nightly.ymltrigger as schedule + push/PR (not just push/PR) - Verify the
apache-arrow.cmakechanges actually fix the CI failure (currently failing) - Pin Dockerfile to
ubuntu:24.04instead oflatest
Reason for this PR
close: #919
What changes are included in this PR?
add missing link library of apache arrow