Missing Version in ISAACConfig.cmake

ax3l commented 7 years ago

The current CMake config file has the following bug(s).

[x] strictly speaking, it should in installed to lib/cmake/ISAAC/ (uppercase), since that is the <name> known to CMake as the package name. Luckily, this dir <name> seems to be checked case-insensitive and is still found at least in the Ubuntu I tested it with. More details, Docs.
```
In all cases the <name> is treated as case-insensitive and corresponds to any of
the names specified
```
[x] a major bug is, that this file defines no version, leading to:

CMake Warning at src/picongpu/CMakeLists.txt:272 (find_package):
  Could not find a configuration file for package "ISAAC" that is compatible
  with requested version "0.1.0".

  The following configuration files were considered but not accepted:

    [...]/gcc-4.9.4/isaac-1.2.0-[...]/lib/cmake/isaac/ISAACConfig.cmake, version: unknown
-- Could NOT find ISAAC - set ISAAC_DIR or check your CMAKE_PREFIX_PATH

when one searches for it via

find_package(ISAAC 0.1.0 CONFIG)

(using QUIET just gives the last line)

[x] We should also rebuild that file so it can be used without QUIET and still is not too verbose. Otherwise hints as the one above will not be forwarded to the user.
[x] I am not sure your ISAACConfig.cmake even works when ISAAC_DIR is not set (which is long deprecated CMake style). You might want to test if it works with only CMAKE_PREFIX_PATH pointing to your ISAAC_DIR and use functions like find_path to find a typical isaac file and derive the include path from it.

theZiz commented 7 years ago

The first bug is fixed: https://github.com/ComputationalRadiationPhysics/isaac/commit/ab25ae2a794fe65a73dbe5a60de3bd406afcf530

theZiz commented 7 years ago

The second bug I cannot reproduce. For me the version checks works well for find_package(ISAAC 1.1.0 REQUIRED) and find_package(ISAAC 1.1.0 CONFIG). If I change the requested version to 1.2.0, it fails. Even if I change it to 0.1.0 (which should be changed in picongpu btw.) it works for me.

ax3l commented 7 years ago

for the second bug, can you test if it still works after installing ISAAC (lib) with make install? that's my install case.

theZiz commented 7 years ago

Good point

theZiz commented 7 years ago

Haha! I don't install the ISAACVersion file :hankey:

theZiz commented 7 years ago

Fixed bug 2: https://github.com/ComputationalRadiationPhysics/isaac/commit/f63ad29c050cddf6d11fbc5511689ceab9230ec0

ax3l commented 7 years ago

cool, now you just forgot to test it to realize that isaac_version.hpp will not be installed in lib/cmake/ISAAC/isaac/isaac_version.hpp but in include/isaac/isaac_version.hpp.

CMake Error at [...]/lib/cmake/ISAAC/ISAACConfigVersion.cmake:1 (file):
  file STRINGS file
  "[...]/lib/cmake/ISAAC/isaac/isaac_version.hpp"
  cannot be read.
Call Stack (most recent call first):
  src/picongpu/CMakeLists.txt:272 (find_package)

theZiz commented 7 years ago

Well... Next week I have a lot of time during talks to fix this cough

ax3l commented 7 years ago

but, but it's right here (read: it's wrong here) :D

theZiz commented 7 years ago

Well, both cases have to work: Installing it and using it without installation. I will do this on monday. I have a very important meeting with Jean-Luc Picard now ;)

ax3l commented 7 years ago

yes, and it's no problem to already have the two cmake files in the same structure in-source, too.

(btw, assuming include and lib will be in the exact same root is not always true when packed and shipped in various distributions :blush: )

ax3l commented 7 years ago

I am confused, you are going to meet: picard-facepalm ?

theZiz commented 7 years ago

I will think about this, when a debian maintainer asks me to ship ISAAC as package. XD

theZiz commented 7 years ago

Yeah, friday evening. Star Trek Time!

ax3l commented 7 years ago

for that reason it's clever to detect the version directly in your ISAACConfig.cmake and pass it to FIND_PACKAGE_HANDLE_STANDARD_ARGS (and moving the ISAACConfig.cmake to lib/lib/cmake/ISAAC in-source, too).

The second mode is more powerful and also supports version checking:

FIND_PACKAGE_HANDLE_STANDARD_ARGS(NAME [FOUND_VAR <resultVar>]
                                       [REQUIRED_VARS <var1>...<varN>]
                                       [VERSION_VAR   <versionvar>]
                                       [HANDLE_COMPONENTS]
                                       [CONFIG_MODE]
                                       [FAIL_MESSAGE "Custom failure message"] )

ax3l commented 7 years ago

talking of lib/lib/, I just want to rub in your face again, that not splitting lib, server and client is messy ;P

theZiz commented 7 years ago

I will have a look for this. I did not write the version checking myself, so I thought a second versionconfig file was the way to go. ;)

ax3l commented 7 years ago

yep, no prob. nothing that can't wait for Monday. Cpt Picard over and out.

P.S.: I am not sure your ISAACConfig.cmake even works when ISAAC_DIR is not set. You might want to test if it works with only CMAKE_PREFIX_PATH pointing to your ISAAC_DIR and use functions like find_path to find a typical isaac file and derive the include path from it.

ax3l commented 7 years ago

@theZiz any updates that I missed?

theZiz commented 7 years ago

Hm, why did I not comment here? I hopefully fixed the issue in this Commit: https://github.com/ComputationalRadiationPhysics/isaac/commit/d8006fb22dd591f15a7455e3bf39e0ff8547855f I support now both: Directly using the ISAAC directoy, but also installing the library. The path of the installation folder is written to the ISAACConfigVersion.cmake while installing it.

