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

Jump to bottom

cmake: set HAVE_SOCKADDR_IN6_SIN6_SCOPE_ID on Windows #9726

Closed
wants to merge 3 commits into from
Closed

cmake: set HAVE_SOCKADDR_IN6_SIN6_SCOPE_ID on Windows #9726

wants to merge 3 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 13, 2022 •
edited

This configuration option is enabled unconditionally in lib/config-win32.h. Make it apply to CMake builds as well.

While here, delete a broken check for HAVE_SOCKADDR_IN6_SIN6_SCOPE_ID from CMakeLists.txt. This came with the initial commit [1], but did not include the actual verification code inside CMake/CurlTests.c, so it always failed. A later commit [2] added a second test, for non-Windows platforms.

Enabling this flag causes test 1056 to fail with CMake builds, just like they do with autotools builds. Let's apply the same solution and ignore the results here as well.

[1] 4c5307b
[2] aec7c5a

Closes #xxxx

@vszakats vszakats added cmake Windows Windows-specific labels Oct 13, 2022
@vszakats vszakats force-pushed the cmakesync1 branch 2 times, most recently from 310733f to 89718ca Compare October 14, 2022 01:18
@vszakats
Copy link
Member Author

AppVeyor CI, CMake debug builds (both MSVC and MinGW) are consistently failing test 1056:
https://ci.appveyor.com/project/curlorg/curl/builds/45065933/job/8hidqd39hbaot4fu#L4047

Seems related. I rarely do debug builds, so any help is appreciated.

@MarcelRaad
Copy link
Member

It doesn't happen in release builds only because they're not running tests at all.

Test 1056 is also failing for the autotools builds on AppVeyor, it's just disabled there. So it probably can be safely disabled for CMake too. I don't know the reason why it fails though.

@vszakats
Copy link
Member Author

vszakats commented Oct 14, 2022 •
edited

It might be because it should not be enabled without IPv6. Committed a fix. Let's see how it runs.

@vszakats
Copy link
Member Author

vszakats commented Oct 14, 2022 •
edited

Couldn't find any place in the code where this flag had an effect with IPv6 disabled. It's also always-on in config-win32.h.

Going with ignoring results for this test with CMake as well. Thank you @MarcelRaad for the suggestion!

@vszakats
cmake: set HAVE_SOCKADDR_IN6_SIN6_SCOPE_ID on Windows
bcf3b42 
This configuration option is enabled unconditionally in
`lib/config-win32.h`. Make it apply to CMake builds as well.

While here, delete a broken check for
`HAVE_SOCKADDR_IN6_SIN6_SCOPE_ID` from `CMakeLists.txt`. This came with
the initial commit [1], but did not include the actual verification code
inside `CMake/CurlTests.c`, so it always failed. A later commit [2]
added a second test, for non-Windows platforms.

Enabling this flag causes test 1056 to fail with CMake builds, just like
they do with autotools builds. Let's apply the same solution and ignore
the results here as well.

[1] 4c5307b
[2] aec7c5a

Reviewed-by: Daniel Stenberg
Assisted-by: Marcel Raad

Closes #xxxx
@vszakats
delete broken test for HAVE_SOCKADDR_IN6_SIN6_ADDR
eb39ad4
@vszakats
disabling test 1056 for CMake (debug) builds
beecb20
@vszakats vszakats closed this in 86f2d8f Oct 14, 2022
@vszakats vszakats deleted the cmakesync1 branch October 14, 2022 19:27
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 14, 2022
@vszakats
curl-cmake: delete manual options after upstreaming them [ci skip]
87543bf 
Ref: curl/curl#9725
Ref: curl/curl#9726
Ref: curl/curl#9727
@mback2k
Copy link
Member

mback2k commented Oct 15, 2022

I wonder if maybe test1056 is not correct if it should not run without full IPv6 support, but it has the corresponding keyword and feature required?

curl/tests/data/test1056

Line 7 in 86f2d8f

IPv6

curl/tests/data/test1056

Line 46 in 86f2d8f

ipv6

@vszakats
Copy link
Member Author

vszakats commented Oct 15, 2022 •
edited

@mback2k: Strangely, today these (8) failures reappeared under a different PR, that did not have this patch. Rebasing to this one "fixed it". Something indeed feels off with these tests.

@mback2k
Copy link
Member

mback2k commented Oct 15, 2022

So, do I understand it correctly that fa62a26 makes test 1056 work again?

@vszakats
Copy link
Member Author

vszakats commented Oct 15, 2022 •
edited

Nope, sorry, I had to rebase it to this patch, which disabled those tests (CI run here). What I didn't understand is why did they (re)appear there in the first place.

@mback2k
Copy link
Member

mback2k commented Oct 15, 2022 •
edited

Ah, understood. Maybe because your PR enabled the IPv6 feature for those builds?

@vszakats
Copy link
Member Author

That's the interesting bit, that PR is not supposed to change things around IPv6. It adds an additional safeguard to lib/hostip4.c to avoid a potential misconfiguration with GETADDRNAME vs GETADDRNAME_THREADSAFE. Such mis-configuration should not happen with curl build systems (and if it does, it would fail at link time), so at this point it should not change anything at all, but certainly not test results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder bagder approved these changes

@MarcelRaad MarcelRaad Awaiting requested review from MarcelRaad

@mback2k mback2k Awaiting requested review from mback2k

Assignees
No one assigned
Labels
cmake Windows Windows-specific
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vszakats @MarcelRaad @mback2k @bagder