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: speed up warning option detection #964

Merged
merged 8 commits into from Oct 14, 2023

Conversation

vszakats
Copy link
Contributor

@vszakats vszakats commented Oct 9, 2023

This patch makes CMake configuration faster by dropping feature checks
for each individual "picky" C/C++ compiler warning options and instead
enable them based on compiler version. It also allows to add options using
the old, feature-check method.

I've implemented this for libssh2 [1] and curl [2] earlier this year.
This patch is based on those, by adapting it to the warning options used
by ngtcp2.

It brings CMake configuration time down from 15 to 5 seconds on my local
machine.

Special mention for -Wextended_offsetof: I could find no evidence that
this warning option was ever implemented in clang (or gcc), so this patch
deletes it.

[1] libssh2
libssh2/libssh2@ec0feae
libssh2/libssh2#952

[2] curl
curl/curl@9c543de
curl/curl#10973

If this is welcome, I have a similar patch for nghttp2 and nghttp3 as well.

@vszakats vszakats force-pushed the cmake-fast-warnopt-detection branch from 4aa49c6 to 8f44279 Compare October 9, 2023 11:46
@vszakats vszakats force-pushed the cmake-fast-warnopt-detection branch from 0afcb97 to 1679e9f Compare October 9, 2023 12:07
@tatsuhiro-t
Copy link
Member

Thank you for PR. Could you change the license to MIT and rpelace the license header with the following one?

# ngtcp2
#
# Copyright (c) 2023 ngtcp2 contributors
#
# Permission is hereby granted, free of charge, to any person obtaining
# a copy of this software and associated documentation files (the
# "Software"), to deal in the Software without restriction, including
# without limitation the rights to use, copy, modify, merge, publish,
# distribute, sublicense, and/or sell copies of the Software, and to
# permit persons to whom the Software is furnished to do so, subject to
# the following conditions:
#
# The above copyright notice and this permission notice shall be
# included in all copies or substantial portions of the Software.
#
# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
# EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
# MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
# NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
# LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
# OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
# WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

@vszakats
Copy link
Contributor Author

I've updated the licenses.

@tatsuhiro-t tatsuhiro-t added this to the v1.0.0 milestone Oct 14, 2023
@tatsuhiro-t tatsuhiro-t merged commit 8d4adb3 into ngtcp2:main Oct 14, 2023
22 checks passed
@tatsuhiro-t
Copy link
Member

Thank you! Merged now.

@vszakats vszakats deleted the cmake-fast-warnopt-detection branch October 14, 2023 09:36
vszakats added a commit to vszakats/ngtcp2 that referenced this pull request Oct 23, 2023
Previously these were were included transitively via
`cmake/ExtractValidFlags.cmake`. ngtcp2#964 deleted that file. The
replacements it added pulled these includes after the local
code uses them.

Re-add these includes into the `CMakeLists.txt` that uses them.

Fixes ngtcp2#979
vszakats added a commit to vszakats/ngtcp2 that referenced this pull request Oct 23, 2023
Previously these were were included transitively via
`cmake/ExtractValidFlags.cmake`. ngtcp2#964 deleted that file. The
replacements it added pulled these includes after the local
code uses them.

Re-add these includes into the `CMakeLists.txt` that uses them.

Reported-by: fpliu
Fixes ngtcp2#979
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants