Skip to content

Specify default opt/debug options for different CMAKE_BUILD_TYPEs#409

Merged
knoepfel merged 4 commits intomainfrom
maintenance/specify-opt-debug-flags
Mar 19, 2026
Merged

Specify default opt/debug options for different CMAKE_BUILD_TYPEs#409
knoepfel merged 4 commits intomainfrom
maintenance/specify-opt-debug-flags

Conversation

@greenc-FNAL
Copy link
Copy Markdown
Contributor

  • May be overridden from CMake command line, environment, etc.

@greenc-FNAL
Copy link
Copy Markdown
Contributor Author

@phlexbot cmake-fix

@github-actions
Copy link
Copy Markdown
Contributor

Automatic cmake-format fixes pushed (commit ff40cdd).
⚠️ Note: Some issues may require manual review and fixing.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds explicit default C++ compiler flags for common CMake build types at the top level, aiming to standardize optimization/debug settings across builds.

Changes:

  • Introduce cached defaults for CMAKE_CXX_FLAGS_DEBUG, CMAKE_CXX_FLAGS_RELEASE, and CMAKE_CXX_FLAGS_RELWITHDEBINFO.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             main     #409      +/-   ##
==========================================
- Coverage   84.43%   84.37%   -0.07%     
==========================================
  Files         127      127              
  Lines        3329     3329              
  Branches      564      564              
==========================================
- Hits         2811     2809       -2     
- Misses        325      326       +1     
- Partials      193      194       +1     
Flag Coverage Δ
unittests 84.37% <ø> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d7eb1a...bedb14b. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@greenc-FNAL greenc-FNAL force-pushed the maintenance/specify-opt-debug-flags branch from ff40cdd to 24926bc Compare March 16, 2026 16:08
@greenc-FNAL
Copy link
Copy Markdown
Contributor Author

Review the full CodeQL report for details.

@greenc-FNAL greenc-FNAL force-pushed the maintenance/specify-opt-debug-flags branch from 24926bc to 13407aa Compare March 16, 2026 19:17
@knoepfel
Copy link
Copy Markdown
Member

@greenc-FNAL, is this comment by Copilot relevant? If not, please resolve the conversation.

greenc-FNAL and others added 3 commits March 18, 2026 13:26
- May be overridden from CMake command line, environment, etc.
Replace string-truthiness if(NOT CMAKE_CXX_FLAGS_<CONFIG>) guards with
if(NOT DEFINED CACHE{CMAKE_CXX_FLAGS_<CONFIG>}) and drop FORCE.

- DEFINED CACHE{} correctly tests whether the variable was explicitly
  placed in the cache (e.g. via -DCMAKE_CXX_FLAGS_DEBUG=... on the
  command line), whereas the original string-truthiness check would
  incorrectly fire even when the user supplied an empty string.

- FORCE was never needed: the guard already prevented the set() from
  running when a cached value existed, so FORCE was always a no-op here
  (and misleading to readers).

- The CMAKE_CXX_FLAGS_<CONFIG>_INIT variables proposed in the review
  are not appropriate here.  GCC's CMake integration initialises them
  with string(APPEND ...), so setting them before project() and then
  letting GCC append its own defaults would produce duplicate flags
  (e.g. -Og -g -g for Debug).  Pre-populating the cache variable
  directly (without FORCE) is the correct mechanism: cmake_initialize_
  per_config_variable() uses set(... CACHE STRING ...) without FORCE,
  so it silently skips variables already present in the cache.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@greenc-FNAL greenc-FNAL force-pushed the maintenance/specify-opt-debug-flags branch from 13407aa to 7c2d602 Compare March 18, 2026 18:38
Copy link
Copy Markdown
Contributor Author

@greenc-FNAL greenc-FNAL left a comment

Choose a reason for hiding this comment

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

Thank you for the review. I have investigated the assertions and proposed solution carefully against the CMake 3.31 documentation and source, and the fix applied differs from the suggestion for the following reasons.

What was wrong with the original code

Two real issues existed:

  1. FORCE was misleading and redundant. The if(NOT CMAKE_CXX_FLAGS_DEBUG) guard already skips the set() entirely when the variable has a non-empty cached value, so FORCE could never actually override a user-supplied value. The combination was at best confusing.
  2. The string-truthiness check if(NOT CMAKE_CXX_FLAGS_DEBUG) would not respect a user who explicitly passed -DCMAKE_CXX_FLAGS_DEBUG= (an intentionally empty string), because an empty string is falsy. Using if(NOT DEFINED CACHE{CMAKE_CXX_FLAGS_DEBUG}) (available since CMake 3.14, well within our 3.31 requirement) correctly tests whether the variable was explicitly placed in the cache by the user.

