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

GHA: add --enable-werror to the quiche job #14041

Closed
wants to merge 3 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Jun 27, 2024

No description provided.

@bagder bagder added the CI Continuous Integration label Jun 27, 2024
... from ‘int’ to ‘curl_uint64_t’
@bagder bagder added the HTTP/3 h3 or quic related label Jun 27, 2024
Comment on lines 295 to +297
--with-openssl=/home/runner/quiche/quiche/deps/boringssl/src
--enable-debug
--enable-werror
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--with-openssl=/home/runner/quiche/quiche/deps/boringssl/src
--enable-debug
--enable-werror
--enable-warnings --enable-werror --enable-debug
--with-openssl=/home/runner/quiche/quiche/deps/boringssl/src

Would it make sense to also level the warnings to the other jobs?

Copy link
Member

@vszakats vszakats Jun 27, 2024

Choose a reason for hiding this comment

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

Well, --enable-warnings might be the default — ./configure defaults confuse me every single time. In that case the patch above may just sync the options visually with the other jobs, for clarity!

Copy link
Member Author

Choose a reason for hiding this comment

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

The default is complicated because it is not a clean on/off; there's logic.

But the main point here is perhaps that --enable-debug enables warnings.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I still think it'd be more readable to use the same line for all jobs here. Even if functionally the same.

@bagder bagder closed this in 7fce488 Jun 27, 2024
bagder added a commit that referenced this pull request Jun 27, 2024
... from ‘int’ to ‘curl_uint64_t’

Closes #14041
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration HTTP/3 h3 or quic related
Development

Successfully merging this pull request may close these issues.

None yet

2 participants