Merged
Conversation
greenc-FNAL
requested review from
Copilot,
knoepfel,
marcpaterno and
sabasehrish
Contributor
Author
|
@phlexbot cmake-fix |
Contributor
|
Automatic cmake-format fixes pushed (commit c2b60b7). |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #410 +/- ##
==========================================
- Coverage 84.43% 84.40% -0.04%
==========================================
Files 127 127
Lines 3329 3329
Branches 564 564
==========================================
- Hits 2811 2810 -1
Misses 325 325
- Partials 193 194 +1
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in CMake configuration to profile build-time behavior (compile and link), helping developers identify expensive translation units and link steps during Phlex builds.
Changes:
- Introduces
ENABLE_BUILD_PROFILINGCMake option to wrap compile/link rules withcmake -E time. - Enables compile time tracing (
-ftime-trace) for Clang/GNU toolchains when profiling is enabled. - Enables additional LLD link tracing/statistics flags when LLD is detected.
greenc-FNAL
force-pushed
the
maintenance/enable-build-profiling
branch
from
c2b60b7 to
b0e703f
Compare
Contributor
Author
|
Review the full CodeQL report for details. |
greenc-FNAL
force-pushed
the
maintenance/enable-build-profiling
branch
from
b0e703f to
ec28a53
Compare
Member
|
@greenc-FNAL, is this comment by Copilot relevant? If not, please resolve the conversation. |
Contributor
Author
It looks relevant at first blush, although the linker's map file writer might be parallel-safe. I'll try to look later; sorry for false start. |
Remove --Map=phlex_link.map from the global LLD link options: since multiple targets are linked in parallel, using a hardcoded filename would cause all link jobs to race to write the same file, resulting in corrupt or overwritten output. The --time-trace option already generates a per-target trace file (appending .time-trace to the output file name), and --print-archive-stats=- writes to stdout, so neither of those has this issue. Also correct an if(...) formatting issue caught by gersemi. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
greenc-FNAL
force-pushed
the
maintenance/enable-build-profiling
branch
from
ec28a53 to
a38525d
Compare
knoepfel
approved these changes
Mar 19, 2026
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.