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

[openssl] Add cmake wrapper to handle OPENSSL_ROOT_DIR #18042

Merged
merged 24 commits into from
Jun 11, 2021

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented May 21, 2021

Since OPENSSL_ROOT_DIR may be defined externally, add cmake wrapper to specify the value as CURRENT_INSTALLED_DIR to avoid FindOpenssl.cmake to find openssl elsewhere.

Fixes #16709.

Fix kf5holidays baseline issue caused by the parallel configure:

CMake Error: Could not open file for write in copy operation D:/buildtrees/kf5holidays/src/v5.81.0-8d1b566dfd.clean/.clang-format.tmp
CMake Error: : System Error: Permission denied
CMake Error at D:/installed/x86-windows/share/ECM/kde-modules/KDEClangFormat.cmake:67 (configure_file):
  configure_file Problem configuring file
Call Stack (most recent call first):
  D:/installed/x86-windows/share/ECM/kde-modules/KDEFrameworkCompilerSettings.cmake:79 (include)
  CMakeLists.txt:18 (include)

@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels May 21, 2021
@JackBoosY
Copy link
Contributor Author

@Krzmbrzl @SamuelMarks Can you please test this PR?

Thanks.

@SamuelMarks
Copy link
Contributor

SamuelMarks commented May 22, 2021

@JackBoosY Perfect

src/main.c

#include <stdio.h>

#include <openssl/crypto.h>


int main(void) {
    puts(OPENSSL_VERSION_TEXT);
}

CMakeLists.txt

cmake_minimum_required(VERSION 3.0)
project(pp VERSION 0.0.0 LANGUAGES C)

find_package(OpenSSL REQUIRED)

add_executable("${PROJECT_NAME}" "src/main.c")

target_link_libraries("${PROJECT_NAME}" PRIVATE OpenSSL::SSL OpenSSL::Crypto)

sh

$ git clone --depth=1 https://github.com/microsoft/vcpkg && cd vcpkg
$ gh pr checkout 18042
$ ./bootstrap-vcpkg.sh
$ ./vcpkg install openssl
$ cd /<source_root>/build
$ cmake -DCMAKE_TOOLCHAIN_FILE=[path]'/vcpkg/scripts/buildsystems/vcpkg.cmake' ..
-- The C compiler identification is AppleClang 12.0.5.12050022
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Found OpenSSL: /tmp/vcpkg/installed/x64-osx/debug/lib/libcrypto.a (found version "1.1.1k")  
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/openssl-ver/build
$ cmake --build .
[ 50%] Building C object CMakeFiles/pp.dir/src/main.c.o
[100%] Linking C executable pp
[100%] Built target pp
$ ./pp
OpenSSL 1.1.1k  25 Mar 2021

@Krzmbrzl
Copy link
Contributor

Krzmbrzl commented May 23, 2021

It always worked on my machine. We only received reports of some users that were having issues with this. Thus I can't really test this.

From the changes this seems to includes the workaround that we always told them to use, so LGTM 👍

@Neumann-A
Copy link
Contributor

I wonder if:

file(TO_CMAKE_PATH "$ENV{PROGRAMFILES}" Z_VCPKG_PROGRAMFILES)
set(Z_VCPKG_PROGRAMFILESX86_NAME "PROGRAMFILES(x86)")
file(TO_CMAKE_PATH "$ENV{${Z_VCPKG_PROGRAMFILESX86_NAME}}" Z_VCPKG_PROGRAMFILESX86)
set(CMAKE_SYSTEM_IGNORE_PATH
"${Z_VCPKG_PROGRAMFILES}/OpenSSL"
"${Z_VCPKG_PROGRAMFILES}/OpenSSL-Win32"
"${Z_VCPKG_PROGRAMFILES}/OpenSSL-Win64"
"${Z_VCPKG_PROGRAMFILES}/OpenSSL-Win32/lib/VC"
"${Z_VCPKG_PROGRAMFILES}/OpenSSL-Win64/lib/VC"
"${Z_VCPKG_PROGRAMFILES}/OpenSSL-Win32/lib/VC/static"
"${Z_VCPKG_PROGRAMFILES}/OpenSSL-Win64/lib/VC/static"
"${Z_VCPKG_PROGRAMFILESX86}/OpenSSL"
"${Z_VCPKG_PROGRAMFILESX86}/OpenSSL-Win32"
"${Z_VCPKG_PROGRAMFILESX86}/OpenSSL-Win64"
"${Z_VCPKG_PROGRAMFILESX86}/OpenSSL-Win32/lib/VC"
"${Z_VCPKG_PROGRAMFILESX86}/OpenSSL-Win64/lib/VC"
"${Z_VCPKG_PROGRAMFILESX86}/OpenSSL-Win32/lib/VC/static"
"${Z_VCPKG_PROGRAMFILESX86}/OpenSSL-Win64/lib/VC/static"
"C:/OpenSSL/"
"C:/OpenSSL-Win32/"
"C:/OpenSSL-Win64/"
"C:/OpenSSL-Win32/lib/VC"
"C:/OpenSSL-Win64/lib/VC"
"C:/OpenSSL-Win32/lib/VC/static"
"C:/OpenSSL-Win64/lib/VC/static"
)

can be moved into the wrapper?

@SamuelMarks
Copy link
Contributor

(reading on my phone) unless I missed something, OpenSSL is being installed to C:\OpenSSL? - That doesn't seem super standard, and sounds more like a CMake contribution so their FindOpenSSL.cmake knows more places to look…

@Neumann-A
Copy link
Contributor

That doesn't seem super standard, and sounds more like a CMake contribution so their FindOpenSSL.cmake knows more places to look…

Yeah it is the upstream CMake module.
https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindOpenSSL.cmake#L167-L172

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

This LGTM, though could you consider addressing @Neumann-A's comment #18042 (comment) ?

versions/o-/openssl.json Outdated Show resolved Hide resolved
ports/openssl/vcpkg-cmake-wrapper.cmake Outdated Show resolved Hide resolved
ports/openssl/vcpkg-cmake-wrapper.cmake Outdated Show resolved Hide resolved
Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

LGTM

versions/o-/openssl.json Outdated Show resolved Hide resolved
@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Jun 1, 2021
ports/openssl/vcpkg.json Outdated Show resolved Hide resolved
versions/o-/openssl.json Outdated Show resolved Hide resolved
ports/openssl/vcpkg.json Outdated Show resolved Hide resolved
versions/o-/openssl.json Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor Author

@strega-nil-ms Please review and merge this PR first if it's suitable for us.

@strega-nil-ms
Copy link
Contributor

@JackBoosY could you update the versions files?

@strega-nil-ms strega-nil-ms merged commit 02fa0eb into microsoft:master Jun 11, 2021
@strega-nil-ms
Copy link
Contributor

Thanks @JackBoosY :)

@JackBoosY JackBoosY deleted the dev/jack/16709 branch June 15, 2021 02:07
@Hoikas
Copy link
Contributor

Hoikas commented Jul 22, 2021

Breaks consumers of static openssl on windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OpenSSL] Explicitly set OPENSSL_ROOT_DIR
8 participants