Make it possible to run tests when configuring with -Dgnuinstall:BOOL=ON #130

ellert · 2016-01-09T08:23:54Z

This is a possible solution for ROOT-7538. I hope this is at least similar to what you had in mind when you wrote the comment in that ticket. Hopefully it can at least be used as a start.

Axel-Naumann · 2016-01-11T13:00:37Z

That is fantastic, wow! Thank you!

Is there a reason you decided to use an env var over a flag to trigger this? (I am honestly polling for reasons here.)

Cheers, Axel.

ellert · 2016-01-12T07:28:42Z

The reason for using an environment variable is the following:

It is a common occurrence in the root code to use initialization classes that perform configuration in its constructor, and then have a static instance of this class declared in a library so that this initialization happens automatically when the library is loaded. This initialization often depends on the various directory paths. Since the initialization of the static instances in the libraries an application links to happens before the main program starts, the behaviour of these initialization routines can not be modified by command line options, since these options are parsed by the main program. With an environment variable this works though.

Axel-Naumann · 2016-01-12T07:50:37Z

Excellent reason. It's also likely only needed in very specific situations, i.e. usability is not really a concern here.

I'll do a review the patch; looked good on a first quick glimpse!

Axel-Naumann · 2016-01-19T10:41:17Z

core/base/src/TROOT.cxx

Does it make sense to move this preprocessor logic into TROOT::GetEtcDir()? I.e. have TROOT::GetEtcDir() return TROOT::GetRootSys() for iphone / simulator?

This didn't make sense to me in the first place. I simply tried to not change what de code did so that I didn't introduce any new behaviour.

The main reason for thinking that it is strange is that if "etc does not exist on ios", then I would have expected this #ifdef to appear every place where ROOTETCDIR is used. This is not the case. It appears - if I remember correctly - twice in the whole source tree. For all the other places ROOTETCDIR is used also for ios.

If ios does not have etc I would have expected that for ios configure/cmake would default to --etcdir='${prefix}' and then ROOTETCDIR would correctly point to rootsys already and no #ifdefs would be needed.

I agree the code as it is written is strange - but I didn't want to just assume the code was wrong and "fix" it in case I had the wrong idea of what was the intended behaviour.

Thanks, that makes perfect sense. If we determine that this should happen in all instances then I will do this in a separate commit.

rassel1304 · 2016-01-20T00:25:54Z

Thx

Axel-Naumann · 2016-01-21T16:45:10Z

net/http/src/THttpServer.cxx

I get

src/net/http/src/THttpServer.cxx:150:50: error: cannot pass non-trivial object of type 'const TString' to variadic function; expected type from format string was 'char *' [-Wnon-pod-varargs] TString jsdir = TString::Format("%s/http", TROOT::GetEtcDir()); ~~ ^~~~~~~~~~~~~~~~~~ 1 error generated.

Could you cross-check the other occurrences, please?

Axel-Naumann · 2016-01-21T16:47:35Z

This is an awesome patch, @ellert - thanks so much!

I am done reviewing the code and the configure/make build system; I'll ask Pere for a to review of the CMake part. Ideally I'd like to see the fixes to my comments first. Do you think you would be able to help us with those, too?

Cheers, Axel.

pcanal · 2016-10-24T08:03:15Z

A general comment, if possible it would be better if we could avoid the commits "Merge branch 'master' into install-test" if possible.

pcanal · 2016-10-24T08:10:54Z

core/base/inc/TROOT.h

+   static const TString& GetDataDir();
+   static const TString& GetDocDir();
+   static const TString& GetMacroDir();
+   static const TString& GetTutorialsDir();


Unfortunately this is back incompatible (as you noticed by having to add .Data() in the tutorials). So we need to find an alternative name/way to introduce the thread safe version. thanks,

I renamed GetTutorialsDir to GetTutorialDir and added a backward compatible GetTutorialsDir that returns const char*.
The name without the s actually is more consistent since none of the others uses the plural (GetIncludeDir, GetDocDir, GetMacroDir, GetSourceDir and so on - not Includes, Docs, Macros, Sources).

ellert · 2016-11-01T15:03:57Z

I have tried to keep this PR mergeable, but since it touches quite a lot of files it is not uncommon that changes to master make changes to the same files in a way that can not be automatically resolved. The "Merge branch 'master' into install-test" commits are the resolutions of these merge conflicts.

