[core] Add test for ROOT_LIBRARY_PATH. by pcanal · Pull Request #23123 · root-project/root

pcanal · 2026-08-20T21:18:52Z

This has been adapted from root-project/roottest#686 by making the test CMake based for platform independence.

This is related to #7031

Created with the help of AI: Claude Opus 5 and Claude Fable 5

Co-authored-by: Christian Tacke 58549698+ChristianTackeGSI@users.noreply.github.com

github-actions · 2026-08-20T23:10:24Z

Test Results

23 files 23 suites 3d 15h 49m 39s ⏱️
3 866 tests 3 864 ✅ 0 💤 2 ❌
79 683 runs 79 679 ✅ 1 💤 3 ❌

For more details on these failures, see this check.

Results for commit c8b9dd9.

♻️ This comment has been updated with latest results.

This has been adapted from root-project/roottest#686 by making the test CMake based for platform independence. Created with the help of AI: Claude Opus 5 Co-authored-by: Christian Tacke <58549698+ChristianTackeGSI@users.noreply.github.com>

The solution was current with the help of Claude Fable 5. The failing output tells the story: ``` ...;.;C:\ROOT-CI\build\bin;C:\ROOT-CI\build\lib;C:\ROOT-CI\build\bin ``` the `.rootrc` entry (`...\rootrcpath`) is missing, instead the built-in default `.;<bindir>` appears, and there's an extra trailing `<bindir>` entry. The cause is an initialization-order bug specific to Windows: TROOT::InitSystem() calls gSystem->Init() before creating gEnv = new TEnv(".rootrc"). On Windows, TWinNTSystem::Init() calls AddDynamicPath(<dir of libCore.dll>) (that's the trailing C:\ROOT-CI\build\bin). AddDynamicPath called DynamicPath(0, kFALSE) which, finding the cached path empty, assembled the full path right then — with gEnv == nullptr. So Root.DynamicPath from .rootrc could not be read and the hard-coded default .;<bindir> was baked in. The result was cached in a static TString, so the rootrc resource was permanently ignored. On Unix this doesn't happen because TUnixSystem::Init() never touches the dynamic path that early (and the Unix DynamicPath() has explicit late-initialization logic via its seenCling/initialized flags). Fix: Fable reworked DynamicPath() in TWinNTSystem.cxx to: - Track directories added via AddDynamicPath() separately (extrapath) instead of freezing the whole assembled path, - Remember whether the cached path was assembled while gEnv was available (sawEnv), and re-assemble it once gEnv exists, re-appending the extra directories — so the WinNT.*.Root.DynamicPath resource is honored, - Keep respecting explicitly user-set paths (SetDynamicPath) via a userpath flag, and reset all state on reset. TWinNTSystem::AddDynamicPath() now simply forwards the directory via the new addpath parameter. What is the advantage of the proposed change in TWinNTSystem::AddDynamicPath?: The old implementation was: ``` TString oldpath = DynamicPath(0, kFALSE); // forces full assembly *now* oldpath.Append(";"); oldpath.Append(dir); DynamicPath(oldpath); // stores it as if user-set ``` That had two problems, both central to the Windows CI failure: 1. It forced premature assembly of the path. DynamicPath(0, kFALSE) triggers the full construction of the default path (ROOT_LIBRARY_PATH + PATH + rootrc + libdir). Since TWinNTSystem::Init() calls AddDynamicPath(<libCore dir>) before gEnv exists, the path got built with the Root.DynamicPath rootrc resource unavailable — so the .rootrc entries were silently dropped and the hard-coded default .;<bindir> was baked in. 2. It froze the result as if it were a user-set path. Passing the concatenated string back as newpath made it indistinguishable from a user calling SetDynamicPath(). There was no way for DynamicPath() to know it could/should re-assemble the default part later (once gEnv became available) without discarding the appended directory. The new form: ``` DynamicPath(0, kFALSE, dir); ``` expresses the actual intent — "append this directory" — as its own operation: - No premature assembly: appending a directory no longer requires materializing the whole default path first. - Separable state: the appended dirs are accumulated in extrapath, distinct from the cached default (dynpath) and from an explicit user path (userpath). When gEnv later becomes available, DynamicPath() can rebuild the default part correctly (now including the rootrc Root.DynamicPath resource) and simply re-append extrapath — the AddDynamicPath additions survive the rebuild. - Correct semantics preserved: SetDynamicPath() still fully overrides (and is never re-assembled), matching the previous user-facing behavior. In short, the change converts "read–modify–write of an opaque cached string" into a declarative append, which is what makes the deferred re-assembly (the actual bug fix) possible.