The reviewer's assertion that FORCE "overrides user-provided cache values" is not quite right as written — the guard prevents that — but the underlying concern about the check being imprecise is valid.

Why the _INIT suggestion is not appropriate here

CMAKE_<LANG>_FLAGS_<CONFIG>_INIT is documented as being "meant to be set by a toolchain file." More importantly, CMake's GCC compiler information module (Modules/Compiler/GNU.cmake) populates these variables using string(APPEND ...), not set():

string(APPEND CMAKE_${lang}_FLAGS_DEBUG_INIT " -g")
string(APPEND CMAKE_${lang}_FLAGS_RELEASE_INIT " -O3 -DNDEBUG")
string(APPEND CMAKE_${lang}_FLAGS_RELWITHDEBINFO_INIT " -O2 -g -DNDEBUG")

This runs during project(), after our CMakeLists.txt code before project(). So if we set CMAKE_CXX_FLAGS_DEBUG_INIT "-Og -g" first, GCC would then append its own -g, yielding -Og -g -g in the final cache variable. The suggested approach would introduce duplicate flags.

By contrast, cmake_initialize_per_config_variable() (the CMake function that promotes _INIT values into the CMAKE_<LANG>_FLAGS_<CONFIG> cache variables) uses set(... CACHE STRING ...) without FORCE. A variable already present in the cache is left untouched. Setting the cache variable directly before project() — which is what the original and corrected code both do — is therefore the correct and intentional mechanism for establishing project-level defaults that survive the language initialization.

What was actually fixed

  • Replaced if(NOT CMAKE_CXX_FLAGS_<CONFIG>) with if(NOT DEFINED CACHE{CMAKE_CXX_FLAGS_<CONFIG>}) for correct cache-presence semantics.
  • Removed FORCE (it was always a no-op with the guard in place, and is misleading).
  • Added descriptive strings to the CACHE STRING entries (they were empty).
  • Added a comment explaining why the variables are set before project().

@greenc-FNAL
Copy link
Copy Markdown
Contributor Author

@phlexbot cmake-fix

@github-actions
Copy link
Copy Markdown
Contributor

Automatic cmake-format fixes pushed (commit bedb14b).
⚠️ Note: Some issues may require manual review and fixing.

@knoepfel knoepfel merged commit b2205f1 into main Mar 19, 2026
38 of 39 checks passed
@knoepfel knoepfel deleted the maintenance/specify-opt-debug-flags branch March 19, 2026 14:20
Copilot AI added a commit that referenced this pull request Mar 21, 2026
Conflicts resolved:
- .github/workflows/{clang-format-fix,cmake-format-fix,codeql-analysis,
  header-guards-fix,jsonnet-format-fix,markdown-fix,python-fix,yaml-fix}.yaml:
  our branch had YAML formatting applied to older workflow infrastructure;
  main has comprehensively refactored these files (workflow-setup composite
  actions, PR #411 consolidation, etc.). Took main's version for all.
- phlex/core/input_arguments.hpp: our branch added a
  detail::verify_no_duplicate_input_products declaration with
  phlex_core_EXPORT; main removed this check entirely (PR #424). Took
  main's version; the corresponding input_arguments.cpp is deleted by
  the merge automatically.

PRs already merged into main that overlap with this PR:
- PR #409 (Specify default opt/debug flags): auto-merged cleanly; our
  PhlexOptimization.cmake PHLEX_ENABLE_IPO/PHLEX_HIDE_SYMBOLS options are
  orthogonal to the per-build-type CXX_FLAGS_ cache variables.
- PR #410 (Enable compile/link time profiling): auto-merged cleanly;
  ENABLE_BUILD_PROFILING option and RULE_LAUNCH_* logic are independent of
  our phlex_apply_optimizations() target-level flags.

Goals of this PR continue to be met:
- PhlexSymbolVisibility.cmake and PhlexOptimization.cmake unchanged
- All 5 shared libraries: phlex_apply_symbol_visibility() +
  phlex_apply_optimizations() + phlex_make_internal_library()
- _internal targets and layer_generator_internal remain conditional on
  PHLEX_HIDE_SYMBOLS
- Tests use _internal variants; benchmark tests remain on public libs
- PHLEX_HIDE_SYMBOLS and PHLEX_ENABLE_IPO independently default ON for
  Release/RelWithDebInfo, OFF for Debug/unset; all four ON/OFF combos valid

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Framework-R-D/phlex/sessions/8c6e042a-f37c-4d4d-93c2-7597fdd5e9d9
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.

3 participants