Fix pkg-config lib suffix for cmake debug builds

[mcmtroffaes]

This fixes the generated pkgconfig file in debug builds (for instance, see microsoft/vcpkg#18123): the debug libraries end with a _d postfix but the pc file was missing this postfix.

The configure.ac script does not support debug builds as far as I can tell so I've left that as is.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Matthias C. M. Troffaes (da5ef92)

[lgritz]

Why didn't you just compare to "Debug" which is always what the build type will be for debug?

[mcmtroffaes]

That's a good question. CMAKE_BUILD_TYPE is case insensitive, and a user could set it to debug, Debug, DEBUG, ... See for instance https://cmake.org/pipermail/cmake/2013-June/055177.html

[lgritz]

There's just as much danger from their calling it "Debugging" and not getting what they intended. I think if we're worried about people setting the build type incorrectly, we should check early in the top-level CMakeLists.txt that CMAKE_BUILD_TYPE is set to one of the few choices we support. It seems error prone and overly verbose for every place it's referenced to need to separately handle for the possibility that somebody may have used nonstandard capitalization.

I would advise just checking for "Debug" here, and as a separate PR we should put a check at the top level to make sure it's not set to anything other than "Release" or "Debug".

[mcmtroffaes]

Well, it is officially documented to be case insensitive (see https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html), but I've seen many projects do exactly what you say, so yes, can do.

[waebbl]

I would advise just checking for "Debug" here, and as a separate PR we should put a check at the top level to make sure it's not set to anything other than "Release" or "Debug".

Strictly limiting the possible CMAKE_BUILD_TYPEs could lead to systems not being able to build a package. For example, on Gentoo, we use CMAKE_BUILD_TYPE=Gentoo, which is essentially a release build type, but ensures compiler and linker flags are set appropriately to the users needs and the desired compiler and toolchain is used. Other source based distributions might have a similar way. However, I don't know how binary distributions handle this.

[waebbl]

I don't know what the right fix here is. I think what @waebbl is saying is that somebody else could be using a totally different build type name to indicate a debug build, and that testing CMAKE_BUILD_TYPE here (even in a case-insensitive way) is not a reliable way to know if this is really a debug build or not?

That's actually not what I wanted to say. Althgouth this is possible, I think the chance is quite low. For example we always use the Gentoo build type and don't have a different one to indicate debug builds. What I wanted to say, is to try to avoid restricting a build to only the common four now types. Also if there's a need to add some special flags with a build type, try to append them to the *{C,CXX,F}FLAGS variables, so the users choices don't get overriden.
Many projects I've seen use a construct like in https://github.com/Kitware/VTK/blob/master/CMake/vtkInitializeBuildType.cmake which works great.

As for the issue at hand, the patch looks like a good way to solve it, but I don't know how debug builds are usually handled by pkg-config.

[mcmtroffaes]

I don't know how debug builds are usually handled by pkg-config.

Debug builds have separate .pc file from release builds, to handle differences in naming schemes. In vcpkg for instance, the debug .pc files are installed in /debug/lib/pkgconfig (along with the libraries in /debug/lib) whilst the release .pc files are found in /lib/pkgconfig (along with the libraries in /lib).

AFAIK the only reliable way to do this is to check case insensitively for a match with Debug,

That's my understanding too. If that's desired, I can revert ee02bdb (which I added to replace the case insensitive match with a case sensitive string equality, as I understood this was requested).

and also check if the user has requested a specific naming override, i.e. an additional optional flag such as OPENEXR_BUILD_TYPE to indicate that a Debug configuration is expected.

That's a good idea.

Please let me know how you wish to proceed.

[dg0yt]

Instead of generally discussing possible build types, I would suggest to focus on the particular case:

• openexr sets CMAKE_DEBUG_POSTFIX
• "This variable is a special case of the more-general CMAKE_<CONFIG>_POSTFIX variable for the DEBUG configuration."
• As long as openexr sets CMAKE_DEBUG_POSTFIX, the DEBUG build type creates incorrect pc files. This is what this PR covers.

The cmake documentation implies that the user might also set CMAKE_<CONFIG>_POSTFIX for other build types. If you want correct pc files also in these cases, it needs more work. But it seems easy to handle them all without even knowing them in advance.

[mcmtroffaes]

Ah, yes, that's actually much cleaner, if possible. Is something like ${CMAKE_${uppercase_CMAKE_BUILD_TYPE}_POSTFIX} guaranteed to cover all bases (defaulting to RELEASE if CMAKE_BUILD_TYPE is empty)?

[mcmtroffaes]

... I'm actually not sure if an empty build type implies release. The project could set a default CMAKE_BUILD_TYPE (and tell the user) if none is specified.

[mcmtroffaes]

I've implemented a few more changes (especially, the suggestion from @dg0yt, and I copied this code from master to set a default build type also in the RB-2.5 branch).

However, for some reason the commits aren't showing up here on the PR yet; github must have some delay? Diff can be seen here: https://github.com/mcmtroffaes/openexr/compare/RB-2.5..feature/fix-pkgconf-lib-suffix

[dg0yt]

It is not mandatory to set a default build type, and setting it now changes bevhaviour. Some users might rely on this behaviour because they don't want the flags which come with the build type.

[mcmtroffaes]

Sure, easy to revert if undesired.

[waebbl]

A default build type is already being set in RB-2.5 too: https://github.com/mcmtroffaes/openexr/blob/RB-2.5/OpenEXR/config/OpenEXRSetup.cmake#L98

[mcmtroffaes]

Oh, thanks for pointing that out. Not sure how I missed that! I've done a forced push retaining just the relevant commit.

[cary-ilm]

Is there a consensus on the right approach here? And @mcmtroffaes, can you confirm that the commits are showing up properly? Everything seems to be in order, but I'd like to confirm before merging. Thanks!

[mcmtroffaes]

Thanks @cary-ilm for following this up. The commits are now showing correctly. If there's anything else needed do let me know.

[cary-ilm]

LGTM

[cary-ilm]

Thanks, we'll make a 2.5.7 release shortly.

Presumably the same fix needs to go into 3.0 and master, and in Imath.

[mcmtroffaes]

Concerning master & Imath, yes, I think so. I'll have a look at it.

Thanks so much for merging, and also everyone for the useful and educational discussion.

[cary-ilm]

Something's still not quite right. The .so's have the proper _d.so suffix, but we're still installing a libImath.so that is a broken link to libImath-2_5.so (without the _d). Same with all the libs: Imath, Half, Iex, IlmThread, IlmImf, IlmImfUtil.

For the Debug build, would it be better to fix the link or eliminate this file altogether?

[mcmtroffaes]

Ah yes, I see it too. I can have a look to see where this one is originating from. IMO it's clearest either to install the symbolic links (correctly) for all configs, or not to install them at all. This said, I imagine someone downstream might rely on them? So for compatibility, my feeling is that it's best to correct the links. However, I'm not a heavy user of openexr, and I'd be happy to hear other arguments.

[mcmtroffaes]

Something else that occurs to me is that someone might want to install the release and debug libraries into the same folder. So perhaps libOpenEXR_d.so should link to libOpenEXR-2_5_d.so and so on for the others? In that way the release library link (libOpenEXR.so) does not conflict with the debug library link.

[dg0yt]

can have a look to see where this one is originating from.

openexr/cmake/OpenEXRLibraryDefine.cmake

Lines 64 to 71 in 52bfbd4

	if(BUILD_SHARED_LIBS AND (NOT "${OPENEXR_LIB_SUFFIX}" STREQUAL "") AND NOT WIN32)
	set(verlibname ${CMAKE_SHARED_LIBRARY_PREFIX}${libname}${OPENEXR_LIB_SUFFIX}${CMAKE_SHARED_LIBRARY_SUFFIX})
	set(baselibname ${CMAKE_SHARED_LIBRARY_PREFIX}${libname}${CMAKE_SHARED_LIBRARY_SUFFIX})
	install(CODE "execute_process(COMMAND ${CMAKE_COMMAND} -E chdir \"\$ENV\{DESTDIR\}${CMAKE_INSTALL_FULL_LIBDIR}\" ${CMAKE_COMMAND} -E create_symlink ${verlibname} ${baselibname})")
	install(CODE "message(\"-- Creating symlink in ${CMAKE_INSTALL_FULL_DIR} ${baselibname} -> ${verlibname}\")")
	set(verlibname)
	set(baselibname)
	endif()

[mcmtroffaes]

Yup, there's another one in cmake/LibraryDefine.cmake

[meshula]

So perhaps libOpenEXR_d.so should link to libOpenEXR-2_5_d.so and so on for the others?

I think that's a crucial requirement on Windows. On Linux, debug and release build can coexist as they both use the same libc. On Windows though, MSVCRT and MSVCRTD don't play well together - memory allocated in one will corrupt the heap if released in the other.