Use ROOT_LIBRARY_PATH in DynamicPath

[ChristianTackeGSI]

ROOT's "dynamic path" has some environment variables to control it. Those environment variables have some issues:

• They are dependant on the OS (DYLD* on macOS, LD_LIBRARY_PATH on Linux, etc)
• LD_LIBRARY_PATH/etc modify the system's search path for dynamic libraries, which can result in all sorts of bad things.

We would like to have a dedicated environment variable, that is

• OS independant.
• does only affect ROOT.

Let's name it ROOT_LIBRARY_PATH (suggestion on mattermost.web.cern.ch).

It was suggested to put this into system.rootrc and/or .rootrc.

This has some issues:

• .rootrc is good for a per user solution. We would like to have a package level solution.
• system.rootrc is usually a place for the local sysadmin to modify. It could be used by a distribution package to put "defaults". But that's not really nice.
• Finally, .rootrc entries replace system.rootrc entries. So any package level configuration would be gone the moment, that the user sets Unix.*.Root.DynamicPath.

So getting the above mentioned environment variable to work at a package level, means to put it into TUnixSystem.cpp.

cc: @dennisklein

[phsft-bot]

Can one of the admins verify this patch?

[vgvassilev]

We would also need to support this on Windows for consistency. Can you make sure that patch works on Windows too?

cc: @bellenot

[ChristianTackeGSI]

We would also need to support this on Windows for consistency. Can you make sure that patch works on Windows too?

