carverauto/serviceradar

Fork 0

Updates/bazel fixes 2 #2447

Merged

mfreeman451 merged 2 commits from refs/pull/2447/head into main

2025-11-23 09:21:40 +00:00

mfreeman451 commented

2025-11-23 09:21:29 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1979
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1979
Original created: 2025-11-23T09:21:29Z
Original updated: 2025-11-23T09:23:05Z
Original head: carverauto/serviceradar:updates/bazel_fixes_2
Original base: main
Original merged: 2025-11-23T09:21:40Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Enhancement

Description

Add cmake_share filegroup to cmake prebuilt archive
Introduce flex, bison, and gperf source dependencies via http_archive
Build flex, bison, gperf from source in age_extension_layer for glibc compatibility
Create BUILD.bazel files for pg_extension_toolchain with source_tree targets
Fix cmake path resolution to handle both relative and absolute paths

Diagram Walkthrough

flowchart LR
  A["MODULE.bazel<br/>Add dependencies"] --> B["http_archive<br/>flex, bison, gperf"]
  B --> C["BUILD.bazel files<br/>Create source_tree targets"]
  C --> D["age_extension_layer<br/>Build from source"]
  E["cmake_share<br/>filegroup"] --> D
  F["Path resolution<br/>fix"] --> D

File Walkthrough

Relevant files

Configuration changes

MODULE.bazel `Add toolchain source dependencies to MODULE.bazel` MODULE.bazel Add `cmake_share` filegroup to cmake_linux_amd64_prebuilt archive Add three new http_archive definitions for flex_src, bison_src, and gperf_src Each archive includes build_file_content with filegroup for all sources	+52/-0
BUILD.bazel `Add bison source_tree target via copy_to_directory` third_party/pg_extension_toolchain/bison/BUILD.bazel Create new BUILD.bazel file with copy_to_directory rule Define source_tree target that copies bison sources from @bison_src Enable external repository inclusion for proper source access	+10/-0
BUILD.bazel `Add flex source_tree target via copy_to_directory` third_party/pg_extension_toolchain/flex/BUILD.bazel Create new BUILD.bazel file with copy_to_directory rule Define source_tree target that copies flex sources from @flex_src Enable external repository inclusion for proper source access	+10/-0
BUILD.bazel `Add gperf source_tree target via copy_to_directory` third_party/pg_extension_toolchain/gperf/BUILD.bazel Create new BUILD.bazel file with copy_to_directory rule Define source_tree target that copies gperf sources from @gperf_src Enable external repository inclusion for proper source access	+10/-0

Enhancement

BUILD.bazel

Build toolchain dependencies from source for age extension

docker/images/BUILD.bazel

Add @cmake_linux_amd64_prebuilt//:cmake_share to cmake tools
dependency
Fix cmake path resolution to handle both relative and absolute paths
with conditional logic
Add bison, gperf, flex source_tree dependencies to age_extension_layer
srcs
Implement complete build process for flex, bison, gperf from source
with proper environment variables
Set up PATH, PKG_CONFIG_PATH, LD_LIBRARY_PATH, and BISON_PKGDATADIR
for age extension build
Copy flex libraries to install prefix for runtime compatibility

+83/-6