Apply to TUnixSystem the same model introduced for Windows in the previous commit ("[core/Windows] Ensure WinNT.*.Root.DynamicPath is used"): give the static DynamicPath() helper an `addpath` parameter and implement TUnixSystem::AddDynamicPath as a simple, declarative append instead of a read-modify-write of the cached path. The old implementation TString oldpath = DynamicPath(nullptr, kFALSE); oldpath.Append(":"); oldpath.Append(path); DynamicPath(oldpath); had several latent problems: - It went through the `newpath` route, which sets `seenCling`. If AddDynamicPath was called before the interpreter was started, this permanently lost the deferred insertion of gCling->GetSTLIncludePath() into the search path: the path was frozen as if the user had set it explicitly via SetDynamicPath. - It was not atomic: it acquired gSystemMutex twice (once to read, once to write), so concurrent AddDynamicPath calls could lose one of the appended directories. - It conflated "path explicitly set by the user" with "directory internally appended", making it impossible for DynamicPath() to re-assemble the automatic part later. The new implementation records appended directories in a separate `extrapath` static (under a single lock acquisition) and re-appends them, after the system part, whenever the path is (re-)assembled. `seenCling` is left untouched so the Cling provided parts are still inserted once the interpreter becomes available. Additionally, fix a pre-existing bug in the reset path (SetDynamicPath(nullptr)): when the path had previously been set explicitly, `initialized` and `seenCling` were both true and the re-assembly block was skipped, returning the stale user path instead of the recomputed default. The reset now clears `extrapath`, re-arms `seenCling`, and forces re-assembly. SetDynamicPath semantics are unchanged: an explicitly set path is frozen and never automatically overridden.

Add a gtest covering the TSystem dynamic path API, exercising the behavior relied upon by the recent fixes to the static DynamicPath() helpers in TUnixSystem and TWinNTSystem: - the default path contains the ROOT library directory; - AddDynamicPath appends the directory at the end, exactly once, keeping the rest of the path intact, and accumulates in order; - AddDynamicPath(nullptr) is a no-op; - SetDynamicPath freezes the path to exactly the given value; - SetDynamicPath(nullptr) resets to the default, dropping both the explicitly set value and previously appended directories.

…dows On Windows the TWinNTSystem constructor prepends "<rootsys>\bin;" to the PATH environment variable itself (so that ROOT's DLLs are always found), before the dynamic path is assembled. The loader part of the dynamic path is therefore "<rootsys>\bin;<PATH set by the test>" and the order-sensitive checks of dynamicPathSetup.cmake failed with: dynamic path: .../rootlibpath;C:\ROOT-CI\build\bin;.../ldpath;... dynamic path should start with: .../rootlibpath;.../ldpath Detect the prefix actually added by ROOT - by comparing gSystem->Getenv("PATH") with the PATH set by the script - instead of hardcoding "<rootsys>\bin", since the prepend does not happen for all build flavours (e.g. ROOTPREFIX builds), and factor it into the check_begin expectations and the environment part re-assembly. No change of behavior on Unix where PATH is not modified by ROOT and the detected prefix is empty.

TWinNTSystem's DynamicPath() ended with an unconditional if (!dynpath.Contains(TROOT::GetLibDir())) { dynpath += ";"; dynpath += TROOT::GetLibDir(); } executed on *every* call, including for a path set explicitly through SetDynamicPath() and for the value returned by GetDynamicPath(). As a result SetDynamicPath(p) followed by GetDynamicPath() did not return `p` but `p;<libdir>`, as caught by the TSystem.DynamicPath unit test: Expected equality of these values: userPath ... "...root-dynpath-gtest-2;...root-dynpath-gtest-1" gSystem->GetDynamicPath() ... "...gtest-2;...gtest-1;C:\ROOT-CI\build\lib" The guarantee that the library directory is part of the path only makes sense for the automatically assembled path, where it is already enforced (right before the appended directories), and TUnixSystem has never applied it to a user provided path. Drop the extra append so that Windows behaves like Unix: an explicitly set path is used verbatim and is never amended.

Replace the hand-rolled `#ifdef WIN32 ... ';' ... #else ... ':' ... #endif` blocks used to select the PATH environment variable separator by the existing shared helper `ROOT::FoundationUtils::GetEnvPathSeparator()` declared in ROOT/FoundationUtils.hxx. Affected files: - core/base/src/TPluginManager.cxx - core/base/src/TROOT.cxx - core/base/test/TSystemTests.cxx - core/clingutils/src/TClingUtils.cxx - core/metacling/src/TCling.cxx - core/rint/src/TTabCom.cxx - tree/treeplayer/src/TTreeGeneratorBase.cxx In TTreeGeneratorBase.cxx the directory separator now also comes from the companion helper `ROOT::FoundationUtils::GetPathSeparator()`. core/base/test/CMakeLists.txt gains core/foundation/res in the include directories of the CoreSystemTests target so that the test can include ROOT/FoundationUtils.hxx. No functional change.

jblomer

The commit messages are too verbose. Arguments are repeated, forward references to future commits, file lists appear (I can see the touched files from the commit itself), etc.

jblomer · 2026-09-11T21:55:48Z

@@ -0,0 +1,219 @@
+# CMake re-implementation of the former `test_dynpath_setup.sh` shell script.


The test_dynpath_setup.sh file was never committed, so I find this comment somewhat confusing. Consider removing this line.

jblomer · 2026-09-11T22:16:12Z

+
+# The dynamic path must contain `expected` as a whole sequence of entries.
+function(check_mid expected)
+  string(FIND "${sep}${cur_dynpath}${sep}" "${sep}${expected}${sep}" pos)


Is the path always ending with a separator? I.e. will this miss expected as a tail of cur_dynpath?

jblomer · 2026-09-12T21:00:51Z

+         if (!dynpath.Contains(TROOT::GetLibDir())) {
+            dynpath += ";"; dynpath += TROOT::GetLibDir();
+         }
+         dynpath += extrapath;  // entries carry a leading ';'
      }
      if (!dynpath.Contains(TROOT::GetLibDir())) {
         dynpath += ";"; dynpath += TROOT::GetLibDir();
      }