The current patch most likely does not work on Windows (it doesn't touch core/winnt/*).
I don't have access to any Windows development machines, so I can't test any possible patches. I could draft up a suggestion on a patch.

Maybe we should first get to the point, what needs to be done for this to get accepted for the unix world? So that the windows implementation can take care of all that?

[bellenot]

Only the PATH variable is needed on Windows

[ChristianTackeGSI]

Only the PATH variable is needed on Windows

Okay, that sounds to me like Windows isn't affected by this PR.

Is there anything I can do, so we get forward with this?

[vgvassilev]

@bellenot, I am confused. This PR introduces another way to express the LD_LIBRARY_PATH. IIUC, the PATH variable is the analog of LD_LIBRARY_PATH for Windows. What happens if somebody defines a variable ROOT_LIBRARY_PATH and not update the PATH? On unix it'd work just fine but fail on Windows?

[Axel-Naumann]

So I think the argument that Windows uses %PATH is in support of having ROOT_LIBRARY_PATH also for Windows. @ChristianTackeGSI would you be able to help us with that, to get this merged for 6.24?

[bellenot]

So you want to introduce ROOT_LIBRARY_PATH on Windows? Pointing to what? DLLs or libraries? And what for?

[Axel-Naumann]

I thought the rationale is spelled out in the original post?

They are dependant on the OS (DYLD* on macOS, LD_LIBRARY_PATH on Linux, etc)
LD_LIBRARY_PATH/etc modify the system's search path for dynamic libraries, which can result in all sorts of bad things.

We would like to have a dedicated environment variable, that is

• OS independant.
• does only affect ROOT.

Why would that exclude Windows? Either we "buy" the rationale and agree that this is a useful feature (and that would then include Windows) or we think it's not needed, but then it shouldn't be needed on any platform. What am I missing / misunderstanding?

[ChristianTackeGSI]

Thanks for revisiting this!

So I think the argument that Windows uses %PATH is in support of having ROOT_LIBRARY_PATH also for Windows. @ChristianTackeGSI would you be able to help us with that, to get this merged for 6.24?

1. So the choosing of the name ROOT_LIBRARY_PATH is okayed by everyone? Just to make sure, I got it.
2. So my priority (re-)ordering is also fine?
3. I can create a patch "suggestion" for Windows. But I can't test it in any way. If that is, what you'd like me to do, I can do that, yes. Disclaimer: I can't even compile test, so it might end up not being compile-able for Windows.

[bellenot]

OK, thanks for the explanation. And don't worry, I can take care of the implementation and testing on Windows.

[bellenot]

BTW, do we have any specific test for this?

[bellenot]

So here is the diff for TWinNTSystem.cxx, if you want to include it in the same PR:

diff --git a/core/winnt/src/TWinNTSystem.cxx b/core/winnt/src/TWinNTSystem.cxx
index 71cc3be3a3..7889fc9008 100644
--- a/core/winnt/src/TWinNTSystem.cxx
+++ b/core/winnt/src/TWinNTSystem.cxx
@@ -358,24 +358,26 @@ namespace {
          dynpath = "";
       }
       if (newpath) {
-
          dynpath = newpath;
-
       } else if (dynpath == "") {
+         dynpath = gSystem->Getenv("ROOT_LIBRARY_PATH");
          TString rdynpath = gEnv ? gEnv->GetValue("Root.DynamicPath", (char*)0) : "";
          rdynpath.ReplaceAll("; ", ";");  // in case DynamicPath was extended
          if (rdynpath == "") {
             rdynpath = ".;"; rdynpath += TROOT::GetBinDir();
          }
          TString path = gSystem->Getenv("PATH");
-         if (path == "")
-            dynpath = rdynpath;
-         else {
-            dynpath = path; dynpath += ";"; dynpath += rdynpath;
+         if (!path.IsNull()) {
+            if (!dynpath.IsNull())
+               dynpath += ";";
+            dynpath += path;
+         }
+         if (!rdynpath.IsNull()) {
+            if (!dynpath.IsNull())
+               dynpath += ";";
+            dynpath += rdynpath;
          }
-
       }
-
       if (!dynpath.Contains(TROOT::GetLibDir())) {
          dynpath += ";"; dynpath += TROOT::GetLibDir();
       }

[ChristianTackeGSI]

Thanks to @bellenot for providing the Windows part. I have squashed it into the main commit and added a Co-Authored-By: Bertrand Bellenot @bellenot line (hope, that's the best way to give credit?).

BTW, do we have any specific test for this?

If I should write tests (sounds like everything else is fine), I could need a small bit of guidance (could you point me at a test, that I could learn from / extend?).

Also rebased.

[bellenot]

@ChristianTackeGSI Thanks! And my question about test mas mostly for @Axel-Naumann

[Axel-Naumann]

It'd be good to have a test for this. This can be done through Google test in root.git/core/base/test or in roottest.git/root/core. @ChristianTackeGSI would you be able to provide us with one?

[ChristianTackeGSI]

I'll look into providing some tests. I have some idea. If I need more help, I'll speak up.

[dennisklein]

Finally, .rootrc entries replace system.rootrc entries. So any package level configuration would be gone the moment, that the user sets Unix.*.Root.DynamicPath.

Just for my curiosity, if it would be possible (and if not would be desired to have):

Does the file-based rootrc-System already have the primitives (like an "include/merge other rootrc file" of sorts) to construct something like /etc/rootrc.d/<pkgname>.rootrc where each package could store its own rootrc file with [ap,pre]pend semantics for a search path entry like Root.DynamicPath?

[Axel-Naumann]

I don't think that exists. Are you suggesting we collect everything from all these files - and what is their relative priority? Or if we don't collect everything, how do we detect which "package" the current process belongs to?

Another option is for a library to modify the combined rootrc entries, by means of gEnv->GetValue(), gEnv->SetValue(), e.g. during static initialization.

[dennisklein]

Are you suggesting we collect everything from all these files - and what is their relative priority?

Yes, system.rootrc would contain a line like import /etc/rootrc.d/*.rootrc or similar. Relative priority could be resolved by sorting by filename (same as shell wildcard expansion) (e.g. can be used with xxx numeric prefix and such - not perfect, but probably practical enough). Examples:

• In autofs maps one can write +dir:/etc/auto.master.d, or
• in apache config IncludeOptional conf.d/*.conf.

I mean, one could also modify the system.rootrc file in some postinstall/preremove pkg hooks ... but that could be tricky and require more sophisticated logic than desirable in such scripts. But then again, spack, aliBuild, and other user-space pkg managers install each package into their own prefix and have no or limited concepts of shared directories across packages ...

In the end, I am looking for a good way to bake-in some default search paths at install time and from a package that depends on root, ideally without setting up environment variables at all. But having ROOT_LIBRARY_PATH would already go a long way for us to be able to resolve some conflicts we have when being forced to use LD_LIBRARY_PATH, so I don't want to stall progress on that aspect.

[ChristianTackeGSI]

Added testsuite for the Unix part.
It works on Linux. I hope it works on other Unix-like OSes.

(rebased)

[ChristianTackeGSI]

@pcanal

I added a new subsection to the release notes, please review and let me know, if this is the right way. I also added my name to the contributors, hope that's okay / wanted too? (Bertrand Bellenot is already listed)

I rebased on master, but as this seems to target 6.24, I wonder, whether I should rebase on 6-24-00-patches?

[pcanal]

v6.24 is already branched and closed for new features; i.e. we are in the last few steps of the release process.

I also added my name to the contributors, hope that's okay / wanted too?

Of course :)

[ChristianTackeGSI]

v6.24 is already branched and closed for new features; i.e. we are in the last few steps of the release process.

I am a bit confused. The "Milestone" for this PR was set to "6.24/00"?

• If this is going into 6.24, should I rebase on that branch and target it?
• If this is going into 6.26, should I rather change the v626 release notes instead of the v624 one? (and someone should update the Milestone, I guess?)

[pcanal]

If this is going into 6.24, should I rebase on that branch and target it?

I had missed Axel's message. It is indeed slated to go in 6.24. Our usual process is to first merge in master and then backport to the v6.24 patch branch.

@ChristianTackeGSI Once you update the release note, I will merge in and then can you open a new PR against the v6.24 patch branch?

[vgvassilev]

@pcanal, just heads up before merging -- #7031 (comment)

[ChristianTackeGSI]

Release notes are ready from my side.

[pcanal]

@vgvassilev thanks for the reminder.

@ChristianTackeGSI Can you remove the test commit? Do you prefer that I add to roottest or can you add it?

[ChristianTackeGSI]

@pcanal

@ChristianTackeGSI Can you remove the test commit? Do you prefer that I add to roottest or can you add it?

Okay, removed the test suite commit (and rebased to master)

Could you please take care of adding it to roottest?

The test suite commit is now here, for your reference:
https://github.com/ChristianTackeGSI/root/tree/pr/root_lib_env_test
(top single commit).

[pcanal]

@phsft-bot build

[phsft-bot]

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

[phsft-bot]

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

• projectroot.roottest.root.dataframe.roottest_root_dataframe_test_nested_rvec_snapshot

[pcanal]

The windows failures is unrelated (test output file deleted before being closed in an unrelated test)

[pcanal]

Thank you very much!

[ChristianTackeGSI]

Many thanks to everybody involved! :-)