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

Add a Windows build target to Travis checks. #208

Merged
merged 39 commits into from
Jun 4, 2020

Conversation

dbaarda
Copy link
Member

@dbaarda dbaarda commented Jun 1, 2020

This adds a windows build target to the Travis checks.

Unfortunately I couldn't find a MSVC compatible version of libpopt that could be easily installed in the Travis build image, so this cannot build and test rdiff. This may be an argument to migrate to a more widely used argument parser like GNU's gengetopt, which I believe is available for Windows on Travis via chocolatey in GnuWin.

This revealed some extra compatibility problems and compiler warnings from MSVC, some of which were genuine bugs that needed fixing. Many of these would have been found by gcc/clang's -Wconversion warnings, but they can be too noisy to turn on permanently for the old gcc used by Travis. The subset of -Wsign-conversion warnings are too spammy even for modern gcc/clang. I've made everything -Wconversion -Wno-sign-conversion compliant when using gcc 9.3.0 or clang 7.0.0, and enabled them in cmake for clang only.

Bugs/Fixes included are;

  • Turn on -Wconversion -Wno-sign-conversion for clang and make all code compile clean.
  • Configured cmake for MSVC compiler flags and made all code compile clean on Windows.
  • Added cmake checking for io.h needed by MSVC for fileutil.c.
  • Improved fileutil.c for MSVC, including io.h and preferring _fileno() to stop warnings.
  • Fix broken error handling in rs_file_copy_cb() in fileutil.c.
  • Improved trace output, making it less spamy and more consistent.
  • Fix patch.c regression added in Fix patch bug for zero-length copies and improve bad copy_cb() handling. #206 that would break patching large files on 32bit architectures.
  • Add checking deltas for invalid literal lengths of 0 or greater than SIZE_MAX.
  • Tidy stream.h, removing unimplemented/unused functions and improving argument types.
  • Delete stream.c, moving last rs_buffers_copy() to rs_tube_copy_from_stream() in tube.c.
  • Remove not-required LIBRSYNC_EXPORT statements from trace.c.

dbaarda added 14 commits June 1, 2020 13:46
Using cmake works regardless of what build system is chosen on the different
target platforms. In particular, this should make it work for Windows builds.
This is the only place we have LIBRSYNC_EXPORT on function definitions in *.c
files and they shouldn't be there, as they break compiling tests on windows.

Note the introduction of netint_test is the first test that links trace.c, so
is the first time we've noticed this failure on Windows.
This is necessary for build systems that support multiple configurations, like
Windows.
It seems passing `--config Debug` to cmake in .travis.yml didn't work. Instead
we need to pass `-C Debug` to ctest in CMakeLists.txt to make it work on
Windows.
Add explicit typecasts whenever truncating or upsizing values. Add 'U' to any
literals that were intended to be unsigned.
This silences MSVC warnings about constant overflows from using a multiply.
Add explicit typecasts for truncating or expanding values. This silences MSVC
warnings about possible loss of data.
In job.h change copy_len and write_len to size_t from rs_long_t and int.

In stream.[hc] and tube.c remove unimplemented/unused rs_buffers_is_empty(),
rs_check_tube(), and rs_buffers_check_exit(). Change all len arguments to
size_t. Change rs_tube_catchup() return to rs_result from int. Tidy code order
in header to reflect order in code. Make trace output more consistent.
This closes out a TODO for this. Move big description comment from stream.c to
stream.h. Move rs_buffers_copy() from stream.c to rs_tube_copy_from_stream()
in tube.c. Make rs_tube_catchup_write()'s trace output a little more
consistent. Make rs_tube_copy_from_scoop() do nothing when there's no input
our output space.
Add 2 explicit conversions where trucation is desired behavour.
Copy link
Contributor

@sourcefrog sourcefrog left a comment

Choose a reason for hiding this comment

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

LGTM

dbaarda added 15 commits June 3, 2020 11:41
This makes the structure and trace output of rs_tube_catchup_write() more
consistent with the tube.c copy handling functions.
Fix s_patch_s_copying() to use rs_long_t for the request copy length, and make
it -Wconversion compliant. This would have broken long file support on 32bit
platforms.

Improve trace output from rs_patch_s_cmdbyte(). Make rs_patch_s_params() more
-Wconversion compliant by using size_t for a buffer length.

Change prototab.h len_1 and len_2 fields from size_t to int, which makes
various code more -Wconversion compliant (netint.c use int for lengths of
these parameters).
This ensures that Windows compiles don't complain about export stuff when
compiling this test.
This ensures literal cmds that are 0 or greater than size_t will abort with an
error instead of potentially corrupting the output.
This silences the last warning about hashtable.[ch] when building on windows.
Add `/D_CRT_SECURE_NO_WARNINGS` to MSVC CFLAGS to turn off warnings about
posix functions.

Also add optional commented out `-Wconversion` and `-Wno-sign-conversion`
flags for gcc and clang for easy experimenting with more warnings.
This silences the last warning on Travis when compiling with MSVC.
This silences the last warnings for stats.c when compiling with MSVC.
This silences the last warnings about sumset.[ch] compiling with MSVC.

Note this still has `-Wsign-conversion` warnings, but they are annoyingly
spurious, and nearly all about mixing size_t with signed ints.
On window io.h is needed for _setmode(), and it has both fileno() and
_fileno() but warns that fileno() is deprecated.
Apparently fread() returns a size_t which can never be negative, so returning
-1 is not how it indicates an error. Instead you should use ferror() to check
if there was an error.

This also makes fileutil.c `-Wconversion -Wno-sign-conversion` compliant.
@dbaarda
Copy link
Member Author

dbaarda commented Jun 4, 2020

This is finally ready to merge. In the interest of preventing myself from fiddling with it any further I'm going to merge it now.

@dbaarda dbaarda merged commit d1938c3 into librsync:master Jun 4, 2020
@dbaarda dbaarda deleted the travis-windows1 branch June 4, 2020 13:03
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