Axel-Naumann · 2017-01-12T14:45:20Z

Sorry Mattias - I believe you're done with my comments, right? So then I'll assign this to Pere for the CMake part.

ellert · 2017-01-16T09:46:01Z

Thanks. I merged master and resolved the conflicts again. This time there were quite a few of them due to the recent changes to the rootcling statge 1 processing.

peremato · 2017-01-17T16:21:57Z

I am testing it now.

peremato · 2017-01-17T20:39:19Z

Hi Mattias. Running with this PR I manage to run all tests successfully without the option gnuinstall. Running with the option I get 227 tests failed out of 1395. Basically all the failures are related to difficulties to build dictionaries or libraries because the ROOT libraries are expected in different place.
I think I would merge the PR since we will not break existing functionality, but we need to complement with some more changes to fix the remaining failures.

ellert · 2017-01-18T12:23:46Z

I always run with the gnuinstall option, and I see no failures. But I don't have as many tests in total as you. For my 6.08.04 build I have (see: https://kojipkgs.fedoraproject.org//packages/root/6.08.04/1.fc25/data/logs/x86_64/build.log):

100% tests passed, 0 tests failed out of 602

This is with a few tests requiring network access disabled

make test 'ARGS=-j16 --output-on-failure -E "test-stressIOPlugins-.*|tutorial-tree-run_h1analysis|tutorial-multicore-imt001_parBranchProcessing|tutorial-multicore-mp103_processSelector|tutorial-multicore-imt101_parTreeProcessing"'

But those few disabled tests do not account for the difference between 602 and 1395.

I disable all the builtin options (except for llvm), and build using the versions of external dependencies that are available in the distribution.

Can you elaborate on what you mean by "because the ROOT libraries are expected in different place"? When running the test suite in the build tree, the libraries are expected to be in the same place independently of the gnuinstall option (i.e. in ${CMAKE_BINARY_DIR}/lib). The location of the built targets in the build tree are supposed to not depend on whether the gnuinstall option is on or off.

peremato · 2017-01-18T14:40:11Z

Are you enabling the 'roottest' tests (with -Droottest=ON)? Because all the failing tests are in this category. Lets take for example roottest-cling-function-refClasses-build, I get this error:

    Start 634: roottest-cling-function-refClasses-build

634: Test command: /Applications/CMake.app/Contents/bin/cmake "--build" "/Users/mato/Development/ROOT/build.master" "--target" "roottest-cling-function-refClasses-compile-macro/fast" "--" "--always-make"
634: Test timeout computed to be: 1500
634: Error in <UnknownClass::FindDynamicLibrary>: libCling[.so | .dll | .dylib | .sl | .dl | .a] does not exist in .:/Users/mato/Development/ROOT/root.prefix/lib/root:/usr/local/lib:/usr/X11R6/lib:/usr/lib:/lib:/lib/x86_64-linux-gnu:/usr/local/lib64:/usr/lib64:/lib64:
634: Fatal in <TROOT::InitInterpreter>: cannot load symbol dlsym(RTLD_DEFAULT, CreateInterpreter): symbol not found
634: make[1]: *** [roottest/cling/function/CMakeFiles/roottest-cling-function-refClasses-compile-macro] Error 1
634: make: *** [roottest-cling-function-refClasses-compile-macro/fast] Error 2
1/1 Test #634: roottest-cling-function-refClasses-build ...***Failed    0.11 sec

0% tests passed, 1 tests failed out of 1

Another type of failures are the following, which involves the location of Clang resources.

    Start 1020: roottest-root-meta-expressiveErrorMessages-libgen-build

1020: Test command: /Applications/CMake.app/Contents/bin/cmake "--build" "/Users/mato/Development/ROOT/build.master" "--target" "roottest-root-meta-expressiveErrorMessages-libgen/fast" "--" "--always-make"
1020: Test timeout computed to be: 1500
1020: Generating expressiveErrorMessages.cxx, expressiveErrorMessages.rootmap
1020: ERROR in cling::CIFactory::createCI():
1020:   resource directory /Users/mato/Development/ROOT/root.prefix/etc/root/cling/lib/clang/3.9.0 not found!
1020: input_line_2:1:10: fatal error: 'cling/Interpreter/RuntimeUniverse.h' file not found
1020: #include "cling/Interpreter/RuntimeUniverse.h"
1020:          ^
...

