Skip to content

refactor: remove -isystem on local include header files #211

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 15, 2025
Merged

refactor: remove -isystem on local include header files #211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de908ff
refactor: remove -isystem on local include header files
HuaHuaY Sep 4, 2025
988d8b1
fix compile errors when using gcc
HuaHuaY Sep 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmake_modules/IcebergBuildUtils.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ function(add_iceberg_lib LIB_NAME)
endif()

if(LIB_INCLUDES)
target_include_directories(${LIB_NAME}_shared SYSTEM PUBLIC ${ARG_EXTRA_INCLUDES})
Copy link
Member

@wgtmac wgtmac Sep 7, 2025

Choose a reason for hiding this comment

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

@raulcd I think I just copied the same thing from Apache Arrow. Do you have any idea why it was added there?

Copy link
Contributor Author

@HuaHuaY HuaHuaY Sep 8, 2025

Choose a reason for hiding this comment

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

I read arrow's cmake files. It doesn't use the parameter EXTRA_INCLUDES of add_arrow_lib. So maybe we misunderstand the meaning of EXTRA_INCLUDES and we should modify the caller of add_iceberg_lib instead of here.

Copy link
Member

@wgtmac wgtmac Sep 8, 2025

Choose a reason for hiding this comment

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

Adding these header files as build time dependencies makes it easier for libiceberg-bundle and unit test executables to find them. But I also think we don't need to add SYSTEM to them in this case.

Copy link
Member

@raulcd raulcd Sep 8, 2025

Choose a reason for hiding this comment

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

This was added ~7 years ago here:
apache/arrow@5c97cd6

I also think SYSTEM is not required, we could try removing it on ARROW too and validate CI, Python bindings builds, etcetera.

target_include_directories(${LIB_NAME}_shared PUBLIC ${ARG_EXTRA_INCLUDES})
endif()

if(ARG_PRIVATE_INCLUDES)
Expand Down Expand Up @@ -179,7 +179,7 @@ function(add_iceberg_lib LIB_NAME)
endif()

if(LIB_INCLUDES)
target_include_directories(${LIB_NAME}_static SYSTEM PUBLIC ${ARG_EXTRA_INCLUDES})
target_include_directories(${LIB_NAME}_static PUBLIC ${ARG_EXTRA_INCLUDES})
endif()

