frmts/pdf: Use correct POPPLER_INCLUDE_DIR

[gdt]

rather than the not-set POPPLER_INCLUDE_DIRS. Resolves failure to
build with poppler on NetBSD.

What does this PR do?

This PR fixesan incorrect use of cmake variables that resulted in not using the poppler include directory. Before, the build failed on NetBSD (where poppler is in /usr/pkg, which is not in the default include path).

What are related issues/pull requests?

There are no related issues.

Tasklist

• It would be nice to have a test case, if we add them for the build system, but github does not support CI runners for other than a few systems. (One could place poppler in a different prefix to test, removing it from /usr.)
• Add documentation
• Review
• Adjust for comments
• All CI builds and checks have passed

Environment

Failure observed and fix verified on NetBSD 9 amd64 with prerequisites from pkgsrc. However, the problem can be seen by inspection as the variable used is never set.

• OS:
  NetBSD 9

• Compiler:
  gcc 7.4.0

[rouault]

a bit strange that it makes a difference because POPPLER_INCLUDE_DIRS and POPPLER_INCLUDE_DIR should be aliases with https://github.com/OSGeo/gdal/blob/master/cmake/modules/packages/FindPoppler.cmake#L138

[gdt]

Interesting; I didn't see that when trying to figure this out. I searched in the cmake cache file, where I found POPPLER_INCLUDE_DIR but not POPPLER_INCLUDE_DIRS. I'm certainly open to a different fix. egrep returns

./build/CMakeCache.txt:POPPLER_INCLUDE_DIR:PATH=/usr/pkg/include/poppler

and I do see FindPoppler.cmake as you point out. I t hink the problem might be that POPPLER_FOUND isn't getting set (not in cache file). But in that case, I would thing the pdf driver should be disabled (I don't have the other pdf prereqs).

[gdt]

$ egrep POPPLER build/CMakeCache.txt 
//Set ON to use POPPLER
GDAL_USE_POPPLER:BOOL=ON
POPPLER_INCLUDE_DIR:PATH=/usr/pkg/include/poppler
POPPLER_LIBRARY:FILEPATH=/usr/pkg/lib/libpoppler.so
//ADVANCED property for variable: POPPLER_INCLUDE_DIR
POPPLER_INCLUDE_DIR-ADVANCED:INTERNAL=1
//ADVANCED property for variable: POPPLER_LIBRARY
POPPLER_LIBRARY-ADVANCED:INTERNAL=1

[rouault]

POPPLER_INCLUDE_DIRS is not a cache variable, just a "volatile" one, so it is expected you don't see it in CMakeCache.txt. So are you sure your patch is needed after all ?

I t hink the problem might be that POPPLER_FOUND isn't getting set (not in cache file). But in that case, I would thing the pdf driver should be disabled (I don't have the other pdf prereqs).

The PDF driver can work in write-only mode without Poppler (or one of the other backends needed for read functionality)

[gdt]

OK on pdf and poppler not being the same thing, but without HAVE_POPPLER we should not be building things in a way that requires poppler includes.

I am sure that I got a build failure. I can do this again and capture more info.

How do I trace the "volatile" variables?
Why is it ok to make some of an interdependent set cached and some volatile? That seems likely to lead to consistency issues (but I don't understand the details of cmake here.)

[rouault]

How do I trace the "volatile" variables?

you can add "message(WARNING ${THE_VAR})" in any CMake executed file

Why is it ok to make some of an interdependent set cached and some volatile? That seems likely to lead to consistency issues (but I don't understand the details of cmake here.)

I don't know. That is apparently the idiomatic way of writing FindXXXX.cmake files. e.g https://github.com/OSGeo/gdal/blob/master/cmake/modules/3.12/FindEXPAT.cmake#L71 that comes from modules packaged in Cmake >= 3.12 (imported here and used when CMake < 3.12)

[dg0yt]

I think this should be fixed in the Find module. Recommended pattern:
<Pkg>_INCLUDE_DIR is an input variable (for the user).
<Pkg>_INCLUDE_DIRS is the output variable (for the consuming project).
(And <Pkg> spelled as in FindPkg.cmake.)

Reference:
https://cmake.org/cmake/help/latest/manual/cmake-developer.7.html#standard-variable-names

[rouault]

I think this should be fixed in the Find module.

renaming FindPoppler.cmake to FindPOPPLER.cmake and keeping the use of POPPLER_INCLUDE_DIRS ?

[dg0yt]

I see _INCLUDE_DIRS as a strict requirement.

I am more relaxed about accepting upper case also for FindPoppler.cmake. I don't think we will see a FindPoppler.cmake from CMake. (But an exported config from Poppler would be nice.)
The coincidence is that I just stumbled over Poppler using an early version of FindFontconfig.cmake, before Kitware/CMake@a3a1e69. Now I need extra polyfill to make Poppler use the static usage requirements vcpkg would offer through proper Fontconfig_... variables.

[gdt]

At this point I am having trouble reproducing my original problem, and I am 98% convinced my change is wrong. It seems like we are having useful discussion about the best approach so I won't hit close just yet.

[jmckenna]

For the record, even for building poppler it's a mix-match of "dir" and "dirs" and "library" and "libraries", there is unfortunately no 'standard' for this and projects make their own decisions. It's best to become very familiar with the /FindXXX.cmake files inside the project's repository and inside CMake's installed directory (on Windows this is a hidden directory such as C:\Program Files (x86)\CMake\share\cmake-3.21\Modules\.

[dg0yt]

I'm not against getting familiar with the tools I use. But here it is a waste of productivity if there is diversity without benefit.

there is unfortunately no 'standard' for this

There is https://cmake.org/cmake/help/latest/manual/cmake-developer.7.html#standard-variable-names. At least since CMake 3.0.

[jmckenna]

I anticipated the fast correction, but the reality is that in the FOSS4G stack it is a total mix-match of variable names currently. +1 for standards.

[rouault]

ok closing that PR. I'll do the FindPoppler.cmake to FindPOPPLER.cmake renaming

[rouault]

I'll do the FindPoppler.cmake to FindPOPPLER.cmake renaming

actually no. Keeping FindPoppler.cmake but using Poppler_XXXX variables: #5165

[gdt]

Thanks - will rebase onto master post #5165 and re-examine.