+1 Kinetic only. This could potentially break a lot of packages and for most older packages it's at least working to some extent. And it doesn't force a fork since the correct code is fully compatible.

I do not think that this patch would solve the problem of https://github.com/ros-simulation/gazebo_ros_pkgs/pull/473/files/a9ac9b93699e72e2d83765b01bed33b467845a01#r70289189, but maybe I am wrong and I have not tested it.

The thing is that cmake "searches for a file called <name>Config.cmake or <lower-case-name>-config.cmake for each name specified" and sets the <package>_FOUND variable using the same capitalisation as the caller of find_package(<package> ...), independent of where the package configuration has been found and what capitalisation is used internally [1]. I assume that a proper cmake package should either provide <name>Config.cmake or respect the value of the PACKAGE_FIND_NAME variable.

As a consequence, package names listed in catkin_package(DEPENDS package) need to have the exact same capitalisation as in their cmake configuration file and the fact that package_FOUND is set does not help to determine which variant was used. We already had related problems with the Eigen3 cmake config (ros/cmake_modules#35) which lead to the cmake snippet in the jade Migration guide and I think some configs even set variables with mixed case.

[1] https://cmake.org/cmake/help/v3.0/command/find_package.html

@meyerj I don't think your expectation is accurate. The case of the package name passed to find_package does not influence the variables the CMake config file (or module) sets. The variable names are hard coded.

The case of the passed name only affects which config files are being considered. <name>Config.cmake must have the same case as the search. <lower-case-name>-config.cmake is always being considered.

catkin packages always generate config files with the first naming. Simply because it make it very clear that the case you use for the find_package call is also the same as for the variables being set. But third-party packages might use any naming and therefore you can't rely on the case of the variables based on the passed name to find_package.

This patch is not intending to solve the problem in the CMake file of the gazebo_plugins package. But with this patch it should fail the configure step for the package and point out that the dependency intended to be exported (gazebo) is not correct (since there is no variable gazebo_FOUND).

+1 for Kinetic.

@meyerj I don't think this is intended to solve the problem there, just to fix the error condition that should have caught the problem at a more reasonable place.

"searches for a file called <name>Config.cmake or <lower-case-name>-config.cmake for each name specified"

As I read the documentation, that only happens when you use the NAMES option with find_package. Otherwise I think it uses the exact case given as the <package> name.

This patch is not intending to solve the problem in the CMake file of the gazebo_plugins package. But with this patch it should fail the configure step for the package and point out that the dependency intended to be exported (gazebo) is not correct (since there is no variable gazebo_FOUND).

I understood that this patch was not supposed to fix gazebo_plugins, but it would also not trigger the FATAL_ERROR message because gazebo_FOUND is set to 1 after the earlier successful call of find_package(gazebo REQUIRED).

As I read the documentation, that only happens when you use the NAMES option with find_package. Otherwise I think it uses the exact case given as the name.

The cited phrase of the cmake documentation is not only for the NAMES attribute. The full paragraph:

"Config mode attempts to locate a configuration file provided by the package to be found. A cache entry called <package>_DIR is created to hold the directory containing the file. By default the command searches for a package with the name <package>. If the NAMES option is given the names following it are used instead of <package>. The command searches for a file called <name>Config.cmake or <lower-case-name>-config.cmake for each name specified. A replacement set of possible configuration file names may be given using the CONFIGS option. The search procedure is specified below. Once found, the configuration file is read and processed by CMake. Since the file is provided by the package it already knows the location of package contents. The full path to the configuration file is stored in the cmake variable <package>_CONFIG."

cmake sets the <package>_FOUND variable with the exact case given in find_package, independent of which file and which NAMES variant has been found.

I created a quick test to verify this. The result (Ubuntu 14.04 with cmake 2.8.12.2) is:

-- The C compiler identification is GNU 4.8.4
-- The CXX compiler identification is GNU 4.8.4
-- Check for working C compiler: /usr/lib/ccache/cc
-- Check for working C compiler: /usr/lib/ccache/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: /usr/lib/ccache/c++
-- Check for working CXX compiler: /usr/lib/ccache/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- [CMakeLists.txt] Calling `find_package(FOO REQUIRED)`
-- [foo-config.cmake] Found package 'foo' in '/home/johannes/catkin-813-test/cmake':
-- [foo-config.cmake]  - FOO_INCLUDE_DIRS=/path/to/package/foo/include
-- [CMakeLists.txt] After `find_package(FOO REQUIRED)`:
-- [CMakeLists.txt]  - FOO_FOUND=1
-- [CMakeLists.txt]  - FOO_DIR=/home/johannes/catkin-813-test/cmake
-- [CMakeLists.txt]  - FOO_CONFIG=/home/johannes/catkin-813-test/cmake/foo-config.cmake
-- [CMakeLists.txt]  - FOO_CONSIDERED_CONFIGS=/home/johannes/catkin-813-test/cmake/foo-config.cmake
-- [CMakeLists.txt]  - FOO_INCLUDE_DIRS=/path/to/package/foo/include
-- [CMakeLists.txt]  - foo_FOUND is not defined.
-- [CMakeLists.txt]  - foo_DIR is not defined.
-- [CMakeLists.txt]  - foo_CONFIG is not defined.
-- [CMakeLists.txt]  - foo_CONSIDERED_CONFIGS is not defined.
-- [CMakeLists.txt]  - foo_INCLUDE_DIRS is not defined.
-- 
-- [CMakeLists.txt] Calling `find_package(foo REQUIRED)`
-- [foo-config.cmake] Found package 'foo' in '/home/johannes/catkin-813-test/cmake':
-- [foo-config.cmake]  - FOO_INCLUDE_DIRS=/path/to/package/foo/include
-- [CMakeLists.txt] After `find_package(foo REQUIRED)`:
-- [CMakeLists.txt]  - FOO_FOUND is not defined.
-- [CMakeLists.txt]  - FOO_DIR=/home/johannes/catkin-813-test/cmake
-- [CMakeLists.txt]  - FOO_CONFIG is not defined.
-- [CMakeLists.txt]  - FOO_CONSIDERED_CONFIGS is not defined.
-- [CMakeLists.txt]  - FOO_INCLUDE_DIRS=/path/to/package/foo/include
-- [CMakeLists.txt]  - foo_FOUND=1
-- [CMakeLists.txt]  - foo_DIR=/home/johannes/catkin-813-test/cmake
-- [CMakeLists.txt]  - foo_CONFIG=/home/johannes/catkin-813-test/cmake/foo-config.cmake
-- [CMakeLists.txt]  - foo_CONSIDERED_CONFIGS=/home/johannes/catkin-813-test/cmake/foo-config.cmake
-- [CMakeLists.txt]  - foo_INCLUDE_DIRS is not defined.
-- 
-- Configuring done
-- Generating done
-- Build files have been written to: /home/johannes/catkin-813-test

As you can see the <package>_FOUND and other variables automatically set by cmake are using the exact case from the find_package() command with the same config script, but the script itself only sets the FOO_INCLUDE_DIRS variable. FOO_DIR has been cached between the two calls and is therefore not affected by the function scoping.

As you point out we have to differentiate which variables we are talking about. <package>_FOUND might be set by find_package itself and not by the config file. All other variable like <package>_INCLUDE_DIRS are hard coded in the config file and therefore not related to the case of the argument.

Since the function catkin_package only takes the package names as arguments it must derive the variable names from the argument. And therefore the case of the argument must match the case of the various variables being set by the config file.

To avoid the inconsistency of having some variables have one case and some other a different case catkin packages us the case sensitive naming scheme to ensure consistency. For packages which use inconsistent case (which includes allowing to find the package with different cases) there is no way to derive the variable names based on the argument (since they might use any case). I don't think catkin can address this inconsistency.

The command searches for a file called <name>Config.cmake or <lower-case-name>-config.cmake for each name specified.

In my opinion, the use of the term name (which I bolded in my quote) is ambiguous in the documentation. Based on your example, I think you're right it does look at the given <package name> as well as the names given in the NAMES option, and then tries to find files matching the pattern using all of those. So I was mistaken about that.

@dirk-thomas I agree with @meyerj that this will not cause the error to be thrown for the scenario in the linked ticket. As he said, gazebo_FOUND will be set, even if the include directories are still GAZEBO_INCLUDE_DIRS, for example. So you get a false positive, where catkin thinks it has found a package properly but that it doesn't have any _INCLUDE_DIRS, etc. But that's not the case here. It's that it appears to be found, but the variables do not match the prefix for the _FOUND flag.

@meyerj acknowledging what you're pointing out about the this pr not causing the error to be raised for gazebo_plugins when it probably should, I'd also agree with @dirk-thomas that there's not a good way for us to detect and report this issue within catkin, since we cannot distinguish between packages which have differently prefixed variables (e.g. gazebo_FOUND and GAZEBO_INCLUDE_DIRS) from packages which lack those variables (e.g. foo_FOUND but foo_INCLUDE_DIRS is undefined and there is no equivalent).

I added a note to the docblock clarifying the importance of the case: 1e4ec4c

If the _FOUND flag is set but none of the expected variables is being set it might be good to print a warning: 822fd8f What do you think?

Thanks Dirk, this is exactly what I imagined as a possible patch in the original comment.

Your first commit, df54f49, is useless because the added expression NOT DEFINED ${depend_name}_FOUND implies NOT ${${depend_name}_FOUND} and therefore the second expression of the OR clause is sufficient, like in the original version, right?

I don't think the first part of the condition is redundant. Please see the following example. The second condition is false while the first one is true:

cmake_minimum_required(3.5)

project(foo)

if(NOT DEFINED bar_FOUND)
  message("this condition is true")
endif()

if(NOT ${bar_FOUND})
else()
  message("but this condition is not")
endif()

I don't think the first part of the condition is redundant. Please see the following example. The second condition is false while the first one is true:

Your counter-example is correct. Apparently there is a difference between IF(NOT ${bar_FOUND}) (the if(<constant>) case) and if(NOT bar_FOUND) (the if(<variable>) case) [1]. The second expression evaluates to true if bar_FOUND is not defined, while the first one is false:

cmake_minimum_required(VERSION 2.8)

project(foo)

if(NOT DEFINED bar_FOUND)
  message("`NOT DEFINED bar_FOUND` is true")
else()
  message("`NOT DEFINED bar_FOUND` is false")
endif()

if(${bar_FOUND})
  message("`\${bar_FOUND}` is true")
else()
  message("`\${bar_FOUND}` is false")
endif()

if(bar_FOUND)
  message("`bar_FOUND` is true")
else()
  message("`bar_FOUND` is false")
endif()

if(NOT ${bar_FOUND})
  message("`NOT \${bar_FOUND}` is true")
else()
  message("`NOT \${bar_FOUND}` is false")
endif()

if(NOT bar_FOUND)
  message("`NOT bar_FOUND` is true")
else()
  message("`NOT bar_FOUND` is false")
endif()

outputs

`NOT DEFINED bar_FOUND` is true
`${bar_FOUND}` is false
`bar_FOUND` is false
`NOT ${bar_FOUND}` is false
`NOT bar_FOUND` is true

So the check in catkin_package() could be simply written as

if(NOT ${depend_name}_FOUND)

[1] https://cmake.org/cmake/help/v3.0/command/if.html

Good point. That is definitely easier to read: 03bf92c