peremato · 2017-01-20T09:03:08Z

The failing tests where from roottest suit. Two fixes:

Added ENVIRONMENT ROOTIGNOREPREFIX=1 for some additional add_test(...) in roottest sources
Configured two root-config (one to be used from the build tree, the other to be installed in PREFIX)
Merged into master.

ellert added 4 commits January 9, 2016 08:37

Copy headers to include, not ${CMAKE_INSTALL_INCLUDEDIR}

3c38b47

Copy python files to lib, not ${CMAKE_INSTALL_LIBDIR}

2f826ca

gnuinstall should not imply rpath

b01f9e2

Make it possible to run tests when configuring with -Dgnuinstall:BOOL=ON

6f634f9

Axel-Naumann self-assigned this Jan 11, 2016

Merge branch 'master' into install-test

ad6e69c

Axel-Naumann reviewed Jan 19, 2016
View reviewed changes

Axel-Naumann reviewed Jan 21, 2016
View reviewed changes

ellert added 5 commits January 30, 2016 00:48

Merge branch 'master' into install-test

fdbcd5e

Merge branch 'master' into install-test

394129a

Avoid using temporaries

8996a74

Add/Remove .Data()

3525df1

Remove unused #define

9a27f02

Adapt new changes to the test-install fix

16ace21

pcanal reviewed Oct 24, 2016

View reviewed changes

ellert added 4 commits November 1, 2016 11:32

Merge branch 'master' into install-test

4d18a98

Rename GetTutorialsDir to GetTutorialDir

7a38212

Add backward compatible GetTutorialsDir that returns const char*

ca3c122

Fix TString / std:string conversion

3b515e1

ellert added 4 commits November 20, 2016 08:32

Better python

b22fb2e

Resolve font paths in TASImage::DrawText

62ecb6a

Let TASImage::DrawText resolve the font paths

e618f1c

Merge branch 'master' into install-test

ffe846b

ellert mentioned this pull request Dec 6, 2016

Use full paths to images #311

Closed

ellert added 3 commits December 7, 2016 18:56

Merge branch 'master' into install-test

f6279cd

Merge branch 'master' into install-test

de24c6c

Merge branch 'master' into install-test

31c25de

Axel-Naumann assigned peremato and unassigned Axel-Naumann Jan 12, 2017

Merge branch 'master' into install-test

4f8e4a7

Merge branch 'master' into install-test

ac0702c

FonsRademakers merged commit ac0702c into root-project:master Jan 20, 2017

ellert deleted the install-test branch January 20, 2017 09:11

ethereal-space-cadet16 mentioned this pull request May 31, 2022

Accessing pyROOT #10676

Closed

Make it possible to run tests when configuring with -Dgnuinstall:BOOL=ON #130

Make it possible to run tests when configuring with -Dgnuinstall:BOOL=ON #130

Uh oh!

Conversation

ellert commented Jan 9, 2016

Uh oh!

Axel-Naumann commented Jan 11, 2016

Uh oh!

ellert commented Jan 12, 2016

Uh oh!

Axel-Naumann commented Jan 12, 2016

Uh oh!

Axel-Naumann Jan 19, 2016

Choose a reason for hiding this comment

Uh oh!

ellert Feb 4, 2016

Choose a reason for hiding this comment

Uh oh!

Axel-Naumann Feb 4, 2016

Choose a reason for hiding this comment

Uh oh!

rassel1304 commented Jan 20, 2016

Uh oh!

Axel-Naumann Jan 21, 2016

Choose a reason for hiding this comment

Uh oh!

Axel-Naumann commented Jan 21, 2016

Uh oh!

pcanal commented Oct 24, 2016

Uh oh!

pcanal Oct 24, 2016

Choose a reason for hiding this comment

Uh oh!

ellert Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

ellert commented Nov 1, 2016

Uh oh!

Axel-Naumann commented Jan 12, 2017

Uh oh!

ellert commented Jan 16, 2017

Uh oh!

peremato commented Jan 17, 2017

Uh oh!

peremato commented Jan 17, 2017

Uh oh!

ellert commented Jan 18, 2017

Uh oh!

peremato commented Jan 18, 2017

Uh oh!

peremato commented Jan 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants