lightvector / KataGo

GTP engine and self-play learning in Go
https://katagotraining.org/
Other
3.56k stars 564 forks source link

Compiler warning flags prevent compilation with g++ 5.3.1 (which should work) #398

Open steve-biggs-fox opened 3 years ago

steve-biggs-fox commented 3 years ago

Hello,

The compilation instructions for KataGo state: "It should compile on Linux or OSX via g++ that supports at least C++14". My Linux distribution come with gcc 5.3.1 and gcc has full support for C++14 from version 5.1.0 onward (and partial support in some 4.x versions). Therefore, I should be able to build KataGo. However, when I try to do so, the build fails with the following errors:

c++: error: unrecognized command line option ‘-Wnull-dereference’
c++: error: unrecognized command line option ‘-Walloc-zero’
c++: error: unrecognized command line option ‘-Wduplicated-branches’
c++: error: unrecognized command line option ‘-Wduplicated-cond’
c++: error: unrecognized command line option ‘-Wdangling-else’
c++: error: unrecognized command line option ‘-Wrestrict’

This is because these g++ warning flags first appeared in gcc version 6.1.0.

I could, of course, install a newer version of gcc. However, as anyone who has ever tried to build gcc will know, this is not a trivial task! Since my existing gcc version supports C++14 so should work, I would rather find a way to make that work. This is indeed possible via a workaround for now by just deleting those warning flags from cpp/CMakeLists.txt, and then re-running cmake and make. I have done this and the build appears to be working now (currently at 88%).

I would suggest that this is a bug. The KataGo build system should check for gcc version. If version < 5, fail with message to upgrade gcc to >= 5 to get C++14 support. If version >= 5 but < 6, then do not use those warning flags. If version >= 6, then use those warning flags (i.e. do what it does already). Alternatively, there could be an option to build without warnings. After all, compiler warning are more of a developer thing rather than a user thing, right?

Incidentally, does KataGo really use C++ language features that are only in C++14 onwards? Or could it in fact be compiled with earlier versions of gcc? Just because there's no point restricting compiler versions any more than is absolutely necessary as some people are just stuck with old versions and, as above, upgrading is not necessarily straightforward.

Thanks

lightvector commented 3 years ago

This sounds like an easy fix indeed - we can just edit the cmake configuration. Feel free to submit a pull request, or else I will push a fix myself this weekend.

I don't know for sure if it would work with C++11 or not. I'm not sure I care to officially support it though, as such a version gradually gets increasingly old and increasingly reasonable to expect users to have moved beyond, because it means increasing the work for me on both Windows and Linux to ensure that any change never breaks the build. Obviously, something that might make that easier would be if there were automated builds - it would be great if someone had advice on how to set that up.

steve-biggs-fox commented 3 years ago

Unfortunately I am very busy with other things so I do not have time to make a PR myself. But, as you say, should be a fairly straightforward fix.

Fair point about not wanting to officially support C++11. As per issue #397, it seems that older than C++11 definitely will not work due to changed function names in the C++ standard, while C++11 itself may or may not work. I guess anyone who is stuck with a C++11 compiler can try it to see if they can get away with it.

As for automated testing, there's some GitHub documentation here. Essentially, you need to:

  1. Code up some tests (unit tests, integration / whole system tests, static coding style tests, etc.) so that the test suite can be run automatically (looks like you have this already).
  2. Decide on some target environments (e.g. windows / linux, gcc / intel, different versions thereof, etc.) and develop a way of automatically creating a clean state of those target environments in a consistent way (e.g. using containers such as docker)
  3. Set up some GitHub CI "actions" that happen at specified times (e.g. when you push to certain branches, merge PRs, etc.) that will create the target environment(s) and run the test suite in those environments.

So a little bit of fiddling about to get it set up but totally worth it to ensure a quality software product.

To get started, the easiest thing is probably to just use the tests you have already, define a single target environment (maybe one that is similar to your development environment as you know the tests should work), and get the basic CI workflow set up. Once you have that, extending to other targets is relatively trivial.

lightvector commented 3 years ago

Pushed what is hopefully a fix to master. Let me know if I missed something or if it doesn't work for you, and if so, then I will fix some more. :)