CMake fixes + CMake search for OpenSSL (macOS) #1383

Clovel · 2020-04-05T13:59:25Z

Homebrew sometimes appends the version of a package to it' directory name. Thus, one could have /usr/local/Cellar/openssl@1.1/ instead of /usr/local/Cellar/openssl/, or even both. I fixed the search for OPENSSL_ROOT_DIR and limited it's contents to one path only.
Added the <package>-config-version.in.cmake file configuration for CMake find_package usage.
Corrected condition in <package>-config.in.cmake for Boost usage. There was a @CPPREST_USES_BOOST@ AND OFF condition, which is always false. The result was that the Boost libraries and includes were not included in the users CMake project.
Use CMAKE_INSTALL_INCLUDEDIR to install headers. cpprestsdk already uses CMAKE_INSTALL_LIBDIR & CMAKE_INSTALL_BINDIR

msftclas · 2020-04-05T13:59:38Z

All CLA requirements met.

Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>

…ig.in.cmake Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>

Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>

garethsb · 2020-04-07T04:29:00Z

Corrected condition in <package>-config.in.cmake for Boost usage. There was a @CPPREST_USES_BOOST@ AND OFF condition, which is always false. The result was that the Boost libraries and includes were not included in the users CMake project.

See this commit and the comment upon it... fc1f692

Clovel · 2020-04-07T11:34:22Z

Corrected condition in <package>-config.in.cmake for Boost usage. There was a @CPPREST_USES_BOOST@ AND OFF condition, which is always false. The result was that the Boost libraries and includes were not included in the users CMake project.

See this commit and the comment upon it... fc1f692

Comments on a lone commits will be lost in the mass of the project. I still commented.

People need this to use find_package(cpprestsdk). If this stays in master, we must change the CMake instructions to add Boost requirements.

And

And anyway :

If this causes problems with versions of CMake before 3.9, then add that as a condition instead.

If cpprestsdk requires Boost, then we must keep Boost as a CMake requirement. If there are issues, then the project must be fixed.
This repair only hides the problem. And the cpprestsdk instructions are to up to date to take this into account. A work around must be described for users of this project.

Clovel · 2020-04-07T11:38:00Z

I don't have time to look into the whole issue. I tried to use cpprestsdk for one of my projects. If this repository's project doesn't work as intended, I won't use it. CMake is a widely used build tool. A project like cpprestsdk should have a working CMake package mechanism.

It should also have a GitHub Action for macOS.

garethsb · 2020-04-07T12:02:53Z

I agree with you, I commented because in the time I invested in reviewing your changes I found that (lost) background.

Clovel · 2020-04-07T12:10:09Z

I agree with you, I commented because in the time I invested in reviewing your changes I found that (lost) background.

Somebody needs to find what the cause of the issue is, so that this can be patched.

Thanks for researching the background of this change.

garethsb · 2020-04-08T19:09:20Z

Added the <package>-config-version.in.cmake file configuration for CMake find_package usage.

I've been missing having the version info available for a long time. The find_package docs seem to indicate there are more variables we should/could set. I wonder if we can use write_basic_package_version_file to do it?

Clovel · 2020-04-08T20:36:45Z

I think the result is the same. My solution is inspired by the files I found on my computer for the Qt CMake Version files. I took a look at other projects that implement this the way I did.

If you believe there is another, better way to do this, I’ll take a look.

However, the link you gave seems to describe a relatively similar method to generate the configuration file.

garethsb · 2020-04-09T07:15:48Z

I think the result is the same.

Ah yes, sorry, I realise your code must ultimately be derived from same root as current CMake config version file template for COMPATIBILITY AnyNewerVersion - https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/BasicConfigVersion-AnyNewerVersion.cmake.in

I'm not sure on C++ REST SDK backward compatibility policy between major/minor releases but maybe SameMajorVersion is appropriate for it?

Clovel · 2020-04-09T11:33:09Z

I'm not sure on C++ REST SDK backward compatibility policy between major/minor releases but maybe SameMajorVersion is appropriate for it?

Maybe. I'm not familiar enough with the project to know, sorry.

c72578 · 2020-04-09T12:42:13Z

