Skip to content

fix: stop -Werror from propagating to downstream consumer builds#2560

Open
mvanhorn wants to merge 1 commit into
borglab:developfrom
mvanhorn:fix/2401-gtsam-werror-downstream
Open

fix: stop -Werror from propagating to downstream consumer builds#2560
mvanhorn wants to merge 1 commit into
borglab:developfrom
mvanhorn:fix/2401-gtsam-werror-downstream

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Building GTSAM with -Werror no longer forces warnings-as-errors onto downstream projects that consume GTSAM via CMake.

Why this matters

cmake/GtsamBuildTypes.cmake set -Werror into GTSAM_COMPILE_OPTIONS_PRIVATE_COMMON, a CACHE variable, so the flag leaked into consuming builds and a single upstream-compiler warning broke downstream compilation (#2401, which @varunagrawal wants fixed). This gates -Werror so it applies only to GTSAM's own top-level build, not to consumers, and recomputes the flag on each configure so toggling -DGTSAM_BUILD_WITH_WERROR in an existing build directory takes effect (stale cached -Werror is scrubbed).

Testing

  • A fresh configure with GTSAM_BUILD_WITH_WERROR=ON adds -Werror to GTSAM's private options; OFF removes it; a previously-ON build directory reconfigured with OFF no longer carries -Werror.

Closes #2401

@Gold856

Gold856 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

This only works if gtsam is pulled via FetchContent or similar. If it's installed instead, and the macro is used at the top-level, it won't work.

@mvanhorn

mvanhorn commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Good catch, and you're right about the boundary. The top-level check (CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME) only covers the case where gtsam is built in-tree as a subproject (FetchContent / add_subdirectory), which is exactly the #2401 repro: the consumer's build pulls gtsam in and gtsam's own targets get -Werror and fail under a newer compiler.

Two cases I want to separate:

  1. Installed gtsam consumed the normal way (find_package(GTSAM) + target_link_libraries(app gtsam)): -Werror lives in GTSAM_COMPILE_OPTIONS_PRIVATE_COMMON and is applied PRIVATE in gtsam_apply_build_flags, so it's never in the exported INTERFACE. Those consumers were already unaffected.
  2. Installed gtsam where the downstream calls gtsam_apply_build_flags() on its own target at its top level: here CMAKE_PROJECT_NAME == PROJECT_NAME is the downstream's name, the option defaults ON, and -Werror lands on the downstream target. That's the hole, the project-name heuristic is the wrong axis for that path.

Is (2) the scenario you have in mind? If so, I think the cleaner fix is to stop keying off "am I the top-level project" and only enable -Werror when flags are applied to gtsam's own targets (gate it inside gtsam_apply_build_flags on a gtsam-owned-target check), so reusing the macro downstream never re-enables it. Happy to rework it that way, just want to confirm that's the path you'd want before I redo the gating.

@Gold856

Gold856 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

That is the scenario I'm thinking of, although I'm not sure if gating it is the right solution, since I'm not really sure what the use cases are for the gtsam_apply_build_flags macro. Ultimately, I'm really curious as to why a downstream consumer is reusing that macro, since that feels a little strange to me (I'd typically just apply the flags independently), and I'd like to know if that's actually a supported use case. If it is, then gating it behind a "Is gtsam project" check is probably fine.

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.

-Werror in GTSAM forces errors in downstream projects

2 participants