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

Fix default installation path for cmake files #845

Closed
wants to merge 1 commit into from

Conversation

LocutusOfBorg
Copy link
Contributor

After #737 got merged, I think there is still something missing.

Fixup the merge to make cpprest being detected by pkg-config.

@LocutusOfBorg LocutusOfBorg changed the title Fix default installation path Fix default installation path for cmake files Aug 18, 2018
@ras0219-msft
Copy link
Contributor

ras0219-msft commented Aug 20, 2018

Thank you again for the PR, however as mentioned in #737 [1] we do not want to change the default install locations at this time.

You can achieve the same effect as this PR by passing a parameter at configure time:

-DCPPREST_EXPORT_DIR:STRING=lib/cmake

[1] #737 (comment)

@LocutusOfBorg
Copy link
Contributor Author

Hello @ras0219-msft unfortunately this doesn't work, because "lib" in Debian is "lib/" and not just lib
forcing "lib" breaks our multiarch implementation.
Why do you want to keep such location?

@ras0219-msft
Copy link
Contributor

Because it would be a breaking change and I don't understand the value. Especially, what do we say to someone else who comes by and asks to change the defaults later? What makes these defaults "the true best defaults that are better than all others?"

Instead, can you explain in detail what the problem is with passing -DCPPREST_EXPORT_DIR:STRING=lib/cmake and let's find a solution that allows you to get the folders you want regardless of what our defaults are.

I don't know what you mean by "lib in Debian is lib/"; do you mean you need to add a trailing slash?

-DCPPREST_EXPORT_DIR:STRING=lib/cmake/

@LocutusOfBorg
Copy link
Contributor Author

ok, let me explain.
It is wrong to use pkg-config and search for pkgconfig files under /usr/lib/cpprestsdk

all the cmake files (at least in Debian and Fedora I think, as well as others), should go in

/lib/cmake
/lib/cmake/subdirectory
/usr/lib/cmake
/usr/lib/cmake/subdirectory
/usr/lib/<triplet>/cmake/
/usr/lib/<triplet>/cmake/subdirectory

I don't understand how cmake can possibly use "/usr/lib/cpprestsdk/*.cmake" as location, without having to manually hint it.

In Debian, we prefer to use
"lib//cmake" to let different architectures being installed at the same time, e.g. amd64 and i386 cpprestsdk can cohexist.
I can't easily pass "triplet" to the build system, without overcomplicating how I generate the package.

The same applies to yocto, where I'm currently maintaining the package

I did an additional commit, does it look better to you?

@LocutusOfBorg
Copy link
Contributor Author

LocutusOfBorg commented Aug 20, 2018

If you reopen the issue, I think the PR will update with the new commit

commit 38bd9cbcecbbd128c6ffb63f24f503faf37f3cec (HEAD -> fix-install, mio/fix-install)
Author: Gianfranco Costamagna <[email protected]>
Date:   Mon Aug 20 19:26:00 2018 +0200

    Fixup previous commit.
    This makes the install location not correct for Debian and similar linux distro, but upstream don't plan to change "lib/cpprestsdk" location,
    so at least we can override the location with -DCPPREST_EXPORT_DIR=cmake and keep the CMAKE_INSTALL_LIBDIR multiarch location

diff --git a/Release/CMakeLists.txt b/Release/CMakeLists.txt
index d20b4b07..9f6bbc41 100644
--- a/Release/CMakeLists.txt
+++ b/Release/CMakeLists.txt
@@ -18,7 +18,7 @@ enable_testing()
 set(WERROR ON CACHE BOOL "Treat Warnings as Errors.")
 set(CPPREST_EXCLUDE_WEBSOCKETS OFF CACHE BOOL "Exclude websockets functionality.")
 set(CPPREST_EXCLUDE_COMPRESSION OFF CACHE BOOL "Exclude compression functionality.")
-set(CPPREST_EXPORT_DIR cmake CACHE STRING "Directory to install CMake config files.")
+set(CPPREST_EXPORT_DIR cpprestsdk CACHE STRING "Directory to install CMake config files.")
 set(CPPREST_INSTALL_HEADERS ON CACHE BOOL "Install header files.")
 set(CPPREST_INSTALL ON CACHE BOOL "Add install commands.")
 

@ras0219-msft
Copy link
Contributor

The fundamental objection I have is twofold:

  1. This would be a breaking change that I would like to avoid before 3.x
  2. I do not want to change the defaults unless the new defaults are clearly superior to any other possible defaults, such that I could easily close any future PRs to change the defaults again.

I don't understand how cmake can possibly use "/usr/lib/cpprestsdk/*.cmake" as location, without having to manually hint it.

See the CMake documentation (https://cmake.org/cmake/help/latest/command/find_package.html) and note that all paths are checked on all OS's, even if they aren't considered "native" (Also note that /usr is a default prefix on unix).

<prefix>/(lib/<arch>|lib*|share)/<name>*/ (U)

In Debian, we prefer to use "lib//cmake"

Great, so what's the problem with

-DCPPREST_EXPORT_DIR:STRING=lib//cmake

?

Or am I misunderstanding and you're saying that you use arch-specific lib directories (lib & lib64) and you need those handled? In that case, you would need to pass in a different value for the correct arch; I don't know the exact logic, but it should be pretty straightforward:

-DCPPREST_EXPORT_DIR=lib64/cmake

@LocutusOfBorg
Copy link
Contributor Author

  1. the current updated PR doesn't change anymore the default behavior
  2. you are right, the current default is ok wrt cmake, I don't know why debian puts everything into a single cmake directory.

Anyway the updated PR won't change the current behavior just add /lib//cpprestsdk instead of /lib/cpprestsdk when GNUInstallDirs finds an appropriate system.

BTW the problem with multiarch is that we need different locations to have amd64 and i386 cohexist, and this is why we put stuff under /lib/

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