Merge lp:~zorba-coders/zorba/zorba-tests-when-installed-fix into lp:zorba

[Ghislain Fourny]

This fix should bring the failing tests down to 7% in case Zorba is installed on the machine.

This fix should bring the failing tests down to 7% in case Zorba is installed on the machine. It swaps the lib path resolution strategy from top-down to bottom-up through the static context, so that the lib path provided on the command line (in the build directory for a test) takes priority over the root static context's builtin module paths (the first one being in the install directory). Some of the remaining failing tests are probably still failing because the test executables also need to be adapted to override the lib path through their command line (so far this is done for rbkt tests only).

Normally, this fix should not lead to any regression in the case Zorba is not installed.

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/zorba-tests-when-installed-fix-2012-06-07T14-49-11.936Z/log.html

[Zorba Build Bot (zorba-buildbot)]

Validation queue job zorba-tests-when-installed-fix-2012-06-07T14-49-11.936Z is finished. The final status was:

All tests succeeded!

[Zorba Build Bot (zorba-buildbot)]

Voting does not meet specified criteria. Required: Approve > 1, Disapprove < 1, Needs Fixing < 1, Pending < 1. Got: 1 Pending.

[Chris Hillery (ceejatec)]

The two changes to add LIB_PATH to --module-path for testdriver are good and reasonable.

As Matthias said in email, I'm not sure about the major change here of putting command-line paths before built-in paths. It was originally done as it is in order to prevent exactly what you're apparently trying to do - put your own versions of Zorba core modules in place of the ones included in Zorba. We did it as a (fairly weak) security measure, as well as to ensure as much as possible that Zorba's core functionality was knowable and unbroken.

What exactly is the use-case here? Why do you need to replace core modules? (If you're not replacing core modules, I'm not sure what problem this change would solve.)

[Ghislain Fourny (gislenius)]

Hi Chris, thanks for your comment. The problem I am trying to solve is the following (occurring on Lion).

If Zorba is not installed ("make install" not executed) the tests (run from the build directory) pass, since the LIB_PATH taken into consideration is the third builtin path (the one in the build directory) - the first two builtin paths (both in the install dir) being nonexistent. The libraries in the build directory's LIB_PATH point back to the zorba libraries in the build directory, so everything is consistent.

If Zorba is installed ("make install" executed) many of the tests (run from the build directory) no longer pass, since the first two builtin paths (in the install directory) now exist and take over. One issue is that they do not point back to the zorba library in the build directory, but to the zorba library in the install directory. This means that two identical, albeit physically different zorba libraries are actually used side-by-side (the one in the build directory, the one in the install directory), so that two stores are instantiated, leading to a failing assertion (store2 == store).

In projects that build in top of zorba and that need "make install" for running their tests, a manual post-install dependency override is required to get all tests to pass.

Having the command-line --module-path take precedence over builtin paths would make sure that tests can specify their LIB_PATH.

I do understand the concern you raise, i.e., it might not be wise to allow this lib path override in real life usage. Would this make sense: have two lib path vectors in each static context node, one used before that of the parent, one used after the parent - it means that the lookup would be bottom-up-and-back-down. The former lib path (which takes priority over the builtin paths) could be only exposed to tests, and the latter one could be the one exposed officially. Would it make sense to you?

[Chris Hillery (ceejatec)]

Aha. Yes, I was aware that this was a problem on MacOS; in fact, bug 867139 discusses it. (It's a problem on Linux too, but the symptom is much less serious.)

I think before committing any questionable changes to core Zorba, I would like to explore two alternatives:

1. If you could arrange for your testing process to rm -rf build/*_PATH after you do "make install", this problem would go away. A quick "make" will restore the contents of those directories. It's not a beautiful solution, but it actually is the "right one" because it guarantees that you are testing your installed image in exactly the way it will be run on a deployed machine, without a build/ directory. (Ideally you should blow away the build/ directory in its entirety to be really sure you're testing what you ship, but I understand that has ... drawbacks.)

2. Alternately, take a look at the description of bug 867139 and then do some research. If you can give me a reliable way on MacOS to determine the filesystem location of libzorba_simplestore.dylib (or, of the zorba executable itself, but that is less preferable), then we can implement the relocatable install feature that is already working on Windows. That would fix this problem, make Zorba a little bit more efficient, and even allow people to move Zorba after installation.

I think both of these solutions are better than this proposal, because they actually address the underlying problem instead of dusting it under the carpet. Hopefully at least one of them can be implemented quickly.

[Ghislain Fourny (gislenius)]

Hi Chris, thanks a lot for your comments! I also think it is worth exploring a long-term solution.

I am not sure suggestion 1. would work, since the problem comes from the installation paths that take precedence - removing the build paths would probably not change this (at least in the case of running tests).

The bug report description however (suggestion 2.) makes a lot of sense to me and I agree with you that an ideal solution would include runtime path discovery. I will give it some thoughts.

Thanks again for your input!

[Chris Hillery (ceejatec)]

> I am not sure suggestion 1. would work, since the problem comes from the
> installation paths that take precedence - removing the build paths would
> probably not change this (at least in the case of running tests).

Ah, I either misread or mis-understood your problem. I assumed that if you were doing a "make install", then any tests you ran later would be running against the *installed* Zorba (why else do an install?). But, then, you're right - that situation should work anyway since the install locations are first on the path.

Since you're continuing to test the build directory, though, could you just re-arrange your tests so that the "make install" step is done AFTER all other testing?

Unmerged revisions

10869. By Ghislain Fourny on 2012-06-07

Added module path for internal rbkt tests.

10868. By Ghislain Fourny on 2012-06-07

Merged trunk back.

10867. By Ghislain Fourny on 2012-06-07

This fix should allow test execution even if Zorba is installed.

=== modified file 'cmake_modules/ZorbaModule.cmake'
--- cmake_modules/ZorbaModule.cmake	2012-05-30 00:33:21 +0000
+++ cmake_modules/ZorbaModule.cmake	2012-06-07 14:46:29 +0000
@@ -806,7 +806,7 @@
       ADD_TEST(${TESTNAME} "${Zorba_TESTDRIVER}"
 	"--rbkt-src" "${TEST_DIR}"
 	"--module-path"
-	"${CMAKE_BINARY_DIR}/URI_PATH/${PATH_SEP}${DEPENDENCY_MODULE_PATH}"
+	"${CMAKE_BINARY_DIR}/URI_PATH/${PATH_SEP}${CMAKE_BINARY_DIR}/LIB_PATH/${PATH_SEP}${DEPENDENCY_MODULE_PATH}"
 	"${TESTFILE}")
 
       # On non-Windows, call EXPECTED_FAILURE() for known crashes

=== modified file 'src/context/static_context.cpp'
--- src/context/static_context.cpp	2012-05-18 22:01:56 +0000
+++ src/context/static_context.cpp	2012-06-07 14:46:29 +0000
@@ -1722,12 +1722,12 @@
 ********************************************************************************/
 void static_context::get_full_lib_path(std::vector<zstring>& path) const
 {
+  get_lib_path(path);
+
   if (theParent != NULL)
   {
     theParent->get_full_lib_path(path);
   }
-
-  get_lib_path(path);
 }
 
 

=== modified file 'test/rbkt/Queries/CMakeLists.txt'
--- test/rbkt/Queries/CMakeLists.txt	2012-05-09 20:29:48 +0000
+++ test/rbkt/Queries/CMakeLists.txt	2012-06-07 14:46:29 +0000
@@ -128,6 +128,12 @@
   FIND_PACKAGE(CURL)
 ENDIF(ZORBA_SUPPRESS_CURL)
 
+IF(WIN32)
+  SET(PATH_SEP ",")
+ELSE(WIN32)
+  SET(PATH_SEP ":")
+ENDIF(WIN32)
+
 
 FOREACH(TESTFILE ${TESTFILES})
   # All testfile paths end in .xq or .xqx. Strip that part off to form
@@ -195,7 +201,11 @@
 
   IF(NOT SKIP_TEST)
 
-    ZORBA_ADD_TEST("${TESTNAME}" testdriver ${TESTFILE})
+    ZORBA_ADD_TEST("${TESTNAME}"
+                   testdriver
+                   "--module-path"
+                   "${CMAKE_BINARY_DIR}/URI_PATH/${PATH_SEP}${CMAKE_BINARY_DIR}/LIB_PATH/"
+                   "${TESTFILE}")
 
     MATH(EXPR TESTCOUNTER ${TESTCOUNTER}+1)
     MATH(EXPR TESTMOD "${TESTCOUNTER}/1000")