COMPATIBILITY AnyNewerVersion is often preferred in upstream projects and causes less (sometimes needless) downstream issues. The primary goal is usually to just provide the version for cmake and not to restrict things. Downstream projects, which use cpprestsdk can still implement a version check, if they need to.

garethsb · 2020-04-22T08:11:10Z

I agree with you, I commented because in the time I invested in reviewing your changes I found that (lost) background.

Somebody needs to find what the cause of the issue is, so that this can be patched.

Thanks for researching the background of this change.

See #593 - the issue is the use of COMPONENTS in find_dependency which wasn't supported before CMake 3.9 (released 2017). I agree it would be better to bump minimum required version from 3.1 (released 2014) to >=3.9 and delete the AND OFF.

@Clovel, how do you think about bumping cmake_minimum_required to 3.9 in this PR? It should be done in Release/CMakeLists.txt of course, but preferably also (root) CMakeLists.txt, README.md (example) and there's also Build_iOS/CMakeLists.txt.

👍

Clovel · 2020-04-22T12:10:05Z

CMake 3.9 is a rather recent but well used version. I personally have used more recent version on older projects with success. I think we could use this without problem. I’ll revise my PR to add these changes.

…

Le 22 avr. 2020 à 10:11, Gareth Sylvester-Bradley ***@***.***> a écrit : I agree with you, I commented because in the time I invested in reviewing your changes I found that (lost) background. Somebody needs to find what the cause of the issue is, so that this can be patched. Thanks for researching the background of this change. See #593 - the issue is the use of COMPONENTS in find_dependency which wasn't supported before CMake 3.9. I agree it would be better to bump minimum required version to <=3.9 and delete the AND OFF. @Clovel, how do you think about bumping cmake_minimum_required to 3.9 in this PR? It should be done in Release/CMakeLists.txt of course, but preferably also (root) CMakeLists.txt, README.md (example) and there's also Build_iOS/CMakeLists.txt. 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

Clovel · 2020-04-22T14:21:44Z

@Clovel, how do you think about bumping cmake_minimum_required to 3.9 in this PR? It should be done in Release/CMakeLists.txt of course, but preferably also (root) CMakeLists.txt, README.md (example) and there's also Build_iOS/CMakeLists.txt.

Done.

garethsb · 2020-04-22T17:04:32Z

Great. What do you think about updating the cmake_minimum_required in the README.md as well?

We need @BillyONeal's review now. :-)

Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>

Clovel · 2020-04-22T18:55:42Z

Great. What do you think about updating the cmake_minimum_required in the README.md as well?

I missed that. Fixed it.

BillyONeal · 2020-04-22T19:28:33Z

If you're trying to fix macos can you turn that back on in azure-pipelines.yml?

garethsb · 2020-04-22T21:05:38Z

FWIW, Homebrew was excised in c813671.

And the other macOS test configs were removed in 3a3cc20.

Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>

Clovel · 2020-04-23T12:25:24Z

If you're trying to fix macos can you turn that back on in azure-pipelines.yml?

Done.

EDIT : I'm trying to fix the CMake build on macOS, amongst other things. I might revert this and leave it for another PR if it blocks merging.

BillyONeal · 2020-04-23T23:56:45Z

#1392 might fix it

garethsb · 2020-04-24T08:37:37Z

I was about to say I couldn't see where the submodule was checked out! Thanks, @BillyONeal!

…ninja path for the vcpkg/MacOS job.

c72578

The .exe extension should be removed from ninja.exe in case of osx

c72578 · 2020-04-24T19:32:59Z

azure-pipelines.yml

+    - task: CMake@1
+      inputs:
+        workingDirectory: 'build.release'
+        cmakeArgs: '-G Ninja -DCMAKE_MAKE_PROGRAM=../vcpkg/downloads/tools/ninja-1.10.0-osx/ninja.exe -DCMAKE_BUILD_TYPE=Release -DCMAKE_TOOLCHAIN_FILE=../vcpkg/scripts/buildsystems/vcpkg.cmake ..'


ninja-1.10.0-osx/ninja.exe->ninja-1.10.0-osx/ninja

c72578 · 2020-04-24T19:33:19Z