Merge with the later commit that removes the trailing block to add .

jblomer · 2026-09-12T21:17:29Z

+   static const char *DynamicPath(const char *newpath = 0, Bool_t reset = kFALSE,
+                                  const char *addpath = 0)


This signature seems more complicated that it needs to be. The newpath and addpath parameters are mutually exclusive, which is not explicitly checked.

How about a single path parameter and an enum to distinguish "new, fixed user path (includes reset)" from "add to path".

jblomer · 2026-09-12T21:27:17Z

+      addedPaths += ":";
+      addedPaths += addpath;
+      if (initialized) {
+         if (!dynpath_full.EndsWith(":"))


What if the initial newpath was empty?

jblomer · 2026-09-12T21:28:29Z

+      // provided parts can now be inserted, do it now (extrapath included).
+   }
+
   // Another thread might have updated this. Even-though this is executed at the


How can another thread have updated this? We are in a mutex-protected region.

jblomer · 2026-09-12T21:30:11Z

   if (!reset && (seenCling || (initialized && gCling == nullptr)))
      return dynpath_full;

   if (!initialized) {


Why not checking for gROOT?

jblomer · 2026-09-12T21:31:04Z

Merge the changes to TWinNTSystem with previous commit.

pcanal requested a review from dpiparo August 20, 2026 21:18

pcanal self-assigned this Aug 20, 2026

pcanal requested a review from bellenot as a code owner August 20, 2026 21:18

pcanal added priority:low in:Core Libraries in:Testing AI-Assisted labels Aug 20, 2026

pcanal mentioned this pull request Aug 20, 2026

Add test for ROOT_LIBRARY_PATH root-project/roottest#686

Closed

pcanal closed this Aug 27, 2026

pcanal reopened this Aug 27, 2026

pcanal force-pushed the test_dynamic_path branch 2 times, most recently from 6522bee to 5b40193 Compare August 27, 2026 19:36

pcanal and others added 2 commits August 27, 2026 14:37

[core] Add test for ROOT_LIBRARY_PATH.

f82a6b0

This has been adapted from root-project/roottest#686 by making the test CMake based for platform independence. Created with the help of AI: Claude Opus 5 Co-authored-by: Christian Tacke <58549698+ChristianTackeGSI@users.noreply.github.com>

pcanal force-pushed the test_dynamic_path branch from 5b40193 to 423c74e Compare August 27, 2026 19:38

pcanal marked this pull request as draft August 28, 2026 17:51

pcanal force-pushed the test_dynamic_path branch from a075059 to aa9a3d5 Compare September 2, 2026 18:58

pcanal closed this Sep 2, 2026

pcanal reopened this Sep 2, 2026

pcanal added 5 commits September 3, 2026 11:52

pcanal force-pushed the test_dynamic_path branch from aa9a3d5 to c8b9dd9 Compare September 3, 2026 16:53

pcanal marked this pull request as ready for review September 4, 2026 01:22

pcanal requested a review from jblomer as a code owner September 4, 2026 01:22

jblomer reviewed Sep 12, 2026

View reviewed changes

Navigation Menu

Repository navigation

[core] Add test for ROOT_LIBRARY_PATH. - #23123

[core] Add test for ROOT_LIBRARY_PATH.#23123
pcanal wants to merge 7 commits into
root-project:masterfrom
pcanal:test_dynamic_path

pcanal commented Aug 20, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 20, 2026 •

edited

Loading

Uh oh!

jblomer left a comment

Uh oh!

jblomer Sep 11, 2026

Uh oh!

jblomer Sep 11, 2026

Uh oh!

jblomer Sep 12, 2026

Uh oh!

jblomer Sep 12, 2026

Uh oh!

jblomer Sep 12, 2026

Uh oh!

jblomer Sep 12, 2026

Uh oh!

jblomer Sep 12, 2026

Uh oh!

jblomer Sep 12, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,219 @@
		# CMake re-implementation of the former `test_dynpath_setup.sh` shell script.

		static const char DynamicPath(const char newpath = 0, Bool_t reset = kFALSE,
		const char *addpath = 0)

Conversation

pcanal commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

jblomer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pcanal commented Aug 20, 2026 •

edited

Loading

github-actions Bot commented Aug 20, 2026 •

edited

Loading