Add CI for Windows #82

Flamefire · 2019-07-29T15:05:55Z

As some Windows code might be different it is worth testing. Easiest solution would be to add

os: 
  - linux
  - windows

to travis.yml, but I dind't want to build all configurations.

Note: Dependencies are a PITA on Windows. Hence I didn't install FFTW or libsndfile, although I tried at least for FFTW but failed. Possible solution: Use vcpkg and CMAKE_TOOLCHAIN_FILE or try Mingw for building and download and copy prebuild binaries. The former requires compilation of FFTW (slow), the latter knowledge where binaries will be placed on builds to copy DLLs to). So not great either.

erikd · 2019-07-29T23:11:20Z

CMakeLists.txt

@@ -10,6 +10,7 @@ endif()
 option(LIBSAMPLERATE_TESTS "Enable to generate test targets" ${IS_ROOT_PROJECT})
 option(LIBSAMPLERATE_EXAMPLES "Enable to generate examples" ${IS_ROOT_PROJECT})
 option(LIBSAMPLERATE_INSTALL "Enable to add install directives" ${IS_ROOT_PROJECT})
+option(LIBSAMPLERATE_ENABLE_WARNINGS "Enable compiler warnings and fail compile if any triggers" OFF)


I think disabling warnings by default is a bad idea. I agree they should never be promoted to errors by default, but all warnings should be visible.

Even if warnings were enabled by default I would question the wisdom of being able to disable them.

Then I propose 2 options: LIBSAMPLERATE_ENABLE_WARNINGS, LIBSAMPLERATE_ENABLE_WERROR. Alternatively just the latter.

Reason for having an option with default OFF is that I set those as INTERFACE option which enables them on all targets linking to this. If I remove LIBSAMPLERATE_ENABLE_WARNINGS and just add LIBSAMPLERATE_ENABLE_WERROR then I'd add warnings per target (PRIVATE for samplerate, tests and examples can be PUBLIC, much easier with #69 as there is a common library used by all tests/examples respectively, so only repeat them 3 times)

BTW: There are some warnings (especially on Windows) about float->double conversions which I disabled here. Better would be to declare constants with f suffix and add explicit casts to out[k] = ... assignments which also makes the conversion visible and is good IMO, but you said you want to avoid casts.

Having LIBSAMPLERATE_ENABLE_WERROR would mirror the autotool config option --enable-werrror.

There are some warnings (especially on Windows) about float->double conversions

I think there is a MSVC #pragma to disable those.

So drop LIBSAMPLERATE_ENABLE_WARNINGS and add LIBSAMPLERATE_ENABLE_WERROR?

I think there is a MSVC #pragma to disable those.

So add #pragma push/disable/pop to the code instead of conversions? I'd advice against that but your call

Yes, enable warnings by default, enable -Werror as an option.

Yes, I would prefer the #prama fix than adding casts.

Done. As warnings are now always enabled I check that the compiler does not error out if the flag is passed (check_c_compiler_flag)

Also done. Used the MSVC definition to only add them for MSVC to avoid e.g. -Wunknown-pragma warnings. I'm not particularly fond of suppressing the warnings as IMO narrowing conversions shouldn't be done implicitly as loss of data happens.

erikd · 2019-08-10T09:51:10Z

.travis.yml

@@ -31,4 +31,4 @@ script:
  - cd build
  - cmake -DBUILD_SHARED_LIBS=$CMAKE_SHARED -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_C_FLAGS="-Wall -Wextra -Werror" ..
  - cat config.h
-  - make && make test
+  - cmake --build . --config $BUILD_TYPE && ctest --output-on-failure -C $BUILD_TYPE


That disables the autotool build in favor of the CMake build. Not a good idea.

No. The autotool build is still done but in a completely different job: https://github.com/erikd/libsamplerate/pull/82/files#diff-354f30a63fb0907d4ad57269548329e3R16

Ok, the way this travis file looks now, it seems like CMake is the default, when it is not. Autotools is the default.

First: Why? Isn't CMake superior to autotools? It can generate buildsystems on any OS.

In this case it kinda has to be this way: All paths (windows, linux, optimized, non-optimized) go to the "default" script tag so the code there has to be the os-agnostic, generic code. Doing it differently in an attempt to make the autotools job more prominent would obfuscate the code (e.g. use yaml anchors)

Isn't CMake superior to autotools?

No, I don't think so and I know I am not alone.

I have used Autotools for 20 years. Its not perfect, but I understand it and know how to use it. I don't know CMake anywhere near as well, and from what I have seen of it, I do not think it is as good as Autotools.