if(ARG_PRIVATE_INCLUDES)
Expand Down
18 changes: 9 additions & 9 deletions src/iceberg/table_metadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,11 @@ struct ICEBERG_EXPORT TableMetadata {
/// The highest assigned column ID for the table
int32_t last_column_id;
/// A list of schemas
std::vector<std::shared_ptr<Schema>> schemas;
Copy link
Collaborator

@zhjwpku zhjwpku Sep 4, 2025

Choose a reason for hiding this comment

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

I don't think this is necessary since we are inside a iceberg namespace here, same for the below.

Copy link
Contributor Author

@HuaHuaY HuaHuaY Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If we don't add iceberg::, gcc will report a compile error due to -Werror and -Wchanges-meaning (included in -Wall).

Copy link
Contributor Author

@HuaHuaY HuaHuaY Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/apache/iceberg-cpp/actions/runs/17463347891/job/49593031029?pr=211

In file included from /home/runner/work/iceberg-cpp/iceberg-cpp/src/iceberg/catalog/in_memory_catalog.cc:27:
/home/runner/work/iceberg-cpp/iceberg-cpp/src/iceberg/table_metadata.h:125:35: error: declaration of ‘iceberg::Result<std::shared_ptr<iceberg::Schema> > iceberg::TableMetadata::Schema() const’ changes meaning of ‘Schema’ [-Wchanges-meaning]
  125 |   Result<std::shared_ptr<Schema>> Schema() const;
      |                                   ^~~~~~

std::vector<std::shared_ptr<iceberg::Schema>> schemas;
/// ID of the table's current schema
std::optional<int32_t> current_schema_id;
/// A list of partition specs
std::vector<std::shared_ptr<PartitionSpec>> partition_specs;
std::vector<std::shared_ptr<iceberg::PartitionSpec>> partition_specs;
/// ID of the current partition spec that writers should use by default
int32_t default_spec_id;
/// The highest assigned partition field ID across all partition specs for the table
Expand All @@ -101,15 +101,15 @@ struct ICEBERG_EXPORT TableMetadata {
/// ID of the current table snapshot
int64_t current_snapshot_id;
/// A list of valid snapshots
std::vector<std::shared_ptr<Snapshot>> snapshots;
std::vector<std::shared_ptr<iceberg::Snapshot>> snapshots;
/// A list of timestamp and snapshot ID pairs that encodes changes to the current
/// snapshot for the table
std::vector<SnapshotLogEntry> snapshot_log;
/// A list of timestamp and metadata file location pairs that encodes changes to the
/// previous metadata files for the table
std::vector<MetadataLogEntry> metadata_log;
/// A list of sort orders
std::vector<std::shared_ptr<SortOrder>> sort_orders;
std::vector<std::shared_ptr<iceberg::SortOrder>> sort_orders;
/// Default sort order id of the table
int32_t default_sort_order_id;
/// A map of snapshot references
Expand All @@ -122,16 +122,16 @@ struct ICEBERG_EXPORT TableMetadata {
int64_t next_row_id;

/// \brief Get the current schema, return NotFoundError if not found
Result<std::shared_ptr<Schema>> Schema() const;
Result<std::shared_ptr<iceberg::Schema>> Schema() const;
/// \brief Get the current schema by ID, return NotFoundError if not found
Result<std::shared_ptr<iceberg::Schema>> SchemaById(
const std::optional<int32_t>& schema_id) const;
/// \brief Get the current partition spec, return NotFoundError if not found
Result<std::shared_ptr<PartitionSpec>> PartitionSpec() const;
Result<std::shared_ptr<iceberg::PartitionSpec>> PartitionSpec() const;
/// \brief Get the current sort order, return NotFoundError if not found
Result<std::shared_ptr<SortOrder>> SortOrder() const;
Result<std::shared_ptr<iceberg::SortOrder>> SortOrder() const;
/// \brief Get the current snapshot, return NotFoundError if not found
Result<std::shared_ptr<Snapshot>> Snapshot() const;
Result<std::shared_ptr<iceberg::Snapshot>> Snapshot() const;
/// \brief Get the snapshot of this table with the given id
Result<std::shared_ptr<iceberg::Snapshot>> SnapshotById(int64_t snapshot_id) const;

Expand All @@ -145,7 +145,7 @@ ICEBERG_EXPORT std::string ToString(const SnapshotLogEntry& entry);
ICEBERG_EXPORT std::string ToString(const MetadataLogEntry& entry);

/// \brief The codec type of the table metadata file.
ICEBERG_EXPORT enum class MetadataFileCodecType {
enum class ICEBERG_EXPORT MetadataFileCodecType {
Copy link
Contributor Author

@HuaHuaY HuaHuaY Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/apache/iceberg-cpp/actions/runs/17463347891/job/49593031039

In file included from /home/runner/work/iceberg-cpp/iceberg-cpp/src/iceberg/catalog/in_memory_catalog.cc:27:
/home/runner/work/iceberg-cpp/iceberg-cpp/src/iceberg/table_metadata.h:148:27: error: attribute ignored in declaration of ‘enum class iceberg::MetadataFileCodecType’ [-Werror=attributes]
  148 | ICEBERG_EXPORT enum class MetadataFileCodecType {
      |                           ^~~~~~~~~~~~~~~~~~~~~
/home/runner/work/iceberg-cpp/iceberg-cpp/src/iceberg/table_metadata.h:148:27: note: attribute for ‘enum class iceberg::MetadataFileCodecType’ must follow the ‘enum class’ keyword

kNone,
kGzip,
};
Expand Down
1 change: 0 additions & 1 deletion src/iceberg/transform.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

#include <format>
#include <regex>
#include <utility>

#include "iceberg/transform_function.h"
#include "iceberg/type.h"
Expand Down
Loading