I tested it with PIConGPU on Hypnos with different paths, installed and directly in the source folder and it seemed to work. :wink:

theZiz commented 7 years ago

I will test the ISAAC_DIR and CMAKE_PREFIX_PATH issue right now :smiley:

theZiz commented 7 years ago

Okay, I gave it a test, CMAKE_PREFIX_PATH works very well for ISAAC and also the JSON library libjansson. Only IceT needs a CMAKE_PREFIX_PATH set to $ROOT\lib instead of $ROOT as its cmake config file is not located in $ROOT\lib\cmake\$PACKAGE_NAME, which is the default way: https://cmake.org/cmake/help/v3.0/manual/cmake-packages.7.html#package-configuration-file

ax3l commented 7 years ago

I tested it with PIConGPU on Hypnos with different paths, installed and directly in the source folder and it seemed to work. :wink:

Sounds great! Did you also try VERSION support again, because your FIND_PACKAGE_HANDLE_STANDARD_ARGS still does not handle the version?

ax3l commented 7 years ago

Only IceT needs a CMAKE_PREFIX_PATH set to $ROOT\lib instead of $ROOT [...]

That's weird, <prefix>/lib is one of the paths CMake should look for the config: https://cmake.org/cmake/help/v3.0/command/find_package.html

ax3l commented 7 years ago

We should also rebuild that file so it can be used without QUIET and still is not too verbose. Otherwise hints as the one above will not be forwarded to the user.

How verbose is the script after the changes without QUIET? Ideally, I would like to avoid setting it since it suppresses error messages as the one posted initially that e.g. a version was not found but ISAAC was, but last time we checked the script was very very verbose.

theZiz commented 7 years ago

Yes, I did test the VERSION. I have an own version cmake config file for this.

theZiz commented 7 years ago

I will check for the verbosity later this day.

theZiz commented 7 years ago

I tested it, I don't see a difference except if ISAAC cannot be found, it states:

CMake Warning at src/picongpu/CMakeLists.txt:272 (find_package):
  Could not find a package configuration file provided by "ISAAC" (requested
  version 0.1.0) with any of the following names:

    ISAACConfig.cmake
    isaac-config.cmake

  Add the installation prefix of "ISAAC" to CMAKE_PREFIX_PATH or set
  "ISAAC_DIR" to a directory containing one of the above files.  If "ISAAC"
  provides a separate development package or SDK, be sure it has been
  installed.

-- Could NOT find ISAAC - set ISAAC_DIR or check your CMAKE_PREFIX_PATH

But I doubt I can do much about this as ISAAC is not found at this stage. ;)

ax3l commented 7 years ago

And when it's found? I remember it was quite verbose when it was found about its dependencies.

theZiz commented 7 years ago

Yes, you are right, it states this:

CMake Warning at /home/am6120/testing/isaac/lib/ISAACConfig.cmake:78 (find_package):
  Could not find a package configuration file provided by "Jansson" with any
  of the following names:

    JanssonConfig.cmake
    jansson-config.cmake

  Add the installation prefix of "Jansson" to CMAKE_PREFIX_PATH or set
  "Jansson_DIR" to a directory containing one of the above files.  If
  "Jansson" provides a separate development package or SDK, be sure it has
  been installed.
Call Stack (most recent call first):
  src/picongpu/CMakeLists.txt:272 (find_package)

CMake Warning at /home/am6120/testing/isaac/lib/ISAACConfig.cmake:93 (find_package):
  Could not find a package configuration file provided by "IceT" with any of
  the following names:

    IceTConfig.cmake
    icet-config.cmake

  Add the installation prefix of "IceT" to CMAKE_PREFIX_PATH or set
  "IceT_DIR" to a directory containing one of the above files.  If "IceT"
  provides a separate development package or SDK, be sure it has been
  installed.
Call Stack (most recent call first):
  src/picongpu/CMakeLists.txt:272 (find_package)

I could set the search for these (or all dependencies) package to QUIET Then cmake would only state:

Could NOT find ISAAC - set ISAAC_DIR or check your CMAKE_PREFIX_PATH

Well, this is not correct. I could of course define a "ISAAC_NOT_FOUND_REASON" variable, which I could print here. Do you think, this may a good idea?

ax3l commented 7 years ago

That sounds great, yes! You could search for the indirect dependencies QUIET and do a message() yourself listing the missing indirect dependencies before returning to find_package.

theZiz commented 7 years ago

Okay, will do so tomorrow. :shipit:

theZiz commented 7 years ago

Okay, ISAAC does now set the variable ISAAC_DEPENDENCY_HINTS with all missing hints, which I use in the PIConGPU CMakeLists.txt, it then looks like this:

-- ISAAC was found, but has the following missing dependencies:
--   libJansson
--   IceT

That's all. ;) If the depenencies could be found, it still states:

-- Found ISAAC: /some/directory/somewhere/lib/cmake/ISAAC

ax3l commented 7 years ago

but, but - we still can't use it without QUIET which means: in case everything is found but the ISAAC version is outdated the user will not get the information that the version is causing the issue.

Can you just directly print the hints instead of forwarding them? And then silence the find_package of Jansson and IceT via QUIET? See this comment: https://github.com/ComputationalRadiationPhysics/picongpu/pull/1894#issuecomment-281745005

ax3l commented 7 years ago

is anything done and a new version released?

theZiz commented 7 years ago

Yes

ComputationalRadiationPhysics / isaac

Missing Version in ISAACConfig.cmake #59