Skip to content

Navigation Menu

Sign in
Sign up

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

Open
Sakshamm-Goyal wants to merge 2 commits into
apache:main from
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:main from
Sakshamm-Goyal:fix/build-definition-parity

Conversation

@Sakshamm-Goyal

@Sakshamm-Goyal Sakshamm-Goyal commented Aug 23, 2026

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.

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.

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")

@zhjwpku zhjwpku Sep 5, 2026

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")

@zhjwpku zhjwpku Sep 5, 2026

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):

@zhjwpku zhjwpku Sep 5, 2026

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:

@zhjwpku zhjwpku Sep 5, 2026

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

Reviewers

1 more reviewer
@zhjwpku zhjwpku zhjwpku left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CMake and Meson build definitions are Out Of Sync

AltStyle によって変換されたページ (->オリジナル) /