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

cmake: set HAVE_SOCKADDR_IN6_SIN6_SCOPE_ID on Windows #9726

Closed
wants to merge 3 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 13, 2022

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 Oct 14, 2022
bagder
bagder approved these changes Oct 14, 2022
@vszakats
Copy link
Member Author

vszakats commented Oct 14, 2022

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

MarcelRaad commented Oct 14, 2022

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

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

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 added 3 commits Oct 14, 2022
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
@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?


@vszakats
Copy link
Member Author

vszakats commented Oct 15, 2022

@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

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

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

@vszakats
Copy link
Member Author

vszakats commented Oct 15, 2022

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
Labels
cmake Windows Windows-specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants