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

Resolve CURLOPT_SSL_OPTIONS issues #1128

Merged
merged 3 commits into from
Nov 9, 2024
Merged

Conversation

gentooise
Copy link
Contributor

@gentooise gentooise commented Oct 18, 2024

Closes #1127

Note for reviewers: I tried to keep equivalent #ifdef conditions, but I'm not really sure on the history/rationale of those. Please double check.

@COM8 COM8 added the Bug 🐛 label Oct 18, 2024
@COM8 COM8 added this to the CPR 1.11.x milestone Oct 18, 2024
Copy link
Member

@COM8 COM8 left a comment

Choose a reason for hiding this comment

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

Only one minor change is required. Thanks for fixing it!

Your assumption on version check macros is correct. The one you removes is redundant.

cpr/session.cpp Show resolved Hide resolved
Co-authored-by: Fabian Sauter <sauter.fabian@mailbox.org>
@gentooise
Copy link
Contributor Author

gentooise commented Oct 21, 2024

Is there some pipeline issue I may help with? @COM8

@COM8
Copy link
Member

COM8 commented Oct 22, 2024

I'm not sure why it's failing right now. To me it looks like the OpenSSL windows install is not working properly again.

@COM8
Copy link
Member

COM8 commented Oct 22, 2024

I triggered master again to see if it happens there as well.

@COM8
Copy link
Member

COM8 commented Oct 22, 2024

@gentooise Looks like it's something related to this MR. Is there any way you can debug this on Windows with OpenSSL?

Ref: https://github.com/libcpr/cpr/blob/master/.github/workflows/ci.yml#L224

@gentooise
Copy link
Contributor Author

@gentooise Looks like it's something related to this MR. Is there any way you can debug this on Windows with OpenSSL?

Ref: https://github.com/libcpr/cpr/blob/master/.github/workflows/ci.yml#L224

I might try but I need some help. Which Windows and OpenSSL version are used for the CI tests?
Can you provide steps to run the tests?

@COM8
Copy link
Member

COM8 commented Oct 23, 2024

Awesome!
Not sure if you have access to the build log, but here it is: job-logs.txt

It's using:

  • Microsoft Windows Server 2022 - 10.0.20348 but I'd say any windows >= 10 is good to go here.
  • openssl v3.3.2 via chocolatey.org

@gentooise
Copy link
Contributor Author

gentooise commented Nov 8, 2024

Awesome! Not sure if you have access to the build log, but here it is: job-logs.txt

It's using:

  • Microsoft Windows Server 2022 - 10.0.20348 but I'd say any windows >= 10 is good to go here.
  • openssl v3.3.2 via chocolatey.org

Thank you.
I'm following these steps (taken from your job-logs.txt) but I get "No tests were found!!!":

cmake -DCMAKE_BUILD_TYPE=Release -B build -G "Visual Studio 17 2022"
cmake --build build --config Release --parallel 4
ctest -C Release --repeat until-pass:5 --output-on-failure

What am I missing? @COM8

@COM8
Copy link
Member

COM8 commented Nov 8, 2024

Try adding -DCPR_BUILD_TESTS=ON when configuring CMake to enable Tests.

Copy link
Member

@COM8 COM8 left a comment

Choose a reason for hiding this comment

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

I looked into it closely again and every time different tests are failing. This to me indicates there is nothing wrong with your code changes but in general something with the tests.

Approving/Merging

Also Clang-Tidy failing has nothing to do with this MR. It just got updated.

@COM8 COM8 merged commit d3e2786 into libcpr:master Nov 9, 2024
50 of 52 checks passed
@gentooise
Copy link
Contributor Author

gentooise commented Nov 9, 2024

I looked into it closely again and every time different tests are failing. This to me indicates there is nothing wrong with your code changes but in general something with the tests.

Approving/Merging

Also Clang-Tidy failing has nothing to do with this MR. It just got updated.

Yesterday I also managed to run the tests, but didn't have time to report the results. Anyway, I can confirm this: I've also seen random/unrelated tests failing... (AsyncGetTests in particular).

If anyone wants to troubleshoot, I collected the steps to run the tests from scratch, starting from a clean Windows Server VM (I've used this vagrant box https://app.vagrantup.com/peru/boxes/windows-server-2022-standard-x64-eval).

Steps used (from admin PowerShell):

# install choco
Set-ExecutionPolicy Bypass -Scope Process -Force; [System.Net.ServicePointManager]::SecurityProtocol = [System.Net.ServicePointManager]::SecurityProtocol -bor 3072; iex ((New-Object System.Net.WebClient).DownloadString('https://community.chocolatey.org/install.ps1'))
# restart powershell here
choco install visualstudio2022community
choco install visualstudio2022buildtools
choco install visualstudio2022-workload-vctools --package-parameters "--includeOptional"
choco install openssl --version=3.3.2
choco install git.install
choco install cmake --installargs 'ADD_CMAKE_TO_PATH=System' --apply-install-arguments-to-dependencies
# restart powershell here
git clone https://github.com/gentooise/cpr.git
cd cpr
git checkout 8497eac1c7124cb53ac3d0534a3093f1b9e2a9b4 # checkout the relevant commit here
cmake -DCMAKE_BUILD_TYPE=Release -DCPR_FORCE_OPENSSL_BACKEND=ON -DCPR_BUILD_TESTS=ON -DCPR_BUILD_TESTS_SSL=ON -B build -G "Visual Studio 17 2022"
cmake --build build --config Release --parallel 4
cd build
ctest -C Release --repeat until-pass:5 --output-on-failure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libcurl CURLOPT_SSL_OPTIONS are not managed correctly
2 participants