-
Notifications
You must be signed in to change notification settings - Fork 129
feat: make the public headers consumable as C++20 #936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c01f40f
4e9b719
16520aa
5ee79e6
7b30963
152ebc4
2551261
869795d
c3fe6f5
8a7fc01
4f9675f
1f1da6a
d005280
de7b278
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,13 +20,79 @@ cmake_minimum_required(VERSION 3.25) | |
|
|
||
| project(example) | ||
|
|
||
| set(CMAKE_CXX_STANDARD 23) | ||
| # C++20 is the minimum standard iceberg-cpp's public headers support, so the | ||
| # example builds as C++20 by default to keep that contract exercised. Set this to | ||
| # 23 to also check the headers from a C++23 consumer. | ||
| set(ICEBERG_EXAMPLE_CXX_STANDARD | ||
| 20 | ||
| CACHE STRING "C++ standard used to build the example (20 or 23)") | ||
| set_property(CACHE ICEBERG_EXAMPLE_CXX_STANDARD PROPERTY STRINGS 20 23) | ||
| if(NOT ICEBERG_EXAMPLE_CXX_STANDARD MATCHES "^(20|23)$") | ||
| message(FATAL_ERROR "ICEBERG_EXAMPLE_CXX_STANDARD must be 20 or 23, got " | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need to specify 20 or 23 here? We need to update this file as well when we support C++26. |
||
| "'${ICEBERG_EXAMPLE_CXX_STANDARD}'") | ||
| endif() | ||
|
|
||
| set(CMAKE_CXX_STANDARD ${ICEBERG_EXAMPLE_CXX_STANDARD}) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
| set(CMAKE_CXX_EXTENSIONS OFF) | ||
|
|
||
| find_package(iceberg CONFIG REQUIRED COMPONENTS bundle rest) | ||
|
|
||
| if(TARGET iceberg::iceberg_bundle_shared) | ||
| set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_shared) | ||
| else() | ||
| set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_static) | ||
| endif() | ||
|
|
||
| if(TARGET iceberg::iceberg_rest_shared) | ||
| set(ICEBERG_REST_TARGET iceberg::iceberg_rest_shared) | ||
| else() | ||
| set(ICEBERG_REST_TARGET iceberg::iceberg_rest_static) | ||
| endif() | ||
|
|
||
| add_executable(demo_example demo_example.cc) | ||
|
|
||
| target_link_libraries(demo_example | ||
| PRIVATE "$<IF:$<TARGET_EXISTS:iceberg::iceberg_bundle_shared>,iceberg::iceberg_bundle_shared,iceberg::iceberg_bundle_static>" | ||
| "$<IF:$<TARGET_EXISTS:iceberg::iceberg_rest_shared>,iceberg::iceberg_rest_shared,iceberg::iceberg_rest_static>" | ||
| ) | ||
| target_link_libraries(demo_example PRIVATE ${ICEBERG_BUNDLE_TARGET} | ||
| ${ICEBERG_REST_TARGET}) | ||
|
|
||
| # Compile every installed public header as a consumer using | ||
| # ICEBERG_EXAMPLE_CXX_STANDARD. The installed include | ||
| # tree is the public API contract: iceberg_install_all_headers excludes internal | ||
| # headers before packaging them. | ||
| get_target_property(ICEBERG_BUNDLE_INCLUDE_DIRS ${ICEBERG_BUNDLE_TARGET} | ||
| INTERFACE_INCLUDE_DIRECTORIES) | ||
| foreach(ICEBERG_INCLUDE_DIR IN LISTS ICEBERG_BUNDLE_INCLUDE_DIRS) | ||
| if(EXISTS "${ICEBERG_INCLUDE_DIR}/iceberg") | ||
| set(ICEBERG_PUBLIC_INCLUDE_DIR "${ICEBERG_INCLUDE_DIR}") | ||
| break() | ||
| endif() | ||
| endforeach() | ||
|
|
||
| if(NOT ICEBERG_PUBLIC_INCLUDE_DIR) | ||
| message(FATAL_ERROR "Could not locate iceberg's installed public headers") | ||
| endif() | ||
|
|
||
| file(GLOB_RECURSE | ||
| ICEBERG_PUBLIC_HEADERS | ||
| CONFIGURE_DEPENDS | ||
| "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.h" | ||
| "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.hpp") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we don't have |
||
| list(SORT ICEBERG_PUBLIC_HEADERS) | ||
|
|
||
| set(ICEBERG_PUBLIC_HEADER_CHECK_SOURCE | ||
| "// Generated from iceberg's installed public headers.\n") | ||
| foreach(ICEBERG_PUBLIC_HEADER IN LISTS ICEBERG_PUBLIC_HEADERS) | ||
| file(RELATIVE_PATH ICEBERG_PUBLIC_HEADER_RELATIVE_PATH "${ICEBERG_PUBLIC_INCLUDE_DIR}" | ||
| "${ICEBERG_PUBLIC_HEADER}") | ||
| string(APPEND ICEBERG_PUBLIC_HEADER_CHECK_SOURCE | ||
| "#include <${ICEBERG_PUBLIC_HEADER_RELATIVE_PATH}>\n") | ||
| endforeach() | ||
| string(APPEND ICEBERG_PUBLIC_HEADER_CHECK_SOURCE "\nint main() { return 0; }\n") | ||
|
|
||
| file(GENERATE | ||
| OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/public_headers_check.cc" | ||
| CONTENT "${ICEBERG_PUBLIC_HEADER_CHECK_SOURCE}") | ||
|
|
||
| add_executable(public_headers_check "${CMAKE_CURRENT_BINARY_DIR}/public_headers_check.cc") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, a better alternative is to add a dedicated test executable built with C++20. It takes extra steps to install iceberg libraries and then build the example. We can add a non-installed header file (e.g. src/iceberg/cpp20_compatibility_internal.h) to include all public headers and then use it in the test case. The challenge is to make this header file in sync when we add new header files. We can update AGENTS.md to add this as an advice. |
||
| target_link_libraries(public_headers_check PRIVATE ${ICEBERG_BUNDLE_TARGET} | ||
| ${ICEBERG_REST_TARGET}) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,10 +23,14 @@ | |
|
|
||
| **Required:** | ||
|
|
||
| - C++23 compliant compiler (GCC 14+, Clang 18+, MSVC 2022+) | ||
| - C++23 compliant compiler (GCC 14+, Clang 18+, MSVC 2022+) to build iceberg-cpp itself | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems better to keep here unchanged to indicate that we officially support C++23. Then we can add a dedicated section below for the contract of C++20 compatibility. |
||
| - CMake 3.25+ | ||
| - [Ninja](https://ninja-build.org/) (recommended build backend) | ||
|
|
||
| **Using iceberg-cpp from your project:** the installed public headers require | ||
| C++20 at minimum, so applications that link against iceberg-cpp can be | ||
| compiled as C++20 or later. The library itself is still built as C++23. | ||
|
|
||
| ## Quick Start | ||
|
|
||
| ```bash | ||
|
|
@@ -112,6 +116,14 @@ If using provided Apache Arrow, include both paths: | |
| cmake -S . -B build -G Ninja -DCMAKE_PREFIX_PATH="/path/to/install;/path/to/arrow" | ||
| ``` | ||
|
|
||
| The examples build as C++20 by default, which is the minimum standard supported | ||
| by the public headers. Set `ICEBERG_EXAMPLE_CXX_STANDARD` to `23` to build them | ||
| as C++23 instead: | ||
|
|
||
| ```bash | ||
| cmake -S . -B build -G Ninja -DCMAKE_PREFIX_PATH=/path/to/install -DICEBERG_EXAMPLE_CXX_STANDARD=23 | ||
| ``` | ||
|
|
||
| ## Customizing Dependency URLs | ||
|
|
||
| If you experience network issues when downloading dependencies, you can override the download URLs using environment variables: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make C++23 still as default and let it accept user supplied option so that C++20 can be test manually locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhjwpku Thank you for the comments.
Making it configurable is a good idea. Have different thoughts on making C++23 as default though.
With this patch, it changes the minimum supported C++ standard from C++23 to C++20 for downstream consumers. And C++20 is the interface contract between the consumer and iceberg-cpp library, and the contract should be tested continuously.
Setting the default to C++20 ensures the minimum supported standard (contract) is continuously exercised. Defaulting it to C++23 would let C++20 only breakages slip through.
One refinement is that C++23 compatibility should still be tested separately. C++23 should accepts C++20 code, but we can enhance this by provide an optional example configuration for C++23, for example, expose an
ICEBERG_EXAMPLE_CXX_STANDARDcache setting that defaults to 20 and accepts 23; then update CI to build both.And also refine the document to state clearly that the minimum C++ standard is C++20 for public headers. What do you think?
Happy to make changes either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense to me, I think we should build both for compatibility purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhjwpku Exposed
ICEBERG_EXAMPLE_CXX_STANDARDand updated document accordingly in de7b278.