Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for select_library_configurations #7

Merged
merged 2 commits into from
Jun 4, 2019
Merged

Add support for select_library_configurations #7

merged 2 commits into from
Jun 4, 2019

Conversation

mloskot
Copy link
Contributor

@mloskot mloskot commented May 31, 2019

@letmaik
Copy link
Collaborator

letmaik commented Jun 1, 2019

Can you describe what this does? I haven't seen that before.

@mloskot
Copy link
Contributor Author

mloskot commented Jun 1, 2019

Sure, but I'mnnit sure what more I could add to the CMake docs whereiit's explained quite well https://cmake.org/cmake/help/latest/module/SelectLibraryConfigurations.html

Let me know what kind of info you're missing

@letmaik
Copy link
Collaborator

letmaik commented Jun 1, 2019

In essence, you want the FindLibRaw module to search for debug variants as well, named rawd and raw_rd, and expose this as new LibRaw_LIBRARY_DEBUG, LibRaw_LIBRARY_RELEASE etc variables, keeping the original ones like LibRaw_LIBRARY more or less as they are but let them prefer the RELEASE variant (if found) over the DEBUG variant.

I'm missing some motivation for this change. The CMake scripts don't build rawd or raw_rd named variants, which makes your FindLibRaw.cmake changes a bit confusing and out of place. Can you explain this a bit more?

@mloskot
Copy link
Contributor Author

mloskot commented Jun 1, 2019

The CMake scripts don't build rawd or raw_rd named variants,
which makes your FindLibRaw.cmake changes a bit confusing and out of place.
Can you explain this a bit more?

I'd argue the statement "CMake scripts don't build rawd or raw_rd" is not quite accurate, because this particular behaviour does not depend on the declared CMake configuration, but on CMake execution. I'll try to explain.

On Windows, optimized and debug builds are 'incompatible' enough and, for optimal workflow, users of a library need both builds. It is common to deploy both, optimized and debug, builds of a library into common {prefix}/{bin,include,lib} location(s). Since it means there is potential for naming clash, CMake offers tools to avoid it, namely CMAKE_DEBUG_POSTFIX.

Please, consider the complete build workflow below which is quite common to Windows + MSVC environment. Here, I'm using Visual Studio 2019 and CMake 3.14, executing all commands in the x64 Native Tools Command Prompt for VS 2019:

D:\> git clone https://github.com/LibRaw/LibRaw.git
D:\> git clone https://github.com/LibRaw/LibRaw-cmake.git
D:\> mkdir LibRaw\cmake
D:\> xcopy LibRaw-cmake\CMakeLists.txt LibRaw\ /Y /F
D:\> xcopy LibRaw-cmake\cmake\* LibRaw\cmake\ /E /Y /F
D:\> set CL=/DWIN32=1
D:\> cd LibRaw
D:\LibRaw> cmake -S . -B _build_vs2019_opt -G Ninja -DCMAKE_INSTALL_PREFIX=%CD%/_install -DCMAKE_BUILD_TYPE=Release
D:\LibRaw> cmake --build _build_vs2019_opt -j 4 --verbose

D:\LibRaw> cmake -S . -B _build_vs2019_dbg -G Ninja -DCMAKE_INSTALL_PREFIX=%CD%/_install -DCMAKE_BUILD_TYPE=Debug -DCMAKE_DEBUG_POSTFIX=d
D:\LibRaw> cmake --build _build_vs2019_dbg -j 4 --verbose
D:\LibRaw> cmake --build _build_vs2019_opt --target install
[0/1] Install the project...
-- Install configuration: "Release"
-- Installing: D:/LibRaw/_install/include/libraw/libraw.h
-- Installing: D:/LibRaw/_install/include/libraw/libraw_alloc.h
-- Installing: D:/LibRaw/_install/include/libraw/libraw_const.h
-- Installing: D:/LibRaw/_install/include/libraw/libraw_datastream.h
-- Installing: D:/LibRaw/_install/include/libraw/libraw_internal.h
-- Installing: D:/LibRaw/_install/include/libraw/libraw_types.h
-- Installing: D:/LibRaw/_install/include/libraw/libraw_version.h
-- Installing: D:/LibRaw/_install/include/libraw/libraw_config.h
-- Installing: D:/LibRaw/_install/lib/raw.lib
-- Installing: D:/LibRaw/_install/bin/raw.dll
-- Installing: D:/LibRaw/_install/lib/raw_r.lib
-- Installing: D:/LibRaw/_install/bin/raw_r.dll
...
D:\LibRaw> cmake --build _build_vs2019_dbg --target install
-- Install configuration: "Debug"
-- Up-to-date: D:/LibRaw/_install/include/libraw/libraw.h
-- Up-to-date: D:/LibRaw/_install/include/libraw/libraw_alloc.h
-- Up-to-date: D:/LibRaw/_install/include/libraw/libraw_const.h
-- Up-to-date: D:/LibRaw/_install/include/libraw/libraw_datastream.h
-- Up-to-date: D:/LibRaw/_install/include/libraw/libraw_internal.h
-- Up-to-date: D:/LibRaw/_install/include/libraw/libraw_types.h
-- Up-to-date: D:/LibRaw/_install/include/libraw/libraw_version.h
-- Installing: D:/LibRaw/_install/include/libraw/libraw_config.h
-- Installing: D:/LibRaw/_install/lib/rawd.lib
-- Installing: D:/LibRaw/_install/bin/rawd.dll
-- Installing: D:/LibRaw/_install/lib/raw_rd.lib
-- Installing: D:/LibRaw/_install/bin/raw_rd.dll
...

