Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
CMake fixes + CMake search for OpenSSL (macOS) #1383
Conversation
|
|
Clovel
changed the title
Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>
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>
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.
And
|
|
I don't have time to look into the whole issue. I tried to use It should also have a GitHub Action for macOS. |
|
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. |
I've been missing having the version info available for a long time. The |
|
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. |
Ah yes, sorry, I realise your code must ultimately be derived from same root as current CMake config version file template for I'm not sure on C++ REST SDK backward compatibility policy between major/minor releases but maybe |
Maybe. I'm not familiar enough with the project to know, sorry. |
|
|
See #593 - the issue is the use of @Clovel, how do you think about bumping |
|
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.
|
Done. |
|
Great. What do you think about updating the We need @BillyONeal's review now. :-) |
Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>
I missed that. Fixed it. |
|
If you're trying to fix macos can you turn that back on in azure-pipelines.yml? |
Signed-off-by: Clovis Durand <cd.clovel19@gmail.com>
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. |
|
#1392 might fix it |
|
I was about to say I couldn't see where the submodule was checked out! Thanks, @BillyONeal! |
…ninja path for the vcpkg/MacOS job.
|
The .exe extension should be removed from ninja.exe in case of osx |
| - 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 ..' |
| 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 |
| displayName: 'Run Tests debug' | ||
| - script: | | ||
| cd build.release | ||
| ../vcpkg/downloads/tools/ninja-1.10.0-osx/ninja.exe |
| - 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 ..' |
|
Megaderp :) |
|
@Clovel iOS is still toast. Do you want to investigate that or just disable it? |
Fixed! |
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
merged commit 319dd31
into
microsoft:master
|
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.
/usr/local/Cellar/openssl@1.1/instead of/usr/local/Cellar/openssl/, or even both. I fixed the search forOPENSSL_ROOT_DIRand limited it's contents to one path only.<package>-config-version.in.cmakefile configuration for CMakefind_packageusage.<package>-config.in.cmakefor Boost usage. There was a@CPPREST_USES_BOOST@ AND OFFcondition, which is always false. The result was that the Boost libraries and includes were not included in the users CMake project.CMAKE_INSTALL_INCLUDEDIRto install headers.cpprestsdkalready usesCMAKE_INSTALL_LIBDIR&CMAKE_INSTALL_BINDIR