Add more compilers to CI and increase FMT_PEDANTIC warning levels #736

eliaskosunen · 2018-05-11T18:26:54Z

In reference to #730

This PR contains:

Add the following compilers/environments to Travis (all on Linux with Debug configuration):

clang 6.0 with C++14
clang 4.0 with C++11
g++ 4.8 with C++11
g++ 4.4 with C++11

Add Visual Studio 2013 and 2015 to Appveyor, while removing Release builds to decrease build time
Include a lot more warnings in FMT_PEDANTIC, see CMakeLists.txt for more information
Add FMT_WERROR, which is now used on Travis but not on Appveyor
Fix every warning on g++ and clang, either by explicitly ignoring them through compiler pragmas or actually fixing them
Restructure appveyor.yml and remove appveyor-build.py

Rationale behind some of the changes:

Clang is a major compiler on Linux, and is not equivalent to Apple clang. Dismissing it entirely would be silly. g++ 4.8, because it's the default compiler on Trusty. Also, there are now also tests with C++11, which I think is beneficial.
There are now 6 build jobs on Appveyor. That number would increase to 12 should release builds be added back. I think it's more important to test multiple platforms rather than multiple build configurations that really only change the compiler optimization level. Because the free Travis version only allows 4 build jobs to be run in parallel, doing the same thing there would decrease the total number of jobs from 10 to 8 and thus decrease build time significantly, but I'm not sure if we want to do that
-
Not used on Appveyor because there are plenty of warnings yet to be fixed
-
This way we'll see the actual build messages in the dedicated Appveyor interface

(Potentially) blocking issues before merging:

The recent ranges-PR Add support for ranges, containers and types tuple interface... #735 doesn't support C++11 and causes build errors
Build errors on g++ 4.8 and 4.4
Build errors on Visual Studio 2013
Test failures on all Visual Studio versions over at Appveyor
Warnings on Visual Studio
I'm not sure which g++ and clang versions introduced which warnings, so adding/suppressing these could lead to errors on compiler versions that aren't tested

eliaskosunen · 2018-05-11T18:38:54Z

Testing with C++98 could also be useful.

vitaut

Thanks a lot for the PR! I think it's a nice improvement over the old config, but I'm a bit concerned about many new ifdefs to suppress warnings in the code. I suggest removing these for now together with noisy warnings and discuss whether they need to be enabled separately on a one-by-one case. And a few more comments inline.

vitaut · 2018-05-12T14:05:51Z

.travis.yml


 matrix:
  exclude:
-    - os: osx
-      env: BUILD=Doc
+    - env: TRAVIS_EMPTY_JOB_WORKAROUND=true


What is this for?

When creating a Travis build matrix, that is sometimes necessary. I just forgot it there. Fixed.

vitaut · 2018-05-12T14:07:23Z

.travis.yml

+      # Documentation
+    - env: CXX_COMPILER=g++-6 BUILD=Doc
+      sudo: required
+      compiler: gcc


CXX_COMPILER and compiler are probably unneeded for building docs

vitaut · 2018-05-12T14:11:16Z

.travis.yml

-    - g++-6
+install:
+    - DEPS_DIR="${TRAVIS_BUILD_DIR}/deps"
+    - mkdir -p ${DEPS_DIR} && cd ${DEPS_DIR}


Where is DEPS_DIR used?

Nothing, removed.

vitaut · 2018-05-12T14:16:23Z

include/fmt/core.h

@@ -30,10 +30,10 @@
 # define FMT_HAS_INCLUDE(x) 0
 #endif

-#if defined(__GNUC__) && !defined(__clang__)
-# define FMT_GCC_VERSION (__GNUC__ * 100 + __GNUC_MINOR__)


Please bring FMT_GCC_VERSION back as it is used in this header.

It's also used in format.h before the inclusion of core.h, but sure.

vitaut · 2018-05-12T14:19:10Z

include/fmt/core.h

+#if FMT_GCC_VERSION >= 406
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wuseless-cast"
+#endif


I suggest either fixing the warning or not setting -Wuseless-cast in CMake config if the warning is too noisy. Similarly in other cases since it's better not to pollute the code with numerous ifdefs for spurious warnings.

Not enabling the warning during testing is only ignoring the problem. People may and will have that warning enabled when including fmt, and they'll have to resort to similar pragma-trickery in order to silence the compiler about issues they have no control over. It's better to explicitly disable a single warning for a small piece of code.

For that specific case, though, fixing the warning should be possible with ease. I don't think the same can be said for the other instances.

vitaut · 2018-05-12T14:30:50Z

support/appveyor.yml

-configuration:
-  - Debug
-  - Release
+configuration: Debug


I think we should build both in Debug and Release modes.

That'll increase the build time from ~30 mins to an hour (6 -> 12 jobs, 5 min/job), but sure. I think we should look into some ways to decrease this time.

vitaut · 2018-05-12T14:33:02Z

support/appveyor.yml


-build_script:
-  - python support/appveyor-build.py


Please keep the Python script, since it's easier to deal with configs in one language and we might want to reintroduce MinGW at some point.

I agree that the Python script was much more pleasant to work with. By using the Appveyor's built-in build functionality, we can have build messages and test results reported in a more aesthetically pleasing (tm) way, as you can see here. I'll look into incorporating the Python script back, prioritizing bringing it back over the PowerShell trickery.

vitaut · 2018-05-12T14:35:50Z

test/gtest-extra-test.cc