azure-pipelines.yml

+        cmakeArgs: '-G Ninja -DCMAKE_MAKE_PROGRAM=../vcpkg/downloads/tools/ninja-1.10.0-osx/ninja.exe -DCMAKE_BUILD_TYPE=Release -DCMAKE_TOOLCHAIN_FILE=../vcpkg/scripts/buildsystems/vcpkg.cmake ..'
+    - script: |
+        cd build.debug
+        ../vcpkg/downloads/tools/ninja-1.10.0-osx/ninja.exe


ninja-1.10.0-osx/ninja.exe->ninja-1.10.0-osx/ninja

c72578 · 2020-04-24T19:33:30Z

azure-pipelines.yml

+      displayName: 'Run Tests debug'
+    - script: |
+        cd build.release
+        ../vcpkg/downloads/tools/ninja-1.10.0-osx/ninja.exe


ninja-1.10.0-osx/ninja.exe->ninja-1.10.0-osx/ninja

c72578 · 2020-04-24T19:34:02Z

azure-pipelines.yml

+    - task: CMake@1
+      inputs:
+        workingDirectory: 'build.debug'
+        cmakeArgs: '-G Ninja -DCMAKE_MAKE_PROGRAM=../vcpkg/downloads/tools/ninja-1.10.0-osx/ninja.exe -DCMAKE_BUILD_TYPE=Debug -DCMAKE_TOOLCHAIN_FILE=../vcpkg/scripts/buildsystems/vcpkg.cmake ..'


ninja-1.10.0-osx/ninja.exe->ninja-1.10.0-osx/ninja

BillyONeal · 2020-04-24T22:34:22Z

Megaderp :)

BillyONeal · 2020-04-24T22:35:11Z

@Clovel iOS is still toast. Do you want to investigate that or just disable it?

BillyONeal · 2020-04-24T22:36:40Z

The .exe extension should be removed from ninja.exe in case of osx

Fixed!

Clovel · 2020-04-24T23:25:16Z

@Clovel iOS is still toast. Do you want to investigate that or just disable it?

I’ll take a look in the coming days. If I don’t feel it, I’ll disable the build plan.

BillyONeal · 2020-04-24T23:53:30Z

@Clovel iOS is still toast. Do you want to investigate that or just disable it?

I’ll take a look in the coming days. If I don’t feel it, I’ll disable the build plan.

This passed so I'll merge and do a release, if you end up fixing it that'd be much appreciated :)

BillyONeal · 2020-04-25T00:38:41Z

Thanks for your contribution!

PR microsoft#1383 switched to use ${CMAKE_INSTALL_INCLUDEDIR} as the destination to install cpprestsdk headers. That change did not update the INTSTALL_INTERFACE for the cpprest target and also did not update all the places where the include directory was hard coded.

Clovel changed the title ~~CMake typo fixes + CMake search for OpenSSL (macOS)~~ CMake fixes + CMake search for OpenSSL (macOS) Apr 5, 2020

Clovel mentioned this pull request Apr 5, 2020

find_package doesn't work on macOS w/ brew installation #1382

Open

Clovel added 4 commits April 6, 2020 02:43

Fixed CMake search for OpenSSL on macOS

fd6f52b

Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>

Generate CMake config-version.cmake file

4d9dd7b

Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>

Boost requirement cannot work if "AND OFF" is used in cpprestsdk-conf…

c9de3b9

…ig.in.cmake Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>

Use CMAKE_INSTALL_INCLUDEDIR to install headers

c199baa

Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>

Clovel force-pushed the cmake branch from 62e7335 to c199baa Compare April 6, 2020 00:45

Changed minimum CMake version to 3.9

9b2f797

Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>

Clovel force-pushed the cmake branch from 944a89e to 9b2f797 Compare April 22, 2020 18:55

Reactivated the macOS build in Azure pipelines

47f0f89

Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>

BillyONeal added 3 commits April 23, 2020 19:14

Merge remote-tracking branch 'origin/master' into cmake

5245b8d

Update cache and checkout the right submodules in MacOS jobs.

aef0a21

Fix path.

e79d4b7

BillyONeal added 2 commits April 24, 2020 01:58

