Skip to content

build: introduce ENABLE_DATE_INSTALL to allow user opt-out of install - #752

Merged
HowardHinnant merged 2 commits into
HowardHinnant:masterfrom
OlivierLDff:enable-install
Sep 28, 2024
Merged

HowardHinnant merged 2 commits into
HowardHinnant:masterfrom
OlivierLDff:enable-install

Conversation

@OlivierLDff

Copy link
Copy Markdown
Contributor

When using date as a PRIVATE dependency, it is not required to install it. This flag give user using date with add_subdirectory the opportunity to disable install target Default behavior is conserved since ENABLE_DATE_INSTALL is ON

fix #751

When using `date` as a PRIVATE dependency, it is not required to install it. This flag give user using `date` with add_subdirectory the opportunity to disable install target Default behavior is conserved since ENABLE_DATE_INSTALL is ON
Comment thread CMakeLists.txt
Comment on lines +43 to +45
if( ENABLE_DATE_INSTALL )
include( GNUInstallDirs )
endif( )

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make sense to move this include() in the installation section?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No because we want CMAKE_INSTALL_INCLUDIR to be defined for target_include_directories

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right, but the variable is not currently used in target_include_directories(); I just saw #753 though, and they seem to complement each other. Thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes sorry for not making it clearer, I split bug fix and feature in different pr to make review easier/faster

@gerhardol gerhardol left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Works as intended, just copy&paste to fix.
I was planning to create a patch like this myself.

Comment thread CMakeLists.txt Outdated
Co-authored-by: Gerhard Olsson <6248932+gerhardol@users.noreply.github.com>
@basilgello

Copy link
Copy Markdown
Contributor

LGTM @HowardHinnant

@HowardHinnant
HowardHinnant merged commit ab1dc3a into HowardHinnant:master Sep 28, 2024
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.

CMake: Allow user to opt out of install

5 participants