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

Enhanced CMake install features #112

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Enhanced CMake install features #112

wants to merge 7 commits into from

Conversation

IMFTool
Copy link

@IMFTool IMFTool commented Aug 31, 2017

Features:

  • Different targets for Release and Debug (the latter with appendix "_d"
  • Creates regxmllibcConfig.cmake and regxmllibcTargets.cmake files that allow other applications to discover regxmllib using find_package(regxmllibc)
  • GUI Option for BUILD_SHARED (default: off)
  • Installs entire *.h tree
  • Standard install targets for MacOS and Windows (Linux untested)
  • Tested with MacOS 10.10 and Win 7 / VS 2015

@palemieux
Copy link
Contributor

@IMFTool Thanks! Will review. Any chance you can rebase against the HEAD of master?

project (regxmllibc)
option(BUILD_SHARED_LIB Bool OFF)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmake already defines BUILD_SHARED_LIBS

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduced for pure convenience when using a CMake GUI, from [1]:
This variable is often added to projects as an option() so that each user of a project can decide if they want to build the project using shared or static libraries

[1] https://cmake.org/cmake/help/v3.5/variable/BUILD_SHARED_LIBS.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok... but it says BUILD_SHARED_LIB and not BUILD_SHARED_LIBS.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I forgot. It's a locally defined name.

if(BUILD_SHARED_LIB)
set(LIB_TYPE SHARED)
if(WIN32 AND NOT CYGWIN)
set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS TRUE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be set for all platforms... or does it cause problems in cygwin?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's being set specifically for WIN32 DLLs. I did not test with cygwin and have it therefore explicitly excluded.

file(RELATIVE_PATH rel_source ${CMAKE_CURRENT_SOURCE_DIR}/src/main/cpp/com/ ${source})
get_filename_component(source_path ${rel_source} DIRECTORY)
string(REPLACE "\\" "/" source_path_msvc "${source_path}")
source_group("${source_path_msvc}" FILES "${source}")
endforeach()

if(WIN32 AND NOT CYGWIN)
set(DEF_INSTALL_CMAKE_DIR CMake)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be set by the user?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resulting INSTALL_CMAKE_DIR can be edited by the user after configuring the build and before generating the build files. DEF_INSTALL_CMAKE_DIR holds useful default locations.

EXPORT ${PROJECT_NAME}Targets DESTINATION ${INSTALL_LIB_DIR}
)

foreach ( file ${INC_FILES} )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this portion of the cmake file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looping through all include files and installing them with their respective full relative path. lines 81 and 82 are debug output and should be removed, though.

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