Turn off the cache task because it seems broken. Explicitly pass the …

32456e3

…ninja path for the vcpkg/MacOS job.

Fix the ninja path to be accurate.

597fb0e

c72578 reviewed Apr 24, 2020

View reviewed changes

What Bill gets for writing code at 3AM.

87814df

Use full path and turn off ios.

9399061

BillyONeal merged commit 319dd31 into microsoft:master Apr 24, 2020

garethsb mentioned this pull request Apr 26, 2020

Missing dependencies on CMake exported target #1384

Open

schultetwin1 mentioned this pull request Apr 29, 2020

Use CMAKE_INSTALL_INCLUDEDIR for all include paths #1403

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake fixes + CMake search for OpenSSL (macOS) #1383

CMake fixes + CMake search for OpenSSL (macOS) #1383

Clovel commented Apr 5, 2020 •

edited

msftclas commented Apr 5, 2020 •

edited

garethsb commented Apr 7, 2020

Clovel commented Apr 7, 2020

Clovel commented Apr 7, 2020

garethsb commented Apr 7, 2020

Clovel commented Apr 7, 2020

garethsb commented Apr 8, 2020

Clovel commented Apr 8, 2020

garethsb commented Apr 9, 2020

Clovel commented Apr 9, 2020

c72578 commented Apr 9, 2020

garethsb commented Apr 22, 2020 •

edited

Clovel commented Apr 22, 2020 via email

Clovel commented Apr 22, 2020

garethsb commented Apr 22, 2020

Clovel commented Apr 22, 2020

BillyONeal commented Apr 22, 2020

garethsb commented Apr 22, 2020 •

edited

Clovel commented Apr 23, 2020 •

edited

BillyONeal commented Apr 23, 2020

garethsb commented Apr 24, 2020

c72578 left a comment

c72578 Apr 24, 2020

c72578 Apr 24, 2020

c72578 Apr 24, 2020

c72578 Apr 24, 2020

BillyONeal commented Apr 24, 2020

BillyONeal commented Apr 24, 2020

BillyONeal commented Apr 24, 2020

Clovel commented Apr 24, 2020

BillyONeal commented Apr 24, 2020

BillyONeal commented Apr 25, 2020

CMake fixes + CMake search for OpenSSL (macOS) #1383

CMake fixes + CMake search for OpenSSL (macOS) #1383

Conversation

Clovel commented Apr 5, 2020 • edited

msftclas commented Apr 5, 2020 • edited

garethsb commented Apr 7, 2020

Clovel commented Apr 7, 2020

Clovel commented Apr 7, 2020

garethsb commented Apr 7, 2020

Clovel commented Apr 7, 2020

garethsb commented Apr 8, 2020

Clovel commented Apr 8, 2020

garethsb commented Apr 9, 2020

Clovel commented Apr 9, 2020

c72578 commented Apr 9, 2020

garethsb commented Apr 22, 2020 • edited

Clovel commented Apr 22, 2020 via email

Clovel commented Apr 22, 2020

garethsb commented Apr 22, 2020

Clovel commented Apr 22, 2020

BillyONeal commented Apr 22, 2020

garethsb commented Apr 22, 2020 • edited

Clovel commented Apr 23, 2020 • edited

BillyONeal commented Apr 23, 2020

garethsb commented Apr 24, 2020

c72578 left a comment

Choose a reason for hiding this comment

c72578 Apr 24, 2020

Choose a reason for hiding this comment

c72578 Apr 24, 2020

Choose a reason for hiding this comment

c72578 Apr 24, 2020

Choose a reason for hiding this comment

c72578 Apr 24, 2020

Choose a reason for hiding this comment

BillyONeal commented Apr 24, 2020

BillyONeal commented Apr 24, 2020

BillyONeal commented Apr 24, 2020

Clovel commented Apr 24, 2020

BillyONeal commented Apr 24, 2020

BillyONeal commented Apr 25, 2020

Clovel commented Apr 5, 2020 •

edited

msftclas commented Apr 5, 2020 •

edited

garethsb commented Apr 22, 2020 •

edited

garethsb commented Apr 22, 2020 •

edited

Clovel commented Apr 23, 2020 •

edited