I personally like CMake more, especially if i look at what is required for autotools: https://github.com/erikd/libsamplerate/blob/master/Makefile.am, https://github.com/erikd/libsamplerate/blob/master/autogen.sh, https://github.com/erikd/libsamplerate/blob/master/configure.ac compared to CMake https://github.com/erikd/libsamplerate/blob/master/CMakeLists.txt.

But I don't want to start a philosophic discussing about meta-buildsystems. So only the advantages of CMake in this case:

Generating build systems for windows (e.g. MSVC projects)

Use of the library as a git submodule without the need to install it from a CMake project via add_subdirectory. This also greatly reduces the pain of using this on Windows/cross-platform projects with all the dependencies on compilers, options, ...

But here all this seems to be void. Why talk about a "default"? Both work are thoroughly tested and a user can choose which one he wants/needs (if he isn't on windows in which case there is no choice)

erikd · 2019-08-10T09:52:31Z

src/src_sinc.c

@@ -31,9 +31,20 @@
 typedef int32_t increment_t ;
 typedef float	coeff_t ;

+#ifdef _MSC_VER
+#define MSVC_DISABLE_WARNING(n) __pragma(warning(push)) \
+								__pragma(warning(disable:n))


Can't this be done in CMake somewhere?

Yes, but that would be more work for no gain. MSVC guarantees that _MSC_VER is defined

Work: Add configured define to config.h if compiling with MSVC
Gain: Can use #if SRC_MSVC instead of the #ifdef _MSC_VER where the former would error on compile time if it is not set catching e.g. a typo in _MSC_VER

But: If _MSC_VER was mistyped then the warnings won't be disabled and hence CI would fail due to the warnings thrown. Therefore a mistyped macro name would already result in a compile-time error which is what is the goal of using #if over #ifdef, but using _MSC_VER is less work

I would vastly prefer to limit compiler specific specical case handling creeping in the code and if it does creep into the code it should be mininal. This approach does not meet those goals.

Oh yes, I could completely disable this warning in CMake if this is what you meant. I had done that initially: b487324#diff-af3b638bc2a3e6c650974192a53c7291R113
But earlier you said to use the pragma instead: #82 (comment)

My preferred approach would be to use the proper explicit casts and f suffix for float constants. This makes the truncation obvious which might be important knowledge when changing the code. This would also never required warning suppressions on any compiler

I added another commit which does just that: Explicit casts where truncation happens. This makes the compilers happy so no suppression required. 37473c2

Just choose which one you want:

warning suppression flag added in CMake

pragma based suppression

explicit handling of narrowing conversions (my preference)

Notes:

It might be worth checking if some of those calculations should really be done using double instead of float.

Loops are used instead of manual unrolling. The compiler is at least as good and in this case generates the same machine code with less code and room for error: https://godbolt.org/z/4a27qn

To kinda prove the above: The manually unrolled loops in calc_output_multi are suboptimal due to incorrect tail handling: The switch (ch % 8) has to be place on top of the do otherwise the condition and conditional jump will be executed on every iteration instead of once. I'd suggest to just use a loop and let the compiler decide.

Edit: The compiler-optimized loop is actually faster: http://quick-bench.com/znRud0mQhKSWDhgJns_5eYP8_wk

erikd · 2019-08-12T21:02:48Z

src/fastest_coeffs.h

@@ -21,2469 +21,2469 @@ static const struct fastest_coeffs_s
 } fastest_coeffs =
 {	128,
 {
- 8.31472372954840555082e-01,


Changing these really should not be necessary. These are three large tables that take half as much room as float as they would as double. Memory space is cheap nowadays, but it almost certainly is worth minimizing memory to CPU bandwidth.

I don't understand exactly. This is a float array (coeff_t coeffs [2464] ; with coeff_t == float)

My only change here is to make the truncation from double to float explicit because the compiler (rightfully) complained that this might be accidental.

complained that this might be accidental.

Its not accidental. The compiler should be informed in a way that doesn't change nearly every line in the file.

If a line is like floatVar = doubleValue which does look accidental then why isn't the correct change floatVar = floatValue? And if the whole file consists of such lines then you gotta change the whole file. Nothing wrong with that especially as it can be done with Search&Replace.

The other solution is a suppression via compiler specific pragmas but you objected them in #82 (comment). In the end it doesn't matter which way it is done: Either by passing float values to float variables or by passing double values to float variables and surrounding the region with a pragma saying "don't tell me about truncating conversions". Just needs a decision. See #82 (comment)

Flamefire · 2019-08-26T11:12:38Z

Rebased to current master. What is the state of this? Having CI for windows and with Werror is valuable.

Questions/answers asked previously:

autotools: still available as before, tested on CI, no change in this PR
truncation warnings for implicit double -> float on e.g. MSVC:
- 3 options provided: suppress globally, suppress locally or use proper constants and conversions
- First 2 implemented for MSVC only, last works on all compilers and is the more correct/robust solution
- see Add CI for Windows #82 (comment) and following
- last is the current option (last commit)

Use the cmake and ctest commands with explicit build type so they work with other generators like MSVC solutions

Useful for (and used in) CI Add some defines required to fix MSVC warnings about "deprecated" CRT functions

evpobr · 2020-10-20T07:57:38Z

I think we need to use GitHub Actions instead of Travis CI.

Flamefire · 2020-10-20T15:09:36Z

Why? I mean: This PR shows that travis is possible. Having said that I also like GHA due to nicer syntax although I haven't used the windows runners yet.

evpobr · 2020-12-03T10:57:17Z

GitHub actions intergated with Windows CI tests.

erikd reviewed Jul 29, 2019

View reviewed changes

Flamefire force-pushed the windows_ci branch 4 times, most recently from 6b7b6a6 to 9d09132 Compare July 30, 2019 12:21

Flamefire force-pushed the windows_ci branch from 9d09132 to ed8b07f Compare August 9, 2019 11:01

Flamefire mentioned this pull request Aug 9, 2019

Require C99 compiler #81

Merged

erikd requested changes Aug 10, 2019

View reviewed changes

Flamefire force-pushed the windows_ci branch from ed8b07f to 728b7db Compare August 11, 2019 16:05

erikd reviewed Aug 12, 2019

View reviewed changes

Flamefire mentioned this pull request Aug 19, 2019

README.md doesn't list all boost deps Return-To-The-Roots/s25client#1100

Closed

erikd force-pushed the master branch 2 times, most recently from 1b3d5b1 to c10c819 Compare August 25, 2019 20:59

Flamefire force-pushed the windows_ci branch 2 times, most recently from 1b44d37 to 8d8a941 Compare August 26, 2019 11:05

Flamefire force-pushed the windows_ci branch 3 times, most recently from f41f21b to a706853 Compare August 27, 2019 07:29

Flamefire mentioned this pull request Aug 27, 2019

Speed improvements #94

Closed

Flamefire force-pushed the windows_ci branch from a706853 to 5390659 Compare April 17, 2020 16:48

Flamefire added 3 commits July 5, 2020 15:21

Make travis build more independent of toolchain

f6775e2

Use the cmake and ctest commands with explicit build type so they work with other generators like MSVC solutions

Add LIBSAMPLERATE_ENABLE_WARNINGS to generically enable warnings

a99b88d

Useful for (and used in) CI Add some defines required to fix MSVC warnings about "deprecated" CRT functions

Add windows builds on travis

a3386cf

Flamefire force-pushed the windows_ci branch from 5390659 to 3e012f8 Compare July 5, 2020 13:23

Flamefire added 2 commits July 5, 2020 15:23

Supress warnings on MSVC

8a9ca7f

Use explicit conversions to suppress warnings

3e012f8

evpobr self-assigned this Dec 3, 2020

evpobr closed this Dec 3, 2020

Flamefire mentioned this pull request Jun 1, 2024

Add LIBSAMPLERATE_ENABLE_WARNINGS to generically enable warnings #217

Open

Flamefire deleted the windows_ci branch June 1, 2024 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CI for Windows #82

Add CI for Windows #82

Flamefire commented Jul 29, 2019

erikd Jul 29, 2019

Flamefire Jul 30, 2019

erikd Jul 30, 2019

Flamefire Jul 30, 2019

erikd Jul 30, 2019

Flamefire Jul 30, 2019

erikd Aug 10, 2019

Flamefire Aug 10, 2019

erikd Aug 11, 2019

Flamefire Aug 11, 2019

erikd Aug 11, 2019

Flamefire Aug 11, 2019

erikd Aug 10, 2019

Flamefire Aug 10, 2019

erikd Aug 11, 2019

Flamefire Aug 11, 2019

Flamefire Aug 12, 2019 •

edited

Loading

erikd Aug 12, 2019

Flamefire Aug 13, 2019

erikd Aug 13, 2019

Flamefire Aug 13, 2019

Flamefire commented Aug 26, 2019

evpobr commented Oct 20, 2020

Flamefire commented Oct 20, 2020

evpobr commented Dec 3, 2020

Add CI for Windows #82

Add CI for Windows #82

Conversation

Flamefire commented Jul 29, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flamefire Aug 12, 2019 • 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

Flamefire commented Aug 26, 2019

evpobr commented Oct 20, 2020

Flamefire commented Oct 20, 2020

evpobr commented Dec 3, 2020

Flamefire Aug 12, 2019 •

edited

Loading