Cache problems in FindERT about opm-core HOT 7 CLOSED

Just a note: This is beyond my knowledge of CMake. Paging Mr. Resident CMake Expert (i.e., @rolk).

from opm-core.

Do not dig into this as this might be hickup on my system. I will double check and get back.

from opm-core.

I would like to understand why this is needed

This is just a short-circuit: Instead of attempting to do gcc foo.cpp -lecl which will fail if the library is missing, it writes an unsuccessful result to the cache if it sees that the library directory is -NOTFOUND.

I had to use -DCMAKE_LIBRARY_ARCHITECTURE=x86_64-linux-gnu to fix this
...
this might be hickup on my system

I think this may be a symptom of the root cause of the problem. On my Ubuntu Precise (which is also multi-arch), CMAKE_LIBRARY_ARCHITECTURE is already set to x86_64-linux-gnu.

Is there perhaps a "stuck" value of ERT_ROOT in CMakeCache.txt? (If ERT_ROOT is set, then it doesn't search around)

from opm-core.

I cannot reproduce the first issue.

the second problem is a real issue. Everything works fine if I remove the cache setting from FindERT.cmake. Otherwise if the test once failed it will never be repeated.

from opm-core.

Concerning the short circuit: There is a return satement after setting the cache anyway .

For me the current situation is quite surprising:

1. I see that opm-core searches for ERT and install.
2. When I rerun configure include directories and libs are found but the compile test is never run and therefore I am told that the (cached) HAVE_ERT is false and ERT is unusable.

from opm-core.

I see that opm-core searches for ERT and install. When I rerun configure include directories and libs are found but the compile test is never run.

This is a question of policy: CMake want the configure step to be fast, so if you have performed an "expensive" test the result is "cached" and not rerun the next time. You can see this if you run cmake twice in a build tree doing nothing in between. The second output is much less voluminous than the first.

Consider first this minimal project, which performs an "inexpensive" test of checking if a file exists:

cmake_minimum_required (VERSION 2.8)
project (Foo)
find_file (HAVE_HELLO hello.txt PATHS ${CMAKE_CURRENT_BINARY_DIR})
message (STATUS "HAVE_HELLO = ${HAVE_HELLO}")
file (WRITE hello.txt "Hello, World!")

If you run it once, you get a -NOTFOUND path. However, the file gets written after the test, so the next time you run cmake, it will pick it up.

Then we turn to a more "expensive" test involving the compiler; consider the following minimal example (assuming foo.h doesn't exist yet):

cmake_minimum_required (VERSION 2.8)
project (Foo)
enable_language (C)
include (CheckCSourceCompiles)
check_c_source_compiles ("
#include <${CMAKE_CURRENT_BINARY_DIR}/foo.h>
int main (void) { return 0; }
" HAVE_FOO_H)
message (STATUS "HAVE_FOO_H = ${HAVE_FOO_H}")
file (WRITE foo.h "/* Hello, World! */")

Run it once, test fails but the file gets written. Run it twice and the test still fails, even though the file was there! Then rm CMakeCache.txt and you see that it now pick up the file correctly.

The original code tries to be smart: If the library directory isn't found, then don't bother to do the compile but just emulate the result, setting a flag that it failed.

But obviously the code doesn't work as desired, as you describe (when installing the library as a response to a failed configuration). And I believe it should. What can be done?

One problem is that the facts on the ground changed, and I didn't take that into account. Consider this minimal project which is a variation of the one above:

cmake_minimum_required (VERSION 2.8)
project (Foo)
enable_language (C)
include (CheckCSourceCompiles)
set (HAVE_FOO_H)
set (HAVE_FOO_H "${HAVE_FOO_H}" CACHE INTERNAL "Have foo?")
check_c_source_compiles ("
int main (void) { return 0; }
" HAVE_FOO_H)
message (STATUS "HAVE_FOO_H = ${HAVE_FOO_H}")

This will never succeed, despite there being nothing wrong with the program being compiled. The reason is that CMake checks the cache and seeing that it reportedly failed before, doesn't even make a second attempt.

Should we just remove the setting of the flag altogether, then? Well, it actually strike both ways: If you have first got a pass, then it is valid for the duration of the cache. Yet another minimal project to illustrate (remove foo.h before running the first time):

cmake_minimum_required (VERSION 2.8)
project (Foo)
enable_language (C)
include (CheckCSourceCompiles)
file (APPEND foo.h "")
check_c_source_compiles ("
#include <${CMAKE_CURRENT_BINARY_DIR}/foo.h>
int main (void) { return 0; }
" HAVE_FOO_H)
message (STATUS "HAVE_FOO_H = ${HAVE_FOO_H}")
file (WRITE foo.h "#error You shall not pass!")

Clearly the example program is now (explicitly!) invalid, but you can still run cmake as many times as you wish -- the result will always be the same.

The real problem is not setting the flag, but how it is cached. Instead of writing a blank value (i.e. "false") to the cache, it should be unset, to indicate for future versions that we should retry and then set as a regular variable this time to indicate failure here and now.

One final minimal project to experiment with this; to see the effect try first without any foo.h present, then touch it to create a blank file. Then try to remove the file and run again.

cmake_minimum_required (VERSION 2.8)
project (Foo)
enable_language (C)
if (EXISTS "${CMAKE_CURRENT_BINARY_DIR}/foo.h")
  include (CheckCSourceCompiles)
  check_c_source_compiles ("
  #include <${CMAKE_CURRENT_BINARY_DIR}/foo.h>
  int main (void) { return 0; }
  " HAVE_FOO_H)
else (EXISTS "${CMAKE_CURRENT_BINARY_DIR}/foo.h")
  set (HAVE_FOO_H)
  unset (HAVE_FOO_H CACHE)
endif (EXISTS "${CMAKE_CURRENT_BINARY_DIR}/foo.h")
message (STATUS "HAVE_FOO_H = ${HAVE_FOO_H}")

It now correctly figures out when it is necessary to compile, and when this step can be skipped. Of course, if we put some illegal code into the file after it has passed then it won't detect this, but to avoid that we would have to recompile every time (or at least checks the file's modification time).

In conclusion, I believe the correct patch is to replace the two lines that say:

  set (HAVE_ERT "${HAVE_ERT}" CACHE INTERNAL "Did an ERT sample program compile?")

with

   unset (HAVE_ERT CACHE)

from opm-core.

Should be fixed in #291

from opm-core.