Skip to content

build: keep CMake and Meson definitions in sync - #896

Open
Sakshamm-Goyal wants to merge 2 commits into
apache:mainfrom
Sakshamm-Goyal:fix/build-definition-parity
Open

build: keep CMake and Meson definitions in sync#896
Sakshamm-Goyal wants to merge 2 commits into
apache:mainfrom
Sakshamm-Goyal:fix/build-definition-parity

Conversation

@Sakshamm-Goyal

Copy link
Copy Markdown

Summary

  • install the missing public headers in Meson and the top-level catalog headers in CMake
  • include snapshot_summary_builder_test.cc in Meson's table_test
  • align the CRoaring, nlohmann/json, and GoogleTest fallback versions across CMake and Meson
  • add a dependency-free parity regression and run it in CI

Closes #894.

Validation

  • python3 dev/test_build_definition_parity.py
  • pre-commit run --files .github/workflows/test.yml cmake_modules/IcebergThirdpartyToolchain.cmake src/iceberg/catalog/CMakeLists.txt src/iceberg/catalog/rest/auth/meson.build src/iceberg/meson.build src/iceberg/test/meson.build dev/test_build_definition_parity.py
  • CMake configure with bundle/REST disabled, followed by cmake --build build-cmake-parity --target table_test --parallel 2
  • ctest --test-dir build-cmake-parity -R '^table_test$' --output-on-failure
  • Meson configure with REST/tests enabled, followed by meson compile -C build-meson-parity table_test -j 2
  • meson test -C build-meson-parity table_test --print-errorlogs
  • inspected both generated install manifests for the corrected public headers

AI assistance disclosure

AI-assisted tooling was used for initial regression-test scaffolding and mechanical build-definition edits. I reviewed the change end-to-end, verified the dependency choices and generated manifests, and ran the validations listed above. I am not aware of remaining uncertainty in the affected build paths.

@Sakshamm-Goyal

Copy link
Copy Markdown
Author

The failing Ubuntu Debug and macOS Build Example steps were caused by CRoaring 4.3.11 exposing roaring-headers and roaring-headers-cpp in the vendored target install interface. Iceberg exports roaring without exporting those build-only helper targets, so downstream find_package(iceberg) marked the package not found.

I pushed 09fabab to keep both helper targets behind BUILD_INTERFACE and added parity coverage for the export invariant. Validation completed locally:

  • build-definition parity: 4/4 passed
  • repository pre-commit on the changed files: passed
  • real Debug CMake build + install: passed
  • installed targets contain no roaring-headers references
  • downstream example/ configure and build against the installed package: passed

Replacement CI is now running on the updated head.

@manuzhang

Copy link
Copy Markdown
Member

@Sakshamm-Goyal Thanks for the PR! I opened #894 to track current status and initiated a discussion on how we can maintain and sync the build tools in the long term. Before taking actions, we'd better gather feedbacks from the community, especially from meson maintainers like @WillAyd. Meanwhile, there are prerequisite issues like #256 we need to resolve first.

else()
set(CROARING_URL
"https://github.com/RoaringBitmap/CRoaring/archive/refs/tags/v4.4.3.tar.gz")
"https://github.com/RoaringBitmap/CRoaring/archive/refs/tags/v4.3.11.tar.gz")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure downgrade the third-party version is the right way to go :-(

else()
set(NLOHMANN_JSON_URL
"https://github.com/nlohmann/json/releases/download/v3.11.3/json.tar.xz")
"https://github.com/nlohmann/json/releases/download/v3.12.0/json.tar.xz")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest upgrade these versions in a separate PR.

return match.group(1)


class BuildDefinitionParityTest(unittest.TestCase):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this check cover all the relevant files? ISTM we could simply add a section to AGENTS.md instructing AI coding tools to keep the CMake and Meson build files in sync. Wouldn’t that be enough?

read("src/iceberg/catalog/CMakeLists.txt"),
)

def test_core_tests_are_run_by_both_build_systems(self) -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right to me. We shouldn't run a test for a previously missing file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CMake and Meson build definitions are Out Of Sync

3 participants