Skip to content

[rootcling] Classify LinkDefs by basename in callbacks - #23730

Open
agourakis82 wants to merge 2 commits into
root-project:masterfrom
agourakis82:investigate/23640-linkdef-include-provenance
Open

agourakis82 wants to merge 2 commits into
root-project:masterfrom
agourakis82:investigate/23640-linkdef-include-provenance

Conversation

@agourakis82

@agourakis82 agourakis82 commented Oct 15, 2026 •

Copy link
Copy Markdown

This Pull request:

Related to #23640. It fixes a demonstrated include-provenance failure; the
reporter's actual installation prefix and full environment remain unknown.

Changes or fixes:

The inclusion callback currently applies the LinkDef filename heuristic to
whole paths. An ordinary header under a directory containing LinkDef can
therefore be mistaken for a selection file, causing internal relative includes
to leak into the generated dictionary or a required include to be lost.

Classify the current and included files by basename in this callback, retaining
the original path for emitted includes. The global helper and other callers
are unchanged. Add two failing-before/passing-after dictionary compilation
tests and an explicit nested-LinkDef control for the existing clear/lock logic.

Native Linux C++23 validation:

  • Fresh revalidation on master c4011b297ee1 (2026-10-24), after the
    CppInterOp update: GCC 15.3, LLVM 22.1.8, C++23. Both directory regressions
    fail with the original callback and pass with the correction; the nested
    control passes in both. All 12 selected CTest entries pass after the fix.
  • ROOT 6.41.01 at base bf65bcfe0162, GCC 15.3, LLVM 22.1.8: 12 selected CTest entries
    pass. Separate disposable clear/lock mutations make the nested test fail;
    restoring the source makes the selected tests pass again.
  • ROOT 6.40.04, GCC 15.3, LLVM 20.1.8: the two directory tests fail before the
    fix; all 12 selected entries pass after it. Each selection contains three
    added tests, eight existing compatibility tests and one build fixture.
  • The issue's Thing.hpp/LinkDef.h example compiles before and after under
    ordinary paths. With copied GCC headers explicitly prioritized using -I
    under LinkDefCache, the original generator emits execution_defs.h and
    compilation fails; the corrected generator's output compiles.

These are minimal source builds with runtime C++ modules and PyROOT disabled,
not the reporter's complete NixOS installation or a conda-forge recipe rebuild.
The marker case is a controlled diagnostic with additional -I arguments,
not an exact reproduction of the reporter's command or prefix. Earlier setup
attempts stopped for missing crypt.h and an ineffective marker-path control;
the final 6.40.04 gate reused the preserved baseline build after correcting
those setup issues and checking source and binary identities. The full suite,
other platforms and runtime modules have not been validated.
A fresh upstream CI execution and maintainer review remain pending.

Checklist:

  • tested changes locally (native cluster builds as described above)
  • updated the docs (if necessary; no user-facing documentation change)

AI assistance: OpenAI Codex assisted with diagnosis, implementation, tests and
validation; model label supplied by the author: GPT 6.1 Sol. No independent
provider approval is claimed.

Assisted-by: codex:GPT 6.1 Sol

Directory names containing LinkDef must not turn ordinary included
headers into selection files. Otherwise the callback can collect an
internal relative include or lose a required public include.

Use basenames only for classification and keep emitted paths intact.
Cover both directory cases and the existing nested clear/lock behavior.

Related to root-project#23640. The reporter's actual prefix remains unknown.

Assisted-by: codex:GPT 6.1 Sol
@github-actions

Copy link
Copy Markdown

Test Results

    24 files      24 suites   4d 0h 27m 35s ⏱️
 3 892 tests  3 890 ✅ 0 💤 2 ❌
83 315 runs  83 313 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit b126049.

@pcanal

pcanal commented Oct 19, 2026

Copy link
Copy Markdown
Member

I am a bit confused. The PR title and example do not seem to match the tests added. In particular on the surface I do not see how only looking at the basename of execution_defs.h would change anything to the result.

@agourakis82

Copy link
Copy Markdown
Author

You're right — taking the basename of execution_defs.h alone doesn't explain it. I should have made the example clearer.

The important part is the file doing the include. For example, LinkDefDirectory/Wrapper.h is an ordinary header, but the callback can treat it as a LinkDef because it checks the whole path. It then records that header's internal includes as though they came from a selection file.

That's what the smaller tests exercise. The patch checks the basenames of both files, while keeping the original spelling for emitted includes. The nested-LinkDef test makes sure the existing clear/lock behavior still works.

I also want to be careful about the connection to the original issue: the LinkDefCache setup demonstrates this bug, but it isn't the reporter's environment. I haven't established that this was the cause of their failure.

@agourakis82
agourakis82 marked this pull request as ready for review October 24, 2026 15:18
Integrate upstream c4011b2 while preserving the reviewed rootcling
correction and its regressions. The merge tree is b36dc99, identical
to the combined tree validated by native Slurm job 16772 and strict audit
16775: two required baseline failures, the nested control passing, and
12 selected CTest entries passing with the correction.

The historical PyROOT and DistRDF CI failures also occur without this
patch. Fresh hosted CI on this combined tree remains a separate gate.
No test or workflow is weakened.

Assisted-by: codex:GPT-6

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants