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 MSVC warning level to 4 #1711

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@MarcelRaad
Member

MarcelRaad commented Jul 29, 2017

The MSVC warning level defaults to 3 in CMake. Change it to 4, which is
consistent with the Visual Studio and NMake builds. Disable level 4
warning C4127 for the library to get a clean CURL_WERROR build.

Also, /WX got added twice to the compiler flags for the release build and not
at all for the debug build. Fix this by using CMAKE_C_FLAGS instead of
CMAKE_C_FLAGS_RELEASE and CMAKE_C_FLAGS_DEBUG.

@mention-bot

This comment has been minimized.

mention-bot commented Jul 29, 2017

@MarcelRaad, thanks for your PR! By analyzing the history of the files in this pull request, we identified @billhoffman, @Sukender and @jzakrzewski to be potential reviewers.

@bagder

bagder approved these changes Jul 29, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jul 29, 2017

It seems this warning level causes numerous tests run by cmake to fail...

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 30, 2017

c:\projects\curl\lib\curl_setup_once.h(164): error C2061: syntax error: identifier 'Missing_definition_of_return_and_arguments_types_of_recv'

I can only imagine that my CMake configuration wasn't updated when I moved the warning suppression from the top-level CMakeLists.txt to the one in the lib directory and the recv detection code doesn't compile because of the warning. I'll try moving it back to the top-level directory.

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:cmake_msvc_warnings branch from 169be63 to 680d068 Jul 31, 2017

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 31, 2017

Still the same problem:

-- Performing Test curl_cv_func_recv_test - Failed
-- Performing Test curl_cv_func_send_test - Failed

Strange that it works on my machine (with the Ninja generator, though) but not on AppVeyor. :-(

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:cmake_msvc_warnings branch from 680d068 to 1ce6d2a Jul 31, 2017

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Jul 31, 2017

CMake: set MSVC warning level to 4
The MSVC warning level defaults to 3 in CMake. Change it to 4, which is
consistent with the Visual Studio and NMake builds. Disable level 4
warning C4127 for the library to get a clean CURL_WERROR build.

Closes curl#1711
@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 31, 2017

I could reproduce this now with the Visual Studio project file generator back-end (which I haven't got to build anything yet, but generating the CMake cache works). Strangely, it failed because of a level 1 warning with CURL_WERROR, so I split the commit fixing CURL_WERROR into new PR #1715 now. Maybe CMAKE_C_FLAGS is used for the configuration tests while CMAKE_C_FLAGS_DEBUG/CMAKE_C_FLAGS_RELEASE is not?

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:cmake_msvc_warnings branch from 1ce6d2a to 24a58d7 Aug 1, 2017

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Aug 1, 2017

CMake: set MSVC warning level to 4
The MSVC warning level defaults to 3 in CMake. Change it to 4, which is
consistent with the Visual Studio and NMake builds. Disable level 4
warning C4127 for the library to get a clean CURL_WERROR build.

Closes curl#1711
@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Aug 1, 2017

What's that Travis failure again....

test 1208...[FTP PORT download, no data conn and no transient negative reply]
 1208: protocol FAILED:
--- log/check-expected	2017-08-01 16:20:14.530239179 +0000
+++ log/check-generated	2017-08-01 16:20:14.530239179 +0000
@@ -5,3 +5,4 @@
 TYPE I[CR][LF]
 SIZE 1208[CR][LF]
 RETR 1208[CR][LF]
+QUIT[CR][LF]

My mobile Chrome doesn't want to show the AppVeyor log, I'll check tomorrow at my desktop computer.

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:cmake_msvc_warnings branch from 24a58d7 to c53d665 Aug 2, 2017

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Aug 2, 2017

CMake: set MSVC warning level to 4
The MSVC warning level defaults to 3 in CMake. Change it to 4, which is
consistent with the Visual Studio and NMake builds. Disable level 4
warning C4127 for the library and test servers to get a clean
CURL_WERROR build as that warning is raised in some macros in older
Visual Studio versions.

Closes curl#1711
@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Aug 2, 2017

test 1399...[Curl_pgrsTime unit tests]

unit1399 returned 1, when expecting 0
 exit FAILED
== Contents of files in the log/ dir after test 1399
=== Start of file stderr1399
 URL: -
 is 1000002 us same as 1 seconds? Yes
 is 1000002 us same as 1 seconds? Yes
 is 3000966 us same as 3 seconds? No
 unit1399.c:99 Assertion 'usec_matches_seconds(data.progress.t_starttransfer, 3)' failed: about 3 second should have passed
=== End of file stderr1399
CMake: set MSVC warning level to 4
The MSVC warning level defaults to 3 in CMake. Change it to 4, which is
consistent with the Visual Studio and NMake builds. Disable level 4
warning C4127 for the library and additionally C4306 for the test
servers to get a clean CURL_WERROR build as that warning is raised in
some macros in older Visual Studio versions.

Closes #1711

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:cmake_msvc_warnings branch from c53d665 to 6a0d2ab Aug 2, 2017

@coveralls

This comment has been minimized.

coveralls commented Aug 2, 2017

Coverage Status

Coverage increased (+0.01%) to 75.335% when pulling 6a0d2ab on MarcelRaad:cmake_msvc_warnings into 7e48aa3 on curl:master.

@curl curl deleted a comment from coveralls Aug 2, 2017

@curl curl deleted a comment from coveralls Aug 2, 2017

@curl curl deleted a comment from coveralls Aug 2, 2017

@curl curl deleted a comment from coveralls Aug 2, 2017

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Aug 2, 2017

Completely unrelated Travis failure again, test 2033 [NTLM connection mapping, pipelining enabled] for the newly added libressl job this time.

AppVeyor is happy now, so this is the version I would push. Unfortunately there were more evil macros than I thought, even using SIG_ERR always results in a warning under MSVC 11 (2012) because of a direct cast from 32-bit integer constant to 64-bit function pointer within the macro.

@MarcelRaad MarcelRaad closed this in 866e029 Aug 3, 2017

@MarcelRaad MarcelRaad deleted the MarcelRaad:cmake_msvc_warnings branch Aug 3, 2017

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