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

test1960: verify CURL_SOCKOPT_ALREADY_CONNECTED #10651

Closed
wants to merge 3 commits into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Mar 1, 2023

When returned from the CURLOPT_SOCKOPTFUNCTION, like when we have a custom socket connected in the app, passed in to libcurl.

Verifies the fix in #10648

@bagder bagder added the tests label Mar 1, 2023
tests/libtest/lib1960.c Outdated Show resolved Hide resolved
When returned from the CURLOPT_SOCKOPTFUNCTION, like when we have a
custom socket connected in the app, passed in to libcurl.

Verifies the fix in #10648
#include "test.h"

#ifdef WIN32
#undef _WIN32_WINNT
Copy link
Member

@MarcelRaad MarcelRaad Mar 2, 2023

Choose a reason for hiding this comment

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

Hmm, seems like my comment got applied only to the commit and not shown in the PR:
Or maybe just make HAVE_INET_PTON a requirement for executing this test? The Windows headers might already be included indirectly by test.h, so this might be too late to redefine the value.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah hurt by that again ...

Copy link
Member Author

Choose a reason for hiding this comment

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

but then, where/when is mingw.h included which is the header that apparently sets the "bad" define?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

While here: should we perhaps drop mingw v1 support and live a happier life?

Copy link
Member

@MarcelRaad MarcelRaad Mar 2, 2023

Choose a reason for hiding this comment

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

I'd just make the test always succeed if HAVE_INET_PTON is not set?

Dropping MinGW v1 might fire back, unfortunately. In another open source project I'm involved in, when that was broken, there were always many complaints by people new to software development even though it was not officially supported. The original MinGW was often the thing tutorials pointed to for Windows, or they mentioned just "MinGW" and a Google search for that points to its Wikipedia page, linking to the original MinGW homepage. Not sure if that's still the case, but that was not so many years ago when original MinGW was already very much outdated.

So I wouldn't care too much about finding workarounds for it to make features work, but being able to get a clean default compile might make our lives easier than not having to care for it at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd just make the test always succeed if HAVE_INET_PTON is not set?

Not terribly easy since the test verifies protocol parts and I think it makes sense to have it do that. I will have to also invent some way for how this test can be skipped for mingw v1...

Copy link
Member Author

Choose a reason for hiding this comment

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

trying a precheck to skip the test if inet_pton is not present

Copy link
Member

@vszakats vszakats Mar 2, 2023

Choose a reason for hiding this comment

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

Replying to https://curl.se/mail/lib-2023-03/0006.html:

"original" MinGW has been a constant source of issues :( (I've spent many days digging for issues unique to it) Its Windows headers are seriously outdated. The compiler as well. It is (and always was) difficult to install. It only supports Intel 32-bit, which by itself is an almost dead platform. Now even its original website (mingw.org) is gone.

It's never an easy decision to drop a platform, compiler, feature, but in this case I'd vote to do so. It takes off a ton of complications, leaving largely a modern mingw-w64 and MSVC to worry about on Windows.

[ Meanwhile mingw-w64 can be installed via MSYS2 or standalone, or using most unixy package managers as well. Its Wine headers are up to date. It's actively developed. ]

@bagder
Copy link
Member Author

bagder commented Mar 3, 2023

Success!! 🚀

@bagder bagder closed this in e4dfe6f Mar 3, 2023
@bagder bagder deleted the bagder/test1960 branch March 3, 2023 07:36
dfandrich added a commit that referenced this pull request Mar 30, 2023
Otherwise, it might find the binary in .libs which can cause it to use
the system libcurl which can fail. This error is only visible by
noticing that the test is skipped.

Follow-up to e4dfe6f

Ref: #10651
dfandrich added a commit that referenced this pull request Mar 31, 2023
Otherwise, it might find the binary in .libs which can cause it to use
the system libcurl which can fail. This error is only visible by
noticing that the test is skipped.

Follow-up to e4dfe6f

Ref: #10651
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
When returned from the CURLOPT_SOCKOPTFUNCTION, like when we have a
custom socket connected in the app, passed in to libcurl.

Verifies the fix in curl#10648

Closes curl#10651
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
Otherwise, it might find the binary in .libs which can cause it to use
the system libcurl which can fail. This error is only visible by
noticing that the test is skipped.

Follow-up to e4dfe6f

Ref: curl#10651
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants