[core] Avoid ROOTSYS in TSystem include path

[guitargeek]

Using environment variables like ROOTSYS in the include paths stored in TSystem - which are used in ACLiC - is problematic if we want to move away from such environment variables.

This commit suggests to figure out instead the actual ROOTSYS path with gROOT->GetRootSys(), and then fill it into the TSystem include path. This makes ALiC work reliably even if ROOTSYS is not defined as an environment variable.

The code is further simplified by using private preprocessor macros defined at compile time to pass around the CMAKE_INSTALL_INCLUDEDIR.

For illustration, here is the output of root_build/bin/root -b -q -e "gSystem->GetIncludePath()" on my system before and after this PR. Before:

(const char *) "-I$ROOTSYS/include"

After:

(const char *) "-I/home/rembserj/code/root/root_install/include"

Or, if running bin/root in the build tree:

(const char *) "-I/home/rembserj/code/root/root_build/include"

Spinoff from this PR, where I want to use ACLiC for some dictionary generation at test time, without ROOTSYS in the environment:

• [cppyy] Enable tests on windows #18872

[pcanal]

Were you able to test this with a relocated install? (i.e. installed in $mylocal_install/root and then copied/moved to $myusable_install/root (of course with $mylocal_install/root being deleted)

[guitargeek]

Yes, that works without problem (checked both with root and root.exe, where ROOTSYS is never set at all). The gROOT->GetRootSys() method figures it out.

[github-actions]

Test Results

    22 files      22 suites   3d 18h 6m 41s ⏱️
 3 790 tests  3 789 ✅ 0 💤 1 ❌
80 293 runs  80 292 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 2770792.

♻️ This comment has been updated with latest results.

[guitargeek]

@pcanal, thanks for your suggestions. I have implemented them.

[pcanal]

LGTM. Thanks.

[ellert]

This breaks when CMAKE_INSTALL_INCLUDEDIR is an absolute path, since it always prepends the rootsys directory, even if the path is not a relative path.
This uses the install include path also when ROOTIGNOREPREFIX is set in order to run tests in the install tree where the include path is always "include" even if the configured install path is something else, e.g. "include/root".
This breaks ROOT::FoundationUtils::GetIncludeDir(), since it relies on the macro ROOTINCDIR:

root/core/foundation/src/FoundationUtils.cxx

Lines 181 to 194 in 6ecf2d7

	const std::string& GetIncludeDir() {
	#ifdef ROOTINCDIR
	if (!IgnorePrefix()) {
	const static std::string rootincdir = ROOTINCDIR;
	return rootincdir;
	}
	#endif
	static std::string rootincdir;
	if (rootincdir.empty()) {
	const std::string& sep = GetPathSeparator();
	rootincdir = GetRootSys() + sep + "include" + sep;
	}
	return rootincdir;
	}

and ROOTINCDIR in turn is defined by incdir:

root/config/RConfigure.in

Line 12 in 6ecf2d7

#define ROOTINCDIR "@incdir@"

and the definition of incdir was removed from cmake/modules/RootConfiguration.cmake by this PR.
This also means that TROOT::GetIncludeDir() is broken, since it calls ROOT::FoundationUtils::GetIncludeDir():

root/core/base/src/TROOT.cxx

Lines 3101 to 3106 in 6ecf2d7

	const TString& TROOT::GetIncludeDir() {
	// Avoid returning a reference to a temporary because of the conversion
	// between std::string and TString.
	const static TString includedir = ROOT::FoundationUtils::GetIncludeDir();
	return includedir;
	}

So please revert this.
Then change the initialization for fInclude to "-I" + TROOT::GetIncludeDir().
This I think will achieve thw desired result without breaking a lot of other things.

[guitargeek]

This breaks when CMAKE_INSTALL_INCLUDEDIR is an absolute path

I was aware of this limitation, but tolerated it because the path is conventionally relative. Having CMAKE_INSTALL_INCLUDEDIR as an absolute path is not good practice as far as I remember. It should be a relative path inside the install prefix. Do you have a usecase where the path needs to be absolute?

I agree with your other observations, but I think the conclusion should be different. I think TROOT::GetIncludeDir() should also be updated to rely on CMAKE_INSTALL_INCLUDEDIR directly. Then we can also remove the hardcoded include/:

root/core/foundation/src/FoundationUtils.cxx

Line 191 in 6ecf2d7

rootincdir = GetRootSys() + sep + "include" + sep;

I'll make a PR to show what I mean

[ellert]

Using absolute paths is the standard practice for package builds in distributions, so this not working will be disruptive. The current cmake files allow both - prepending the prefix already in cmake in case the paths are relative:

root/cmake/modules/RootConfiguration.cmake

Lines 42 to 51 in 6ecf2d7

	if(IS_ABSOLUTE ${CMAKE_INSTALL_SYSCONFDIR})
	set(etcdir ${CMAKE_INSTALL_SYSCONFDIR})
	else()
	set(etcdir ${prefix}/${CMAKE_INSTALL_SYSCONFDIR})
	endif()
	if(IS_ABSOLUTE ${CMAKE_INSTALL_BINDIR})
	set(bindir ${CMAKE_INSTALL_BINDIR})
	else()
	set(bindir ${prefix}/${CMAKE_INSTALL_BINDIR})
	endif()

and following lines. (The definition of incdir was here before too). These paths are only used for gnu-install builds. The non-gnuinstall builds assume that the relative install path are the same as the relative build paths.
Not using TROOT::GetIncludeDir() to initialize fInclude would result in code duplication and repeating the intricate logic for handling relative and absolute paths and different settings of ROOTIGNOREPREFIX in more than one place.
Granted, the current TROOT::GetIncludeDir() doesn't allow using another value for CMAKE_INSTALL_INCLUDEDIR than "include" for a non-gnuinstall build. Up to this point non-gnuistall builds never allowed changing the default relative paths and the GetXxxDir() functions always used the same relative paths as in the build directory for non-gnuinstall builds. So the ROOTIGNOREPREFIX did not have to be checked for non-gnuinstall builds, since the relative paths are the same for the build and install trees in this case. If you start allowing changing the install paths for non-gnuinstall builds you also need to start checking the ROOTIGNOREPREFIX for them as is currently doen for the gnuinstall builds.

On the other hand the gnuinstall builds have a hardcoded install directories and can not be relocated.

In my opinion, fInclude shou be initialized using GetIncludeDir() as every other place that requires the include directory in the code does it. Any improvements, like allowing non-default values for paths in non-gnuinstall builds should be implemented in the GetXxxDir() functions, so that such changes only have to be implemented in one place.

[guitargeek]

Using absolute paths is the standard practice for package builds in distributions, so this not working will be disruptive. The current cmake files allow both - prepending the prefix already in cmake in case the paths are relative:

Okay fair enough, I don't see why but if that's widely done then we should account for it. That's easy: we can just infer the relative path, from CMAKE_INSTALL_PREFIX from CMAKE_INSTALL_INCLUDEDIR. But note that this is even discouraged in the CMake documentation.

In my opinion, fInclude shou be initialized using GetIncludeDir() as every other place that requires the include directory in the code does it. Any improvements, like allowing non-default values for paths in non-gnuinstall builds should be implemented in the GetXxxDir() functions, so that such changes only have to be implemented in one place.

Yes, absolutely. Sorry I was not clear enough, I meant to imply that GetIncludeDir() will be rewritten but then used to set the TSystem include path, just like you suggested. Like that, there won't be any code duplication.

I'm suggesting this followup PR:

• [core] Generalize TROOT::GetIncludeDir() using build tree marker #20828

If you start allowing changing the install paths for non-gnuinstall builds you also need to start checking the ROOTIGNOREPREFIX for them as is currently doen for the gnuinstall builds.

We have started to allow that, and explicitly do that in the build of the Python wheels. However, I don't see why we need to check the ROOTIGNOREPREFIX what's the point of it anyway?

I think the only use of ROOTIGNOREPREFIX is for running tests in the build tree when the header install directory is different from the header directory in the build tree. In that case, setting ROOTIGNOREPREFIX will make GetIncludeDir() fall through to $ROOTSYS/include, which is always correct for the build tree. But then, I'd argue that this mechanism is also a bit opaque.

But the possible differences in the relative header/library/etc dirs between install and build tree are indeed a problem that needs to be addressed, while keeping both build and install tree relocatable. My goal for this is to introduce some other mechanism to auto-detect that, so that tests don't need to explicitly set ROOTIGNOREPREFIX. A simple marker file in the lib dir of only the build tree would be sufficient and AFAIK robust, which is what I have in mind right now to solve this problem longterm.