@@ -195,33 +195,39 @@ TEST(ExpectSystemErrorTest, DoesNotGenerateUnreachableCodeWarning) {
 }

 TEST(AssertionSyntaxTest, ExceptionAssertionBehavesLikeSingleStatement) {
-  if (::testing::internal::AlwaysFalse())
+  if (::testing::internal::AlwaysFalse()) {


I don't think it's necessary, just adds syntactic noise.

Fix these warnings

Fix (some) of the compiler errors with them

Increase warning level on MSVC when compiling with FMT_PEDANTIC

Formatting

Fix some of the MSVC warnings Implement C++11 integer_sequence

eliaskosunen · 2018-05-15T16:55:15Z

How serious are we about C++98 support or older compilers? Reading the docs I got the impression that C++98 would be dropped for 5.0, is this true? What comes to older compilers, g++ 4.8 and 4.4 and Visual Studio 2013 builds are currently failing.

mwinterb · 2018-05-15T17:29:08Z

Rvalue references, variadic templates, decltype, trailing return types, and deleted functions all seem unconditionally required in the master branch. Unrestricted unions are sort-of required (there's a fallback to struct for GCC 4.4 only).
Feature-wise it seems like VS2015 (unrestricted unions, 2013 for variadic templates), GCC 4.4 (trailing return types), and Clang 3.0 (deleted functions) are probably the minimum baselines for the "big three".
I doubt supporting a compiler without variadic template support is something that's going to come back in v5.

eliaskosunen · 2018-05-15T19:49:07Z

It looks like that's indeed the case. The documentation should definitely be updated prior to release, as it claims support for VS 2010 and C++98 with minor restrictions.

vitaut · 2018-05-16T13:26:49Z

How serious are we about C++98 support or older compilers? Reading the docs I got the impression that C++98 would be dropped for 5.0, is this true?

C++98 is only supported on the 4.x branch. The master branch and the forthcoming 5.0 release requires some C++11 features such as variadic templates and type traits.

Rvalue references, variadic templates, decltype, trailing return types, and deleted functions all seem unconditionally required in the master branch. Unrestricted unions are sort-of required (there's a fallback to struct for GCC 4.4 only). Feature-wise it seems like VS2015 (unrestricted unions, 2013 for variadic templates), GCC 4.4 (trailing return types), and Clang 3.0 (deleted functions) are probably the minimum baselines for the "big three". I doubt supporting a compiler without variadic template support is something that's going to come back in v5.

You are completely right. I plan to update the docs before the next release.

eliaskosunen · 2018-05-16T15:44:05Z

As of now, ranges.h requires C++14 for its use of generic lambdas. This is breaking the builds using C++11. There should probably be a way to exclude range tests during build configuration or to fix ranges.h to only use C++11 features.

vitaut · 2018-05-17T03:53:23Z

As of now, ranges.h requires C++14 for its use of generic lambdas.

We can temporarily exclude ranges-test.cc from the build until C++11 compatibility is fixed.

ukreator · 2018-05-17T09:38:50Z

Is gcc 4.4 support really necessary for 5.0? It's 9 years old already

eliaskosunen · 2018-05-17T10:09:49Z

To me it seems like this would be ready for merging. Are you still concerned of the disabled warnings?

vitaut · 2018-05-19T12:59:10Z

Is gcc 4.4 support really necessary for 5.0? It's 9 years old already

I think gcc 4.4 achieves a good balance between being old enough to be available even on legacy systems, but new enough to provide most important C++11 features that we need.

Are you still concerned of the disabled warnings?

Yes. I'm happy to merge the CI improvements, but IMHO explicitly disabling warnings in the code adds too much noise for little value since warnings not included in -Wall often give too many false positives. Could you remove additional diagnostic #pragmas from the code and corresponding switches that trigger them from CMake? I'm open to discussing enabling specific warnings, but not wholesale and better in separate PRs.

vitaut · 2018-05-20T16:09:47Z

Compilation on gcc 4.4 should be fixed now.

Fix C++ standard setting in CI

mwinterb · 2018-05-23T18:36:09Z

include/fmt/ranges.h

-template <size_t... Is, class Tuple, class F>
-void for_each(std::index_sequence<Is...>, Tuple &&tup, F &&f) noexcept {
+// Check for integer_sequence
+#if defined(__cpp_lib_integer_sequence) || FMT_MSC_VER >= 1910


Why 1910 for MSVC? cppreference lists that it should be in VS2015, which is 1900. https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros

vitaut

Looks great, but there are some errors on Travis and a few small comments.

vitaut · 2018-05-26T14:44:10Z

appveyor.yml

@@ -0,0 +1 @@
+support/appveyor.yml


I don't think this is necessary. appveyor.yml is intentionally placed in support to reduce clutter in the top-level directory.

vitaut · 2018-05-26T14:52:08Z

.travis.yml

+    - env: BUILD=Doc
+      sudo: required
+      # g++ 6 on Linux with C++14
+    - env: CXX_COMPILER=g++-6 BUILD=Debug STANDARD=14


Why not use CXX instead of CXX_COMPILER and simplify before_script?

Travis exports CXX as g++ in between of matrix and before_script, so the value set in matrix would be lost.

vitaut · 2018-05-26T14:54:33Z

include/fmt/ranges.h

+
+    static FMT_CONSTEXPR std::size_t size() {
+        return sizeof...(N);
+    }


Please use 2-space indent for consistency (https://github.com/fmtlib/fmt/blob/master/CONTRIBUTING.rst).

vitaut · 2018-05-26T14:55:33Z

support/appveyor-build.py

    if platform == 'x64':
        generator += ' Win64'
    cmake_command.append('-G' + generator)
+    # build_command = ['msbuild', r'C:\projects\fmt\build\fmt.sln', '/m:4',
+    #        r'/logger:"C:\Program Files\AppVeyor\BuildAgent\Appveyor.MSBuildLogger.dll"']


vitaut · 2018-05-26T14:55:57Z

support/appveyor-build.py

    build_command = ['cmake', '--build', '.', '--config', config, '--', '/m:4']
-    test_command = ['ctest', '-C', config]
+    test_command = ['ctest', '-C', config, '-T', 'Test']
+    with open('DartConfiguration.tcl', 'w+') as file:


What is it for?

vitaut · 2018-05-26T14:57:41Z

support/appveyor-test-upload.ps1

+$file = $(ls Testing\*\Test.xml) | Select -first 1
+$XSLInputElement.Transform((Resolve-Path $file), (Join-Path (Resolve-Path .) "ctest-to-junit-results.xml"))
+$wc = New-Object 'System.Net.WebClient'
+$wc.UploadFile("https://ci.appveyor.com/api/testresults/junit/$jobid", (Resolve-Path .\ctest-to-junit-results.xml))


Is this file a debugging artefact? Does it need to be in the PR?

eliaskosunen · 2018-06-05T11:12:59Z

Sorry for taking so long with this. I was busy with my finals, but now that my summer vacation has begun, I was able to take another look at this.

Builds are now succeeding, except for VS 2013 and g++ 4.4. I feel like this would now finally be ready for merging.

Re-enable ranges-test Fix a Visual Studio error about function not returning a value in printf.h Fix a bug in .travis.yml

vitaut · 2018-06-06T19:14:48Z

Merged with minor tweaks, thanks!

vitaut requested changes May 12, 2018

View reviewed changes

vitaut force-pushed the master branch from 951b743 to dece85b Compare May 12, 2018 20:58

eliaskosunen added 6 commits May 14, 2018 08:18

Add a _lot_ more warnings to FMT_PEDANTIC

22e42a6

Fix these warnings

Add more compilers to CI

97e1271

Fix (some) of the compiler errors with them

Enable -Werror on CI

5d7103a

Increase warning level on MSVC when compiling with FMT_PEDANTIC

Add VS 2013 and 2015 to Appveyor

b49acb3

Fix Appveyor tests

710d078

Formatting

Implement requested changes

58991c7

Fix some of the MSVC warnings Implement C++11 integer_sequence

Reintroduce appveyor-build.py

ed839c4

Remove ranges-test from tests

a8976bd

Merge branch 'master' into ci

194616e

Remove (some) explicit warning suppressions

584a99c

Fix C++ standard setting in CI

eliaskosunen mentioned this pull request May 22, 2018

VisualStudio 15.7.x compilation error in /W4 /WX #748

Closed

eliaskosunen added 3 commits May 23, 2018 13:56

Merge branch 'master' into ci

01fe59d

Remove (some) explicit warning suppressions

8d27036

Fix C++ standard setting in CI

Fix test builds with C++11

16533b2

mwinterb reviewed May 23, 2018

View reviewed changes

vitaut requested changes May 26, 2018

View reviewed changes

Enable pedantic warnings on tests

6ba5f70

eliaskosunen added 3 commits June 5, 2018 02:11

Merge branch 'master' into ci

739af47

Fix warnings from edits to master

25f9b09

Cleanups

d1976cf

Add C++11 support to ranges.h

d79b096

Re-enable ranges-test Fix a Visual Studio error about function not returning a value in printf.h Fix a bug in .travis.yml

vitaut approved these changes Jun 6, 2018

View reviewed changes

vitaut merged commit 691a7a9 into fmtlib:master Jun 6, 2018

eliaskosunen mentioned this pull request Jun 8, 2018

Fixed GCC version test. #771

Closed

eliaskosunen mentioned this pull request Nov 5, 2018

Inconsistent warning flags specified in CMakeLists.txt for different compilers (possibly by mistake) #928

Closed

Add more compilers to CI and increase FMT_PEDANTIC warning levels #736

Add more compilers to CI and increase FMT_PEDANTIC warning levels #736

Conversation

eliaskosunen commented May 11, 2018 • edited Loading

eliaskosunen commented May 11, 2018

vitaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliaskosunen May 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliaskosunen commented May 15, 2018

mwinterb commented May 15, 2018

eliaskosunen commented May 15, 2018

vitaut commented May 16, 2018

eliaskosunen commented May 16, 2018

vitaut commented May 17, 2018

ukreator commented May 17, 2018

eliaskosunen commented May 17, 2018

vitaut commented May 19, 2018

vitaut commented May 20, 2018

Choose a reason for hiding this comment

vitaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliaskosunen commented Jun 5, 2018 • edited Loading

vitaut commented Jun 6, 2018

eliaskosunen commented May 11, 2018 •

edited

Loading

eliaskosunen May 13, 2018 •

edited

Loading

eliaskosunen commented Jun 5, 2018 •

edited

Loading