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.

Sign up for GitHub

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

GHA/windows: add MSVC MSH3 job #14922

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

GHA/windows: add MSVC MSH3 job #14922

wants to merge 1 commit into from
+8 −1

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Sep 15, 2024 •
edited
Loading

Add msvc msh3 job
Fix compilation warning / error on test: Unreachable code.
Test 1014 failing. Should fix.

Http2 don't compile with msh3. Is it by design?

Compilation error:

D:\a\curl\curl\lib\vquic\curl_msh3.c(251,13): error C2084: function 'void drain_stream(Curl_cfilter *,Curl_easy *,h2_stream_ctx *)' already has a body [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')
      D:\a\curl\curl\lib\http2.c(352,13):
      see previous definition of 'drain_stream'
  
D:\a\curl\curl\lib\vquic\curl_msh3.c(599,7): error C2198: 'void drain_stream(Curl_cfilter *,Curl_easy *,h2_stream_ctx *)': too few arguments for call [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')
  
D:\a\curl\curl\lib\vquic\curl_msh3.c(725,7): error C2198: 'void drain_stream(Curl_cfilter *,Curl_easy *,h2_stream_ctx *)': too few arguments for call [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')
  
D:\a\curl\curl\lib\vquic\curl_msh3.c(7[29](https://github.com/curl/curl/actions/runs/10873342646/job/30169351365?pr=14922#step:10:29),7): error C2198: 'void drain_stream(Curl_cfilter *,Curl_easy *,h2_stream_ctx *)': too few arguments for call [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')
  
D:\a\curl\curl\lib\vquic\curl_msh3.c(752,7): error C2198: 'void drain_stream(Curl_cfilter *,Curl_easy *,h2_stream_ctx *)': too few arguments for call [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')
  
D:\a\curl\curl\lib\vquic\curl_msh3.c(764,5): error C2198: 'void drain_stream(Curl_cfilter *,Curl_easy *,h2_stream_ctx *)': too few arguments for call [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')

Depends:

@github-actions github-actions bot added tests CI Continuous Integration labels Sep 15, 2024
vszakats
vszakats reviewed Sep 15, 2024
View reviewed changes
tests/unit/unit1660.c
@@ -41,7 +41,7 @@ unit_stop(void)
#if defined(CURL_DISABLE_HTTP) || defined(CURL_DISABLE_HSTS)
UNITTEST_START
{
return CURLE_OK; /* nothing to do when HTTP or HSTS are disabled */
/* return CURLE_OK; nothing to do when HTTP or HSTS are disabled */
Copy link
Member

@vszakats vszakats Sep 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A fix has been pending for this here:
https://github.com/curl/curl/pull/14772/files#diff-aa6618147822be7a9ddb938893eaa6c9e8e7216a9e875147eb1bd1e5708143cd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know when it merge, and I will rebase.

@vszakats
Copy link
Member

vszakats commented Sep 15, 2024 •
edited
Loading

Add msvc msh3 job Fix compilation warning / error on test: Unreachable code. Test 1014 failing. Should fix.

Http2 don't compile with msh3. Is it intention?

The nghttp2 package is not installed, isn't that the reason?

@talregev
Copy link
Contributor Author

Add msvc msh3 job Fix compilation warning / error on test: Unreachable code. Test 1014 failing. Should fix.
Http2 don't compile with msh3. Is it intention?

The nghttp2 package is not installed, isn't that the reason?

No. I intentionally not install it. When I will add it, it will create a compilation error.
Do you want to see the error?
I can add it. I don't remember it.

@vszakats
Copy link
Member

Add msvc msh3 job Fix compilation warning / error on test: Unreachable code. Test 1014 failing. Should fix.
Http2 don't compile with msh3. Is it intention?

The nghttp2 package is not installed, isn't that the reason?

No. I intentionally not install it. When I will add it, it will create a compilation error. Do you want to see the error? I can add it. I don't remember it.

Yep, I don't know much about MSH3 (tried building it once on non-Windows and failed), but on a quick glance it looks like a library that is a QUIC/H3 backend but not a general TLS backend, for H2 or H1 to use.

So it seems by design, but someone please correct me if wrong.

@talregev
Copy link
Contributor Author

tried building it once on non-Windows and failed

I can build it with vcpkg on non windows platform as well (linux, osx, other ...)

@vszakats
Copy link
Member

Ah OK, thanks, it's always useful to include the actual failures.

...these ones are known, and pending fixes in this PR: #14815

@talregev
Copy link
Contributor Author

Ah OK, thanks, it's always useful to include the actual failures.

...these ones are known, and pending fixes in this PR: #14815

Thanks! I added as depend PR.

@bagder
Copy link
Member

bagder commented Sep 15, 2024

looks like a library that is a QUIC/H3 backend but not a general TLS backend, for H2 or H1 to use.

msh3 is a HTTP/3 library (that uses msquic). It does no other HTTP version.

@vszakats vszakats changed the title GHA/windows: add msvc msh3 job GHA/windows: add MSVC MSH3 job Sep 15, 2024
@talregev
Copy link
Contributor Author

talregev commented Sep 16, 2024 •
edited
Loading

@vszakats Can you fix test 1014 as well?

I found #14927

@vszakats vszakats mentioned this pull request Sep 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vszakats vszakats vszakats left review comments

Assignees
No one assigned
Labels
CI Continuous Integration tests
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@talregev @vszakats @bagder