Imported from GitHub pull request. Original GitHub pull request: #1979 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1979 Original created: 2025-11-23T09:21:29Z Original updated: 2025-11-23T09:23:05Z Original head: carverauto/serviceradar:updates/bazel_fixes_2 Original base: main Original merged: 2025-11-23T09:21:40Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Enhancement ___ ### **Description** - Add cmake_share filegroup to cmake prebuilt archive - Introduce flex, bison, and gperf source dependencies via http_archive - Build flex, bison, gperf from source in age_extension_layer for glibc compatibility - Create BUILD.bazel files for pg_extension_toolchain with source_tree targets - Fix cmake path resolution to handle both relative and absolute paths ___ ### Diagram Walkthrough ```mermaid flowchart LR A["MODULE.bazel Add dependencies"] --> B["http_archive flex, bison, gperf"] B --> C["BUILD.bazel files Create source_tree targets"] C --> D["age_extension_layer Build from source"] E["cmake_share filegroup"] --> D F["Path resolution fix"] --> D ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Configuration changes</td><td><table> <tr> <td> <details> <summary>MODULE.bazel<dd><code>Add toolchain source dependencies to MODULE.bazel</code>                </dd></summary> <hr> MODULE.bazel <ul><li>Add <code>cmake_share</code> filegroup to cmake_linux_amd64_prebuilt archive <li> Add three new http_archive definitions for flex_src, bison_src, and gperf_src <li> Each archive includes build_file_content with filegroup for all sources</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1979/files#diff-6136fc12446089c3db7360e923203dd114b6a1466252e71667c6791c20fe6bdc">+52/-0</a>    </td> </tr> <tr> <td> <details> <summary>BUILD.bazel<dd><code>Add bison source_tree target via copy_to_directory</code>              </dd></summary> <hr> third_party/pg_extension_toolchain/bison/BUILD.bazel <ul><li>Create new BUILD.bazel file with copy_to_directory rule <li> Define source_tree target that copies bison sources from @bison_src <li> Enable external repository inclusion for proper source access</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1979/files#diff-fbfc10a476d09ca7991b9f862d8d38af205a28a50675e450c0344ec1361466cf">+10/-0</a>    </td> </tr> <tr> <td> <details> <summary>BUILD.bazel<dd><code>Add flex source_tree target via copy_to_directory</code>                </dd></summary> <hr> third_party/pg_extension_toolchain/flex/BUILD.bazel <ul><li>Create new BUILD.bazel file with copy_to_directory rule <li> Define source_tree target that copies flex sources from @flex_src <li> Enable external repository inclusion for proper source access</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1979/files#diff-eaaee0b20fb60e67e510c7f15a7e7ac3677d3457025e052cd94d5b3b81b2f575">+10/-0</a>    </td> </tr> <tr> <td> <details> <summary>BUILD.bazel<dd><code>Add gperf source_tree target via copy_to_directory</code>              </dd></summary> <hr> third_party/pg_extension_toolchain/gperf/BUILD.bazel <ul><li>Create new BUILD.bazel file with copy_to_directory rule <li> Define source_tree target that copies gperf sources from @gperf_src <li> Enable external repository inclusion for proper source access</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1979/files#diff-0e60452a6fc2abd7b6e9a189c7d04bd6ba6dfca0048f454653b167169da73ec7">+10/-0</a>    </td> </tr> </table></td></tr><tr><td>Enhancement</td><td><table> <tr> <td> <details> <summary>BUILD.bazel<dd><code>Build toolchain dependencies from source for age extension</code></dd></summary> <hr> docker/images/BUILD.bazel <ul><li>Add <code>@cmake_linux_amd64_prebuilt//:cmake_share</code> to cmake tools dependency <li> Fix cmake path resolution to handle both relative and absolute paths with conditional logic <li> Add bison, gperf, flex source_tree dependencies to age_extension_layer srcs <li> Implement complete build process for flex, bison, gperf from source with proper environment variables <li> Set up PATH, PKG_CONFIG_PATH, LD_LIBRARY_PATH, and BISON_PKGDATADIR for age extension build <li> Copy flex libraries to install prefix for runtime compatibility</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1979/files#diff-0e4db31c224a8f72ae8e870a849e38a59d74a2c7f7b04347b0b3eb07e20c5a80">+83/-6</a>    </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-11-23 09:22:04 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1979#issuecomment-3567667895
Original created: 2025-11-23T09:22:04Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Untrusted build execution Description: Building and executing third-party autotools configure/make steps at build time without verification or sandboxing (flex, gperf, bison) risks executing untrusted build scripts on the build host; although archives are pinned by sha256, the configure scripts and Makefiles run arbitrary shell which can exfiltrate data or modify outputs unless executed in a hermetic/sandboxed environment. BUILD.bazel [1578-1651] Referred Code FLEX_SRC="$$(pwd)/$(execpath //third_party/pg_extension_toolchain/flex:source_tree)" if [[ -d "$${OUT_DIR}/flex_src" ]]; then chmod -R u+w "$${OUT_DIR}/flex_src" rm -rf "$${OUT_DIR}/flex_src" fi mkdir -p "$${OUT_DIR}/flex_src" cp -a "$${FLEX_SRC}/." "$${OUT_DIR}/flex_src" chmod -R u+w "$${OUT_DIR}/flex_src" touch "$${OUT_DIR}/flex_src/src/parse.c" "$${OUT_DIR}/flex_src/src/parse.h" "$${OUT_DIR}/flex_src/src/scan.c" "$${OUT_DIR}/flex_src/src/scan.h" cd "$${OUT_DIR}/flex_src" export ACLOCAL=true export AUTOCONF=true export AUTOHEADER=true export AUTOMAKE=true export MAKEINFO=true ./configure --prefix=/usr --disable-nls make -C src -j4 make -C src DESTDIR="$${OUT_DIR}/flex_install" install FLEX_ROOT="$${OUT_DIR}/flex_install/usr" GPERF_SRC="$${REPO_ROOT}/$(execpath //third_party/pg_extension_toolchain/gperf:source_tree)" ... (clipped 53 lines)
	Unsafe library injection Description: Blindly copying all shared libraries from the locally built flex tree into the install prefix (cp -a "$${FLEX_ROOT}/lib/." "$${INSTALL_PREFIX}/usr/lib/" \|\| true) may introduce unintended or conflicting libraries into runtime images, enabling DLL preloading/hijacking or ABI conflicts if downstream consumers pick up these libraries before system ones. BUILD.bazel [1660-1662] Referred Code `mkdir -p "$${INSTALL_PREFIX}/usr/lib" cp -a "$${FLEX_ROOT}/lib/." "$${INSTALL_PREFIX}/usr/lib/" \|\| true tar -C "$${INSTALL_PREFIX}" -cf "$${OUT_TAR}" .`
	Tool path spoofing Description: The CMake path handling treats any non-absolute path as trustworthy and later chmod +x and executes it; if Bazel action inputs could be influenced, this could allow path spoofing—ensure the resolved tool path comes only from the trusted repository and not from the working directory. BUILD.bazel [1506-1515] Referred Code `CMAKE_RELATIVE="$(location @cmake_linux_amd64_prebuilt//:cmake_bin)" if [[ "$${CMAKE_RELATIVE}" != /* ]]; then CMAKE_BIN="$$(pwd)/$${CMAKE_RELATIVE}" else CMAKE_BIN="$${CMAKE_RELATIVE}" fi # Don't use readlink -f - cmake needs the original path to find its modules chmod +x "$${CMAKE_BIN}" CMAKE_DIR="$$(dirname "$${CMAKE_BIN}")" mkdir -p "$${OUT_DIR}/bin"`
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No audit logs: The added build steps for fetching and building toolchain components (flex, bison, gperf) and packaging layers do not include any logging of critical actions, but as build infrastructure code this may not require audit trails. Referred Code mkdir -p "$${OUT_DIR}/age" cp -R "$${AGE_TREE}/." "$${OUT_DIR}/age" chmod -R u+w "$${OUT_DIR}/age" # Build flex from source so the toolchain matches the RBE glibc FLEX_SRC="$$(pwd)/$(execpath //third_party/pg_extension_toolchain/flex:source_tree)" if [[ -d "$${OUT_DIR}/flex_src" ]]; then chmod -R u+w "$${OUT_DIR}/flex_src" rm -rf "$${OUT_DIR}/flex_src" fi mkdir -p "$${OUT_DIR}/flex_src" cp -a "$${FLEX_SRC}/." "$${OUT_DIR}/flex_src" chmod -R u+w "$${OUT_DIR}/flex_src" touch "$${OUT_DIR}/flex_src/src/parse.c" "$${OUT_DIR}/flex_src/src/parse.h" "$${OUT_DIR}/flex_src/src/scan.c" "$${OUT_DIR}/flex_src/src/scan.h" cd "$${OUT_DIR}/flex_src" export ACLOCAL=true export AUTOCONF=true export AUTOHEADER=true export AUTOMAKE=true export MAKEINFO=true ./configure --prefix=/usr --disable-nls ... (clipped 69 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Partial error checks: The script uses set -euo pipefail but many build and copy operations (e.g., cp -a, make, configure) lack explicit error handling or validation of prereqs, which may be acceptable in Bazel genrule context. Referred Code set -euo pipefail REPO_ROOT="$$(pwd)" OUT_DIR="$$(pwd)/$(@D)" OUT_TAR="$$(pwd)/$@" ROOT_DIR="$${OUT_DIR}/rootfs_age" python3 "$(location //docker/images:extract_rootfs.py)" "$(location :cnpg_postgresql_16_6_rootfs_tar)" "$${ROOT_DIR}" python3 "$(location //docker/images:overlay_deb_packages.py)" "$${ROOT_DIR}" \ "$(location @postgresql_server_dev_16_deb//file)" \ "$(location @debian_libpq_dev_amd64_deb//file)" sed -i 's\|^CLANG = .\|CLANG = clang\|' "$${ROOT_DIR}/usr/lib/postgresql/16/lib/pgxs/src/Makefile.global" sed -i 's\|^with_llvm\t= .\|with_llvm\t= no\|' "$${ROOT_DIR}/usr/lib/postgresql/16/lib/pgxs/src/Makefile.global" AGE_TREE="$$(pwd)/$(execpath //age:source_tree)" if [[ -d "$${OUT_DIR}/age" ]]; then chmod -R u+w "$${OUT_DIR}/age" rm -rf "$${OUT_DIR}/age" fi mkdir -p "$${OUT_DIR}/age" cp -R "$${AGE_TREE}/." "$${OUT_DIR}/age" chmod -R u+w "$${OUT_DIR}/age" ... (clipped 87 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: External sources: The script builds external sources and adjusts environment paths without explicit verification of inputs beyond Bazel hashes, which may be sufficient under hermetic builds but not evident from the diff. Referred Code genrule( name = "age_extension_layer", srcs = [ ":cnpg_postgresql_16_6_rootfs_tar", "@postgresql_server_dev_16_deb//file", "@debian_libpq_dev_amd64_deb//file", "//third_party/pg_extension_toolchain/bison:source_tree", "//third_party/pg_extension_toolchain/gperf:source_tree", "//third_party/pg_extension_toolchain/flex:source_tree", "//age:source_tree", "//docker/images:pg_config_wrapper.sh", ], outs = ["age_extension_layer.tar"], tools = [ "//docker/images:extract_rootfs.py", "//docker/images:overlay_deb_packages.py", "//docker/images:pg_config_rewrite.py", ], cmd = """ set -euo pipefail REPO_ROOT="$$(pwd)" ... (clipped 96 lines) Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1979#issuecomment-3567667895 Original created: 2025-11-23T09:22:04Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR: <table><tbody><tr><td colspan='2'>Security Compliance</td></tr> <tr><td rowspan=3>⚪</td> <td><details><summary>Untrusted build execution </summary> Description: Building and executing third-party autotools configure/make steps at build time without verification or sandboxing (flex, gperf, bison) risks executing untrusted build scripts on the build host; although archives are pinned by sha256, the configure scripts and Makefiles run arbitrary shell which can exfiltrate data or modify outputs unless executed in a hermetic/sandboxed environment. <a href='https://github.com/carverauto/serviceradar/pull/1979/files#diff-0e4db31c224a8f72ae8e870a849e38a59d74a2c7f7b04347b0b3eb07e20c5a80R1578-R1651'>BUILD.bazel [1578-1651]</a> <details open><summary>Referred Code</summary> ```txt FLEX_SRC="$$(pwd)/$(execpath //third_party/pg_extension_toolchain/flex:source_tree)" if [[ -d "$${OUT_DIR}/flex_src" ]]; then chmod -R u+w "$${OUT_DIR}/flex_src" rm -rf "$${OUT_DIR}/flex_src" fi mkdir -p "$${OUT_DIR}/flex_src" cp -a "$${FLEX_SRC}/." "$${OUT_DIR}/flex_src" chmod -R u+w "$${OUT_DIR}/flex_src" touch "$${OUT_DIR}/flex_src/src/parse.c" "$${OUT_DIR}/flex_src/src/parse.h" "$${OUT_DIR}/flex_src/src/scan.c" "$${OUT_DIR}/flex_src/src/scan.h" cd "$${OUT_DIR}/flex_src" export ACLOCAL=true export AUTOCONF=true export AUTOHEADER=true export AUTOMAKE=true export MAKEINFO=true ./configure --prefix=/usr --disable-nls make -C src -j4 make -C src DESTDIR="$${OUT_DIR}/flex_install" install FLEX_ROOT="$${OUT_DIR}/flex_install/usr" GPERF_SRC="$${REPO_ROOT}/$(execpath //third_party/pg_extension_toolchain/gperf:source_tree)" ... (clipped 53 lines) ``` </details></details></td></tr> <tr><td><details><summary>Unsafe library injection </summary> Description: Blindly copying all shared libraries from the locally built flex tree into the install prefix (cp -a "$${FLEX_ROOT}/lib/." "$${INSTALL_PREFIX}/usr/lib/" || true) may introduce unintended or conflicting libraries into runtime images, enabling DLL preloading/hijacking or ABI conflicts if downstream consumers pick up these libraries before system ones. <a href='https://github.com/carverauto/serviceradar/pull/1979/files#diff-0e4db31c224a8f72ae8e870a849e38a59d74a2c7f7b04347b0b3eb07e20c5a80R1660-R1662'>BUILD.bazel [1660-1662]</a> <details open><summary>Referred Code</summary> ```txt mkdir -p "$${INSTALL_PREFIX}/usr/lib" cp -a "$${FLEX_ROOT}/lib/." "$${INSTALL_PREFIX}/usr/lib/" || true tar -C "$${INSTALL_PREFIX}" -cf "$${OUT_TAR}" . ``` </details></details></td></tr> <tr><td><details><summary>Tool path spoofing </summary> Description: The CMake path handling treats any non-absolute path as trustworthy and later chmod +x and executes it; if Bazel action inputs could be influenced, this could allow path spoofing—ensure the resolved tool path comes only from the trusted repository and not from the working directory. <a href='https://github.com/carverauto/serviceradar/pull/1979/files#diff-0e4db31c224a8f72ae8e870a849e38a59d74a2c7f7b04347b0b3eb07e20c5a80R1506-R1515'>BUILD.bazel [1506-1515]</a> <details open><summary>Referred Code</summary> ```txt CMAKE_RELATIVE="$(location @cmake_linux_amd64_prebuilt//:cmake_bin)" if [[ "$${CMAKE_RELATIVE}" != /* ]]; then CMAKE_BIN="$$(pwd)/$${CMAKE_RELATIVE}" else CMAKE_BIN="$${CMAKE_RELATIVE}" fi # Don't use readlink -f - cmake needs the original path to find its modules chmod +x "$${CMAKE_BIN}" CMAKE_DIR="$$(dirname "$${CMAKE_BIN}")" mkdir -p "$${OUT_DIR}/bin" ``` </details></details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>⚪</td><td><details><summary>🎫 No ticket provided </summary> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'>Codebase Duplication Compliance</td></tr> <tr><td>⚪</td><td><details><summary>Codebase context is not defined </summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'>Custom Compliance</td></tr> <tr><td rowspan=3>🟢</td><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Logging Practices</summary> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=3>⚪</td> <td><details> <summary>Generic: Comprehensive Audit Trails</summary> **Objective:** To create a detailed and reliable record of critical system actions for security analysis and compliance. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1979/files#diff-0e4db31c224a8f72ae8e870a849e38a59d74a2c7f7b04347b0b3eb07e20c5a80R1573-R1662'>No audit logs</a>: The added build steps for fetching and building toolchain components (flex, bison, gperf) and packaging layers do not include any logging of critical actions, but as build infrastructure code this may not require audit trails. <details open><summary>Referred Code</summary> ```txt mkdir -p "$${OUT_DIR}/age" cp -R "$${AGE_TREE}/." "$${OUT_DIR}/age" chmod -R u+w "$${OUT_DIR}/age" # Build flex from source so the toolchain matches the RBE glibc FLEX_SRC="$$(pwd)/$(execpath //third_party/pg_extension_toolchain/flex:source_tree)" if [[ -d "$${OUT_DIR}/flex_src" ]]; then chmod -R u+w "$${OUT_DIR}/flex_src" rm -rf "$${OUT_DIR}/flex_src" fi mkdir -p "$${OUT_DIR}/flex_src" cp -a "$${FLEX_SRC}/." "$${OUT_DIR}/flex_src" chmod -R u+w "$${OUT_DIR}/flex_src" touch "$${OUT_DIR}/flex_src/src/parse.c" "$${OUT_DIR}/flex_src/src/parse.h" "$${OUT_DIR}/flex_src/src/scan.c" "$${OUT_DIR}/flex_src/src/scan.h" cd "$${OUT_DIR}/flex_src" export ACLOCAL=true export AUTOCONF=true export AUTOHEADER=true export AUTOMAKE=true export MAKEINFO=true ./configure --prefix=/usr --disable-nls ... (clipped 69 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1979/files#diff-0e4db31c224a8f72ae8e870a849e38a59d74a2c7f7b04347b0b3eb07e20c5a80R1556-R1663'>Partial error checks</a>: The script uses set -euo pipefail but many build and copy operations (e.g., cp -a, make, configure) lack explicit error handling or validation of prereqs, which may be acceptable in Bazel genrule context. <details open><summary>Referred Code</summary> ```txt set -euo pipefail REPO_ROOT="$$(pwd)" OUT_DIR="$$(pwd)/$(@D)" OUT_TAR="$$(pwd)/$@" ROOT_DIR="$${OUT_DIR}/rootfs_age" python3 "$(location //docker/images:extract_rootfs.py)" "$(location :cnpg_postgresql_16_6_rootfs_tar)" "$${ROOT_DIR}" python3 "$(location //docker/images:overlay_deb_packages.py)" "$${ROOT_DIR}" \ "$(location @postgresql_server_dev_16_deb//file)" \ "$(location @debian_libpq_dev_amd64_deb//file)" sed -i 's|^CLANG = .*|CLANG = clang|' "$${ROOT_DIR}/usr/lib/postgresql/16/lib/pgxs/src/Makefile.global" sed -i 's|^with_llvm\t= .*|with_llvm\t= no|' "$${ROOT_DIR}/usr/lib/postgresql/16/lib/pgxs/src/Makefile.global" AGE_TREE="$$(pwd)/$(execpath //age:source_tree)" if [[ -d "$${OUT_DIR}/age" ]]; then chmod -R u+w "$${OUT_DIR}/age" rm -rf "$${OUT_DIR}/age" fi mkdir -p "$${OUT_DIR}/age" cp -R "$${AGE_TREE}/." "$${OUT_DIR}/age" chmod -R u+w "$${OUT_DIR}/age" ... (clipped 87 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1979/files#diff-0e4db31c224a8f72ae8e870a849e38a59d74a2c7f7b04347b0b3eb07e20c5a80R1537-R1653'>External sources</a>: The script builds external sources and adjusts environment paths without explicit verification of inputs beyond Bazel hashes, which may be sufficient under hermetic builds but not evident from the diff. <details open><summary>Referred Code</summary> ```txt genrule( name = "age_extension_layer", srcs = [ ":cnpg_postgresql_16_6_rootfs_tar", "@postgresql_server_dev_16_deb//file", "@debian_libpq_dev_amd64_deb//file", "//third_party/pg_extension_toolchain/bison:source_tree", "//third_party/pg_extension_toolchain/gperf:source_tree", "//third_party/pg_extension_toolchain/flex:source_tree", "//age:source_tree", "//docker/images:pg_config_wrapper.sh", ], outs = ["age_extension_layer.tar"], tools = [ "//docker/images:extract_rootfs.py", "//docker/images:overlay_deb_packages.py", "//docker/images:pg_config_rewrite.py", ], cmd = """ set -euo pipefail REPO_ROOT="$$(pwd)" ... (clipped 96 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2">   </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant 🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label </details>

qodo-code-review[bot] commented

2025-11-23 09:23:04 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1979#issuecomment-3567671653
Original created: 2025-11-23T09:23:04Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Use Bazel rules for external dependencies Instead of building `flex`, `bison`, and `gperf` inside a `genrule` shell script, use a dedicated Bazel ruleset like `rules_foreign_cc`. This will create proper Bazel targets for these dependencies, improving modularity and enabling better caching. Examples: docker/images/BUILD.bazel [1537-1664] `genrule( name = "age_extension_layer", srcs = [ ":cnpg_postgresql_16_6_rootfs_tar", "@postgresql_server_dev_16_deb//file", "@debian_libpq_dev_amd64_deb//file", "//third_party/pg_extension_toolchain/bison:source_tree", "//third_party/pg_extension_toolchain/gperf:source_tree", "//third_party/pg_extension_toolchain/flex:source_tree", "//age:source_tree", ... (clipped 118 lines)` Solution Walkthrough: Before: # docker/images/BUILD.bazel genrule( name = "age_extension_layer", srcs = [ "//third_party/.../bison:source_tree", "//third_party/.../gperf:source_tree", "//third_party/.../flex:source_tree", ... ], cmd = """ # Build flex from source cd .../flex_src ./configure ... && make && make install # Build gperf from source cd .../gperf_src ./configure ... && make && make install # Build bison from source cd .../bison_src ./configure ... && make && make install # Build age extension using the new tools export PATH=... cd .../age make ... """, ... ) After: # third_party/pg_extension_toolchain/flex/BUILD.bazel (conceptual) load("@rules_foreign_cc//foreign_cc:defs.bzl", "configure_make") configure_make( name = "flex", lib_source = "@flex_src//:all_srcs", # ... configure options ) # (Similar rules for bison and gperf) # docker/images/BUILD.bazel genrule( name = "age_extension_layer", srcs = ["//age:source_tree", ...], tools = [ "//third_party/pg_extension_toolchain/flex", "//third_party/pg_extension_toolchain/bison", "//third_party/pg_extension_toolchain/gperf", ], cmd = """ # PATH is set by Bazel's `tools` attribute cd $(execpath //age:source_tree) make ... """, ... ) Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that building three dependencies within a single `genrule` script is a significant Bazel anti-pattern, proposing a more robust, maintainable, and idiomatic solution that improves caching and modularity.	High
Possible issue	Avoid suppressing errors during copy Remove the `\|\| true` error suppression from the `cp` command. Instead, explicitly check if the source directory (`$${FLEX_ROOT}/lib`) exists and is not empty before copying, and fail the build with an error message if it is not. docker/images/BUILD.bazel [1660-1661] `-+mkdir -p "$${INSTALL_PREFIX}/usr/lib" -+cp -a "$${FLEX_ROOT}/lib/." "$${INSTALL_PREFIX}/usr/lib/" \|\| true ++if [ -d "$${FLEX_ROOT}/lib" ] && [ -n "$(ls -A "$${FLEX_ROOT}/lib")" ]; then ++ mkdir -p "$${INSTALL_PREFIX}/usr/lib" ++ cp -a "$${FLEX_ROOT}/lib/." "$${INSTALL_PREFIX}/usr/lib/" ++else ++ echo "Error: Flex library directory '$${FLEX_ROOT}/lib' is empty or does not exist." >&2 ++ exit 1 ++fi` Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that using `\|\| true` hides potential critical errors, such as missing libraries, which could lead to a broken build artifact. The proposed change adds explicit checks and error handling, making the build process more robust and reliable.	Medium
General	Simplify directory cleanup and creation Simplify directory cleanup by replacing the conditional check and removal with an unconditional `rm -rf`, followed by `mkdir -p`. This makes the script more concise. docker/images/BUILD.bazel [1578-1585] `+FLEX_SRC="$$(pwd)/$(execpath //third_party/pg_extension_toolchain/flex:source_tree)" -+if [[ -d "$${OUT_DIR}/flex_src" ]]; then -+ chmod -R u+w "$${OUT_DIR}/flex_src" -+ rm -rf "$${OUT_DIR}/flex_src" -+fi ++rm -rf "$${OUT_DIR}/flex_src" +mkdir -p "$${OUT_DIR}/flex_src" +cp -a "$${FLEX_SRC}/." "$${OUT_DIR}/flex_src" +chmod -R u+w "$${OUT_DIR}/flex_src"` Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies a shell script pattern that can be simplified, improving code conciseness and readability. The `if` check is redundant with `rm -rf`.	Low
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1979#issuecomment-3567671653 Original created: 2025-11-23T09:23:04Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td>Category</td><td align=left>Suggestion                                                                                                                                    </td><td align=center>Impact</td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>Use Bazel rules for external dependencies</summary> ___ **Instead of building <code>flex</code>, <code>bison</code>, and <code>gperf</code> inside a <code>genrule</code> shell script, use a dedicated Bazel ruleset like <code>rules_foreign_cc</code>. This will create proper Bazel targets for these dependencies, improving modularity and enabling better caching.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1979/files#diff-0e4db31c224a8f72ae8e870a849e38a59d74a2c7f7b04347b0b3eb07e20c5a80R1537-R1664">docker/images/BUILD.bazel [1537-1664]</a> </summary> ```starlark genrule( name = "age_extension_layer", srcs = [ ":cnpg_postgresql_16_6_rootfs_tar", "@postgresql_server_dev_16_deb//file", "@debian_libpq_dev_amd64_deb//file", "//third_party/pg_extension_toolchain/bison:source_tree", "//third_party/pg_extension_toolchain/gperf:source_tree", "//third_party/pg_extension_toolchain/flex:source_tree", "//age:source_tree", ... (clipped 118 lines) ``` </details> ### Solution Walkthrough: #### Before: ```starlark # docker/images/BUILD.bazel genrule( name = "age_extension_layer", srcs = [ "//third_party/.../bison:source_tree", "//third_party/.../gperf:source_tree", "//third_party/.../flex:source_tree", ... ], cmd = """ # Build flex from source cd .../flex_src ./configure ... && make && make install # Build gperf from source cd .../gperf_src ./configure ... && make && make install # Build bison from source cd .../bison_src ./configure ... && make && make install # Build age extension using the new tools export PATH=... cd .../age make ... """, ... ) ``` #### After: ```starlark # third_party/pg_extension_toolchain/flex/BUILD.bazel (conceptual) load("@rules_foreign_cc//foreign_cc:defs.bzl", "configure_make") configure_make( name = "flex", lib_source = "@flex_src//:all_srcs", # ... configure options ) # (Similar rules for bison and gperf) # docker/images/BUILD.bazel genrule( name = "age_extension_layer", srcs = ["//age:source_tree", ...], tools = [ "//third_party/pg_extension_toolchain/flex", "//third_party/pg_extension_toolchain/bison", "//third_party/pg_extension_toolchain/gperf", ], cmd = """ # PATH is set by Bazel's `tools` attribute cd $(execpath //age:source_tree) make ... """, ... ) ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies that building three dependencies within a single `genrule` script is a significant Bazel anti-pattern, proposing a more robust, maintainable, and idiomatic solution that improves caching and modularity. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Avoid suppressing errors during copy</summary> ___ **Remove the <code>|| true</code> error suppression from the <code>cp</code> command. Instead, explicitly check if the source directory (<code>$${FLEX_ROOT}/lib</code>) exists and is not empty before copying, and fail the build with an error message if it is not.** [docker/images/BUILD.bazel [1660-1661]](https://github.com/carverauto/serviceradar/pull/1979/files#diff-0e4db31c224a8f72ae8e870a849e38a59d74a2c7f7b04347b0b3eb07e20c5a80R1660-R1661) ```diff -+mkdir -p "$${INSTALL_PREFIX}/usr/lib" -+cp -a "$${FLEX_ROOT}/lib/." "$${INSTALL_PREFIX}/usr/lib/" || true ++if [ -d "$${FLEX_ROOT}/lib" ] && [ -n "$(ls -A "$${FLEX_ROOT}/lib")" ]; then ++ mkdir -p "$${INSTALL_PREFIX}/usr/lib" ++ cp -a "$${FLEX_ROOT}/lib/." "$${INSTALL_PREFIX}/usr/lib/" ++else ++ echo "Error: Flex library directory '$${FLEX_ROOT}/lib' is empty or does not exist." >&2 ++ exit 1 ++fi ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies that using `|| true` hides potential critical errors, such as missing libraries, which could lead to a broken build artifact. The proposed change adds explicit checks and error handling, making the build process more robust and reliable. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Simplify directory cleanup and creation</summary> ___ **Simplify directory cleanup by replacing the conditional check and removal with an unconditional <code>rm -rf</code>, followed by <code>mkdir -p</code>. This makes the script more concise.** [docker/images/BUILD.bazel [1578-1585]](https://github.com/carverauto/serviceradar/pull/1979/files#diff-0e4db31c224a8f72ae8e870a849e38a59d74a2c7f7b04347b0b3eb07e20c5a80R1578-R1585) ```diff +FLEX_SRC="$$(pwd)/$(execpath //third_party/pg_extension_toolchain/flex:source_tree)" -+if [[ -d "$${OUT_DIR}/flex_src" ]]; then -+ chmod -R u+w "$${OUT_DIR}/flex_src" -+ rm -rf "$${OUT_DIR}/flex_src" -+fi ++rm -rf "$${OUT_DIR}/flex_src" +mkdir -p "$${OUT_DIR}/flex_src" +cp -a "$${FLEX_SRC}/." "$${OUT_DIR}/flex_src" +chmod -R u+w "$${OUT_DIR}/flex_src" ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: The suggestion correctly identifies a shell script pattern that can be simplified, improving code conciseness and readability. The `if` check is redundant with `rm -rf`. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>