Skip to content

change default installation directory for cmake files: #1306

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

Merged
merged 1 commit into from
Feb 20, 2020

Conversation

LocutusOfBorg
Copy link
Contributor

now the default becomes "{CMAKE_INSTALL_LIBDIR}/cmake/cpprestsdk"

This is a respin of #845

now the default becomes "{CMAKE_INSTALL_LIBDIR}/cmake/cpprestsdk"
@BillyONeal
Copy link
Member

Is there anything substantial that has changed since #845 which would warrant a change here? I'm concerned that this is a breaking change.

@c72578
Copy link
Contributor

c72578 commented Feb 19, 2020

For the Fedora packages [1] we currently use -DCPPREST_EXPORT_DIR=cmake/cpprestsdk.
This matches the directory suggested by PR #1306 here. So, if the default value for CPPREST_EXPORT_DIR gets updated according to this PR, we could omit -DCPPREST_EXPORT_DIR=cmake/cpprestsdk in the future.

[1] https://src.fedoraproject.org/rpms/cpprest/blob/53defcf55ec147f7ac9b5b96a56e804bf67767e8/f/cpprest.spec#_61

@BillyONeal
Copy link
Member

@c72578 I get that, but what about anyone who is not currently passing that switch and are relying on the current layout? I know absolutely nothing about this pkg-config support stuff, so I would not want to merge this unless there's something substantial that changed since #845 where my predecessor (who understands that area much better than I) rejected an identical change.

@LocutusOfBorg
Copy link
Contributor Author

@BillyONeal #845 was a mistake on my side, because it was forcing people to add CMAKE_INSTALL_LIBDIR in the export directory, unconditionally.
"DESTINATION ${CMAKE_INSTALL_LIBDIR}/${CPPREST_EXPORT_DIR}"

even if this was right, it was a wrong approach, removing the configurability of the full path from maintainer perspective.

Now, since the standard cmake approach is to put stuff in cmake/$SOURCE, this pull request changes the default to that, and BTW I can say that Fedora, Debian, Yocto are already using that standard PATH, so with this PR merged we will just drop the need to explictly change the default and carry a patch.

@LocutusOfBorg
Copy link
Contributor Author

I'm pretty sure nobody will complain about that change, because the current default can't be found via cmake.

to test this:

cat ../CMakeLists.txt 
cmake_minimum_required(VERSION 3.10)
find_package(cpprestsdk REQUIRED)

cmake ..

and then

//The directory containing a CMake configuration file for cpprestsdk.
cpprestsdk_DIR:PATH=/usr/lib/x86_64-linux-gnu/cmake/cpprestsdk

@BillyONeal
Copy link
Member

OK, sounds reasonable. Thanks for your contribution! (And thanks @ras0219-msft for double checking)

@BillyONeal BillyONeal merged commit feb0755 into microsoft:master Feb 20, 2020
@LocutusOfBorg
Copy link
Contributor Author

Btw please tag me if anybody complains and I'll make sure to find a way that fits everybody's needs

@LocutusOfBorg LocutusOfBorg deleted the change-default-cmake branch July 15, 2020 14:38
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.

3 participants