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

Don't explicitely include FindOpenSSL.cmake #2171

Closed
wants to merge 1 commit into from

Conversation

cedral
Copy link

@cedral cedral commented Jan 5, 2024

There is no need to do this because find_package is documented to do this for you and vcpkg has a toolchain file that short circuits find_package for OpenSSL. This does not work correctly if you explicitly include FindOpenSSL in your CMakeLists.txt file.
You can reproduce this issue by installing OpenSSL 1.1.1w from Shining Light Productions OpenSSL in C:\Program Files\OpenSSL then setting up vcpkg on windows with visual studio and calling

vcpkg install bzip2 zlib json-c openssl dirent getopt

then try to build rnp with

cmake -DCMAKE_TOOLCHAIN_FILE=...\vcpkg\scripts\buildsystems\vcpkg.cmake -DCRYPTO_BACKEND=openssl ..

After doing this the include files will be the OpenSSL 3.2 version in vcpkg but the libraries for the debug configuration in the generated visual studio solution will be the ones from OpenSSL 1.1.1 in Program Files. This will of course cause a bunch of undefined symbols at link time.
Removing the explicit include of FindOpenSSL fixes this and as I mentioned before it shouldn't be necessary to ever do that because find_package will implicitly include that for you.

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2cf8fd2) 77.07% compared to head (900ca4a) 77.07%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2171   +/-   ##
=======================================
  Coverage   77.07%   77.07%           
=======================================
  Files         194      194           
  Lines       37257    37256    -1     
=======================================
  Hits        28715    28715           
+ Misses       8542     8541    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ni4
Copy link
Contributor

ni4 commented Jan 16, 2024

@cedral Thanks for the contribution! Don't you mind if I cherry-pick your commit into the #2172 ? Otherwise you'll need to rebase after that PR is merged as it fixes some CI failures, introduced with new FreeBSD runner version.

@ni4
Copy link
Contributor

ni4 commented Jan 19, 2024

Closing this as commit was cherry-picked and merged via #2172

@ni4 ni4 closed this Jan 19, 2024
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