Bug 828467 - CMake test suite fails when postgresql-devel is installed
CMake test suite fails when postgresql-devel is installed
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: cmake (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Orion Poplawski
Fedora Extras Quality Assurance
http://public.kitware.com/Bug/view.ph...
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-04 14:50 EDT by Petr Machata
Modified: 2015-05-04 21:37 EDT (History)
9 users (show)

See Also:
Fixed In Version: cmake-2.8.8-5.fc18
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-07-05 14:17:25 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
A fix for FindPostgreSQL.cmake (1.41 KB, patch)
2012-06-04 14:50 EDT, Petr Machata
no flags Details | Diff
A fix for FindPostgreSQL.cmake (5.53 KB, patch)
2012-07-04 17:16 EDT, Petr Machata
no flags Details | Diff

  None (edit)
Description Petr Machata 2012-06-04 14:50:52 EDT
Created attachment 589245 [details]
A fix for FindPostgreSQL.cmake

Description of problem:
This is a fedora leg of a bug reported filed against RHEL, bug 825186.  The problem is in CMakeOnly.AllFindModules, which fails for FindPostgreSQL module.  (This finding is courtesy of Gregor Jasny, whom I CC.)

This failure is caused by PostgreSQL header files having a different layout.  Apparently, due to multiarch conflicts, /usr/include/pg_config.h is now a dispatcher to a number of pg_config_$arch.h header files that contain what used to be pg_config.h.  I'm not sure whether this is an upstream arrangement or a Fedora-specific patch.  I'm CCing Tom Lane, who might actually know about this.

The fix that I present simply checks for pg_config_$arch.h first, and falls back on pg_config.h if that is not found.

Version-Release number of selected component (if applicable):
cmake-2.8.8-4

How reproducible:
Always.

Steps to Reproduce:
1. install postgresql-devel
2. in CMake build directory, issue bin/ctest -R CMakeOnly.AllFindModules
3. observe failure
  
Actual results:
The following tests FAILED:
	196 - CMakeOnly.AllFindModules (Failed)

Expected results:
The following tests passed:
	CMakeOnly.AllFindModules
Comment 1 Tom Lane 2012-06-04 14:56:57 EDT
(In reply to comment #0)
> This failure is caused by PostgreSQL header files having a different layout.
> Apparently, due to multiarch conflicts, /usr/include/pg_config.h is now a
> dispatcher to a number of pg_config_$arch.h header files that contain what
> used to be pg_config.h.

Um, yeah, it's been that way in RHEL and Fedora since about 2005 ...
Comment 2 Petr Machata 2012-06-05 07:05:11 EDT
I checked that neither Debian sid (the package is called libpq-dev), nor openSUSE 10.3 do this.  I'm not sure it makes sense to upstream this patch.  Maybe it should compile a program that outputs the version to stdout, so that we don't depend on parsing arbitrary C preprocessor expressions.  This might be upstreamable and would work across distros.  I'll contact upstream about their opinions.
Comment 3 Tom Lane 2012-06-05 08:58:18 EDT
Not sure why you're finding this quite so astonishing.  AFAIK this is the standard method for dealing with build-time-generated include files that contain architecture dependencies.  In the rather small set of packages that I maintain, there are three others that use the same trick; and a quick poke around /usr/include on my Fedora box finds a few more.  If CMake can't cope with this then postgresql is just the tip of the iceberg.
Comment 4 Rex Dieter 2012-06-05 09:14:29 EDT
Tom's right of course.

Imo, cmake checks that grep header files are particularly fragile, and should be avoided if at all possible.  In this case, trying to grok PG_VERSION.

I wonder if we could get away with parsing the output from `pg_config --version` instead (or something else that's reasonably simple and safer)
Comment 5 Tom Lane 2012-06-05 09:42:21 EDT
Or maybe feed

#include <pg_config.h>
PG_VERSION

to the C preprocessor, and look at the output of that?
Comment 6 Petr Machata 2012-06-05 11:09:18 EDT
(In reply to comment #3)
> Not sure why you're finding this quite so astonishing.

I'm not.  I point out that others don't do this merely to illustrate that this is RHEL and Fedora specific, and upstream therefore may be reluctant to accept the patch.

> If CMake can't cope with this then postgresql is just the tip of the iceberg.

It can, as witnessed by patch attached to comment 0.

(In reply to comment #5)
> Or maybe feed
> 
> #include <pg_config.h>
> PG_VERSION
> 
> to the C preprocessor, and look at the output of that?

Yes, this is roughly what I had in mind in comment 2, except I'd probably make it a full binary that just outputs the string and exits, so that we don't have to deal with all the directives that the cpp produces.  This seems to be the most portable solution.
Comment 7 Rex Dieter 2012-06-05 11:29:22 EDT
Re: the last comment

'make it a full binary tha tjust outputs the string and exits' => or just parse output of already-existing 'pg_config --version' as suggested in comment #4  (no extra compiling required)
Comment 8 Tom Lane 2012-06-05 11:39:24 EDT
Well, the issue with either pg_config or a special binary is whether you care about the possibility of the header files not matching that executable.  I'm not clear on what the goal of this test is at all.  Is it to know whether postgres is installed (and for that why don't you just ask rpm)?  Or is it to know whether you can compile stuff against the headers?  If the latter, probing pg_config seems a couple of steps removed from what you really want to test.
Comment 9 Rex Dieter 2012-06-05 11:46:29 EDT
via /usr/share/cmake/Modules/FindPostgreSQL.cmake

# - Find the PostgreSQL installation.
# In Windows, we make the assumption that, if the PostgreSQL files are installed, the default directory
# will be C:\Program Files\PostgreSQL.
#
# This module defines
#  PostgreSQL_LIBRARIES - the PostgreSQL libraries needed for linking
#  PostgreSQL_INCLUDE_DIRS - the directories of the PostgreSQL headers
#  PostgreSQL_VERSION_STRING - the version of PostgreSQL found (since CMake 2.8.8)


This is all the information that currently needs to be queried
Comment 10 Tom Lane 2012-06-05 12:29:48 EDT
(In reply to comment #9)
> # This module defines
> #  PostgreSQL_LIBRARIES - the PostgreSQL libraries needed for linking
> #  PostgreSQL_INCLUDE_DIRS - the directories of the PostgreSQL headers
> #  PostgreSQL_VERSION_STRING - the version of PostgreSQL found (since CMake
> 2.8.8) 
> 
> This is all the information that currently needs to be queried

In that case I think you can get it all just by asking pg_config, and that would probably be the most reasonable way to go about it.  The main downside I can think of is pg_config not being available --- but I don't know of any distros where it's not included in either the base package or the -devel package, so that seems probably not a case to worry about.
Comment 11 Petr Machata 2012-07-04 17:16:18 EDT
Created attachment 596289 [details]
A fix for FindPostgreSQL.cmake

This fetches all necessary values from pg_config.  This required more changes in the code, and frankly I have no idea whether it works on Windows.
Comment 12 Orion Poplawski 2012-07-05 13:34:10 EDT
Filed upstream: http://public.kitware.com/Bug/view.php?id=13378
Comment 13 Orion Poplawski 2012-07-05 14:04:34 EDT
One problem -

 This fails the cmake build test checks with:

25: module: /builddir/build/BUILD/cmake-2.8.8/Tests/FindModulesExecuteAll/../../Modules/FindPostgreSQL.cmake
25: CMake Error at /builddir/build/BUILD/cmake-2.8.8/Modules/FindPostgreSQL.cmake:114 (message):
25:   Couldn't determine PostgreSQL configuration.
25: Call Stack (most recent call first):
25:   /builddir/build/BUILD/cmake-2.8.8/Modules/FindPostgreSQL.cmake:130 (fail_if)
25:   CMakeLists.txt:16 (include)

I think we need to change SEND_ERROR to WARNING or similar.
Comment 14 Orion Poplawski 2012-07-05 14:17:25 EDT
Building with that change.
Comment 15 Petr Machata 2012-07-09 03:55:57 EDT
(In reply to comment #14)
> Building with that change.

Thanks, apparently I didn't test the case that PostgreSQL is not installed.
Comment 16 Orion Poplawski 2012-10-02 17:56:33 EDT
Last upstream comment:

I think a middle-ground approach is to run pg_config if it is available and otherwise fall back to parsing the header. Any multiarch install should provide pg_config and any non-multiarch install (e.g. on Windows) should not have a forwarding header. 

Petr - could you produce such a patch against the current cmake git?

Note You need to log in before you can comment on or make changes to this bug.