Fix build tarball scripts

[yi-xmu]

Summary by CodeRabbit

• Chores
  • Enhanced build configuration consistency for logging state propagation across all build targets.
  • Improved cloud storage configuration handling for different deployment scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This change propagates the WITH_LOG_STATE variable through cmake invocations in the build tarball script and adjusts conditional logic for cloud storage scenarios. The variable is consistently passed to log service and data-store build configurations without altering overall control flow.

Changes

Cohort / File(s)	Change Summary
Build configuration variable propagation concourse/scripts/build_tarball.bash	Adds -DWITH_LOG_STATE=${WITH_LOG_STATE} to multiple top-level cmake invocations; extends cmake_args in build_upload_log_srv to include the variable; adjusts conditional logic for DATA_STORE_TYPE branches by replacing specific WITH_LOG_STATE values (ROCKSDB_CLOUD_S3/ROCKSDB_CLOUD_GCS) with alternative flags (CLOUD_AZ_INFO) and removes explicit WITH_LOG_STATE settings in those branches

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Variable propagation consistency across multiple cmake invocations requires verification
• Conditional logic adjustments in cloud storage branches need careful scrutiny to ensure intended behavior
• Cross-referencing with downstream build components (log service, data store) recommended

Poem

🐰 Through cmake's winding garden path,
I carry logs with rightful wrath,
WITH_LOG_STATE flows, both near and far,
Cloud branches gleam like evening star,
Build scripts dance—hooray, hooray! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	❓ Inconclusive	The title 'Fix build tarball scripts' is vague and generic, lacking specific details about what issue is being addressed or which aspect of the build process is being fixed.	Provide a more specific title that describes the actual fix, such as 'Propagate WITH_LOG_STATE variable through CMake build configurations' or 'Fix CMAKE variable propagation in tarball build scripts'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix_tarball_scripts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

concourse/scripts/build_tarball.bash (1)

279-279: Inconsistent variable expansion syntax on line 279.

Line 279 uses $WITH_LOG_STATE without braces, while lines 205 and 238 use ${WITH_LOG_STATE} with braces. For consistency and to avoid potential issues with adjacent characters, use the braced form throughout.

-    local cmake_args="-DCMAKE_BUILD_TYPE=$BUILD_TYPE -DWITH_ASAN=$ASAN -DDISABLE_CODE_LINE_IN_LOG=ON -DWITH_LOG_STATE=$WITH_LOG_STATE"
+    local cmake_args="-DCMAKE_BUILD_TYPE=$BUILD_TYPE -DWITH_ASAN=$ASAN -DDISABLE_CODE_LINE_IN_LOG=ON -DWITH_LOG_STATE=${WITH_LOG_STATE}"

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec85d6c and f443a66.

📒 Files selected for processing (1)

• concourse/scripts/build_tarball.bash (3 hunks)

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 127
File: concourse/scripts/build_tarball_open.bash:136-136
Timestamp: 2025-09-25T11:33:33.221Z
Learning: The open log service (OPEN_LOG_SERVICE=ON) in concourse/scripts/build_tarball_open.bash only supports ROCKSDB as its log state and does not use the WITH_LOG_STATE parameter that was introduced to replace USE_ROCKSDB_LOG_STATE in other parts of the codebase.

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 127
File: storage/eloq/build_log_service.cmake:97-124
Timestamp: 2025-09-25T11:41:40.111Z
Learning: The `storage/eloq/build_log_service.cmake` file is specifically for the open log service build and only supports ROCKSDB as its log state backend. Unlike the enterprise version in `build_eloq_log_service.cmake`, it does not need to handle cloud-specific variants like ROCKSDB_CLOUD_S3 or ROCKSDB_CLOUD_GCS.

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 127
File: concourse/scripts/main.bash:164-165
Timestamp: 2025-09-25T11:40:48.717Z
Learning: The main.bash script in concourse/scripts/main.bash is for open log build, which only supports ROCKSDB as the log state option and does not need cloud-specific variants like ROCKSDB_CLOUD_S3.

📚 Learning: 2025-09-25T11:33:33.221Z

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 127
File: concourse/scripts/build_tarball_open.bash:136-136
Timestamp: 2025-09-25T11:33:33.221Z
Learning: The open log service (OPEN_LOG_SERVICE=ON) in concourse/scripts/build_tarball_open.bash only supports ROCKSDB as its log state and does not use the WITH_LOG_STATE parameter that was introduced to replace USE_ROCKSDB_LOG_STATE in other parts of the codebase.

Applied to files:

• concourse/scripts/build_tarball.bash

📚 Learning: 2025-09-25T11:41:40.111Z

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 127
File: storage/eloq/build_log_service.cmake:97-124
Timestamp: 2025-09-25T11:41:40.111Z
Learning: The `storage/eloq/build_log_service.cmake` file is specifically for the open log service build and only supports ROCKSDB as its log state backend. Unlike the enterprise version in `build_eloq_log_service.cmake`, it does not need to handle cloud-specific variants like ROCKSDB_CLOUD_S3 or ROCKSDB_CLOUD_GCS.

Applied to files:

• concourse/scripts/build_tarball.bash

📚 Learning: 2025-09-25T11:40:48.717Z

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 127
File: concourse/scripts/main.bash:164-165
Timestamp: 2025-09-25T11:40:48.717Z
Learning: The main.bash script in concourse/scripts/main.bash is for open log build, which only supports ROCKSDB as the log state option and does not need cloud-specific variants like ROCKSDB_CLOUD_S3.

Applied to files:

• concourse/scripts/build_tarball.bash

🔇 Additional comments (3)

concourse/scripts/build_tarball.bash (3)

280-281: Conditional cloud storage flag handling looks correct.

The build_upload_log_srv function correctly mirrors the main script's approach: only ELOQDSS_ROCKSDB_CLOUD_S3 receives the CLOUD_AZ_INFO flag (lines 280–281), while ELOQDSS_ROCKSDB_CLOUD_GCS does not, consistent with the conditional logic at lines 119–123.

---

205-206: The concern about WITH_LOG_STATE being undefined is not supported by the codebase. All Concourse pipeline configurations (build_debug_tarball.yml, build_nightly_tarball.yml, build_release_tarball.yml) consistently define WITH_LOG_STATE as an environment variable before invoking the script—with values such as ROCKSDB, ROCKSDB_CLOUD_S3, or ROCKSDB_CLOUD_GCS. The script correctly relies on the caller to provide this variable, and all observed calling contexts comply with this requirement.

Likely an incorrect or invalid review comment.

---

228-228: The data store service build correctly does not receive -DWITH_LOG_STATE. This is intentional and correct. WITH_LOG_STATE is specifically for log state configuration (MEMORY, ROCKSDB, ROCKSDB_CLOUD variants), used by the main eloqsql and log service builds. The DSS build uses -DWITH_DATA_STORE instead, which is the appropriate configuration variable for the data store service. These are separate, purpose-specific parameters for different services with different architectural concerns.

Likely an incorrect or invalid review comment.