Skip to content

build: scope warnings-as-error to project targets, not FetchContent subprojects#819

Open
yadavay-amzn wants to merge 1 commit into
apache:mainfrom
yadavay-amzn:fix-246-werror-subprojects
Open

build: scope warnings-as-error to project targets, not FetchContent subprojects#819
yadavay-amzn wants to merge 1 commit into
apache:mainfrom
yadavay-amzn:fix-246-werror-subprojects

Conversation

@yadavay-amzn

Copy link
Copy Markdown

What

Fixes #246. -Werror (via a global CMAKE_COMPILE_WARNING_AS_ERROR) leaked into FetchContent-built dependencies, so a warning in a third-party subproject (e.g. Arrow) could fail the whole build. As noted on the issue, the global flag does not reliably isolate subprojects.

How

  • Removed the global CMAKE_COMPILE_WARNING_AS_ERROR ON.
  • Set COMPILE_WARNING_AS_ERROR ON as a per-target property on Iceberg's own shared/static libraries, so the project stays strict.
  • Set BUILD_WARNING_LEVEL PRODUCTION for Arrow to suppress its independent -Werror injection.
  • Kept a defensive guard in prepare_fetchcontent() for the case where -DCMAKE_COMPILE_WARNING_AS_ERROR=ON is passed on the command line.

Verified

Iceberg targets still compile with -Werror; FetchContent subprojects do not. Build + tests green (18/18).


This contribution was authored with assistance from Claude Opus 4.8, in line with the Iceberg guidelines for AI-assisted contributions (https://iceberg.apache.org/contribute/#guidelines-for-ai-assisted-contributions). All changes were reviewed and tested by the author.

@zhjwpku

zhjwpku commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Could you please configure pre-commit locally? It seems the patch contains some unnecessary changes.

@yadavay-amzn yadavay-amzn force-pushed the fix-246-werror-subprojects branch from 1131c39 to e8a1856 Compare July 6, 2026 05:12
@yadavay-amzn

Copy link
Copy Markdown
Author

Done - configured pre-commit locally with the pinned cmake-format (v0.6.10), reverted the unrelated cosmetic reformattings, and re-pushed. The diff now contains only the warnings-as-error scoping change and passes pre-commit.

@zhjwpku zhjwpku added the ready to merge This PR has been approved and it is ready to merge. label Jul 6, 2026

@evindj evindj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The change looks good to me. However, I wonder if this comes from an hypothetical or an actual occurence where the dependencies triggering the issue was not able/willing to address. I generally agree that we should do this but I wonder if relaxing the requirement causes side effects.

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

Labels

ready to merge This PR has been approved and it is ready to merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Werror in CMake Build propogates warnings from subprojects as errors

3 participants