This technique is used by package managers which deploy both types of builds, for example, Vcpkg (see libraw/portfile.cmake).

@letmaik
Copy link
Collaborator

letmaik commented Jun 1, 2019

Thanks, that helped. To make this PR complete, why not additionally set CMAKE_DEBUG_POSTFIX if it's not specified? Then there would be a direct relation to the naming in the Find* script. See Brad King's comment at curl/curl#2121 (comment).

@letmaik
Copy link
Collaborator

letmaik commented Jun 1, 2019

Regarding your reference to vcpkg, I was under the impression that vcpkg installs debug libraries in a separate folder tree anyway, which then is correctly selected when using the vcpkg CMake toolchain file. Is that incorrect? If not, how can there be naming conflicts?

@mloskot
Copy link
Contributor Author

mloskot commented Jun 2, 2019

To make this PR complete, why not additionally set CMAKE_DEBUG_POSTFIX if it's not specified?

Good point. Brad's comment make sense. I will update this PR.

Regarding your reference to vcpkg, I was under the impression that vcpkg installs debug libraries
in a separate folder tree anyway (...)

This is correct. I was pointing at Vcpkg as example where it is used, but not necessarily for all the reasons I presented. AFAIS, Vcpkg still seem to follow the CMAKE_DEBUG_POSTFIX convention across the ports.
Possibly, it's to avoid potential problems when DLLs of run-time dependencies are copied as per https://github.com/microsoft/vcpkg/blob/master/docs/users/integration.md

a post-build action for executable projects that will analyze and copy any DLLs you need to the output folder, enabling a seamless F5 experience.

@letmaik
Copy link
Collaborator

letmaik commented Jun 3, 2019

What about this part?

LibRaw-cmake/CMakeLists.txt

Lines 582 to 591 in d4e130a

# Configure and install data file for packaging.
IF(NOT WIN32)
CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/cmake/data/libraw.pc.cmake ${CMAKE_CURRENT_BINARY_DIR}/libraw.pc @ONLY)
INSTALL(FILES ${CMAKE_CURRENT_BINARY_DIR}/libraw.pc DESTINATION lib${LIB_SUFFIX}/pkgconfig)
CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/cmake/data/libraw_r.pc.cmake ${CMAKE_CURRENT_BINARY_DIR}/libraw_r.pc @ONLY)
INSTALL(FILES ${CMAKE_CURRENT_BINARY_DIR}/libraw_r.pc DESTINATION lib${LIB_SUFFIX}/pkgconfig)
CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/cmake/data/libraw.lsm.cmake ${CMAKE_CURRENT_BINARY_DIR}/libraw.lsm)
ENDIF()

Libs: -L${libdir} -lraw

If you have d as suffix then these files won't work anymore if pkgconfig is used. I'm not sure how this is handled in general, i.e. whether there is typically only a single pkgconfig file for release mode, or whether there should be a debug variant as well in that case. Either way, wouldn't it be a breaking change in some cases?

@mloskot
Copy link
Contributor Author

mloskot commented Jun 3, 2019

I was concerned about that and I'd be more comfortable with least intrusive change like this:

IF(WIN32 AND NOT DEFINED CMAKE_DEBUG_POSTFIX)  # or even IF(MSVC ...
    SET(CMAKE_DEBUG_POSTFIX "d")
ENDIF()

I just could not find any OS-specific recommendations regarding use of CMAKE_DEBUG_POSTFIX.

Shall I enable SET(CMAKE_DEBUG_POSTFIX "d") for MSVC or WIN32 only?

@letmaik
Copy link
Collaborator

letmaik commented Jun 4, 2019

OK, let's do it for Windows only to avoid any potential issues. I think WIN32 is fine.

The CMAKE_<CONFIG>_POSTFIX variable is not meant for direct reference
by project code (e.g. explicitly set any properties, library names).
The variable initializes the <CONFIG>_POSTFIX target property.
@mloskot
Copy link
Contributor Author

mloskot commented Jun 4, 2019

I updated the PR setting the postfix for WIN32 only.

@letmaik letmaik merged commit e681f1b into LibRaw:master Jun 4, 2019
@letmaik
Copy link
Collaborator

letmaik commented Jun 4, 2019

Thanks again for this and for your patience! Future PRs always welcome :)

@mloskot mloskot deleted the ml/select_library_configurations branch June 4, 2019 18:17
@mloskot
Copy link
Contributor Author

mloskot commented Jun 4, 2019

I'm happy to help. Thanks for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants