TODO
* test GCC 5.4 on the grid (issue859)
* decide compiler versions
* update CPLEX find scripts (issue853)
* add native build options (issue857) 
* switch all buildbot configurations to new compilers
* set up modules for new GCC version on the grid
* test new GCC modules on the grid (issue859)
* update documentation in the wiki

If we get the GCC 8.2 setup working on the grid, I don't think we need to bother setting up GCC 5.4.

I just tried

module purge
module load GCC/8.2.0-2.31.1
module load CMake/3.9.5-GCC-8.2.0-2.31.1
module load matplotlib/2.2.3-foss-2018.08-Python-2.7.15

and compiling Fast Downward with CPLEX 12.5.1 works but prints many warnings of the type that you 
already mentioned:

/scicore/soft/apps/binutils/2.31.1-GCCcore-8.2.0/bin/ld.gold: warning: 
/infai/seipp/lib/cplex/CPLEX_Studio/cplex/lib/x86-64_sles10_4.1/static_pic/libcplex.a(netmain.o): 
section .rodata.str1.32 contains incorrectly aligned strings; the alignment of those strings won't 
be preserved

I don't know what to do about these warnings.

About the new module instructions:
- GCC 5.4: I used the foss/2016b package suggested by Thomas. I did not load any
specific Python or Matplotlib module, but we can ask Martin or Pablo to install
them. What I did was: 
  module purge
  module load foss/2016b
  module load CMake/3.9.6-foss-2016b

- GCC 8.2: I also did not include any Python module, but here we at least have
the Matplotlib one. There are also two available versions for CMake with 8.2 and
I used the newest one. What I did was:
  module purge
  module load foss/2018.08
  module load matplotlib/2.2.3-foss-2018.08-Python-2.7.15
  module load CMake/3.9.5-GCC-8.2.0-2.31.1

  
However, there is one issue: In my grid account, I have CPLEX 12.71 and these
new modules (either the ones for GCC 5.4 or 8.2) are using the ld.gold linker by
default, which triggers a lot of warnings regarding "incorrectly aligned
strings" in the CPLEX 12.71 code. I had to add a new flag to the configuration
on the "build_configs.py" script. This problem was also mentioned in issue859.
I changed from
  "-DCMAKE_CXX_FLAGS='-m64'"
to
  "-DCMAKE_CXX_FLAGS='-m64 -fuse-ld=bfd'"
in order to use the standard linker.
Since it is not ideal to modify the Fast Downward source code just because of
our own issues on the grid, I will have a look if we can change it in our
".bashrc" scripts or something similar.
It would be great if someone with another version of CPLEX on the grid could try
to reproduce this as well.

We should also update the "module load" instructions in the internal wiki which still use 
GCC 4.8 (https://wiki.dmi.unibas.ch/doku.php?id=fbi:ai:compute-cluster). Has someone 
already figured out the right combination of modules?

The wiki should be up to date now. I also updated the summary to show that all
items are done now.

> std::any would be nice to have but since g++ only adds this in version 7,
> I'd say we wait until the next time we raise the requirement.

Absolutely, I just added my opinion on the newer clang versions in case we look
at this again in the future.

> For this time, I think we are done, except for the documentation in the wiki.
> Issue859 is still open because we are interested in LAMA results, but from what
> I understood this is mostly curiosity and should not block this issue.
>
> For the documentation, I only found two pages that require an update. I didn't
> want to do the update before we agreed on the version, so here is a "pull
> request":

Looks good to me! Feel free to set this one as resolved and apply the wiki changes.

Many thanks! :-)

std::any would be nice to have but since g++ only adds this in version 7, I'd
say we wait until the next time we raise the requirement.

For this time, I think we are done, except for the documentation in the wiki.
Issue859 is still open because we are interested in LAMA results, but from what
I understood this is mostly curiosity and should not block this issue.

For the documentation, I only found two pages that require an update. I didn't
want to do the update before we agreed on the version, so here is a "pull request":

http://www.fast-downward.org/ObtainingAndRunningFastDownward
  * Under "Dependencies"
    change "GCC 4.8" to "GCC 5.4" 
    change "Clang 3.3/3.4" to "Clang 3.9"
    change "we recommend using GCC 4.8" to "we recommend using the latest GCC"
  * Under "Dependencies on Windows"
    change 2015 to 2017 (the link is still valid and can stay like this)
  * In section "Compiling on Windows"
    change 2015 to 2017
    update the path to
      "C:\Program Files (x86)\Microsoft Visual
Studio\2017\Community\VC\Auxiliary\Build\vcvarsall.bat"
  * In CMake examples under Manual Builds
    change -G"Visual Studio 14 2015" to -G"Visual Studio 15 2017" 
    change "clang-3.5" to "clang-3.9"
    update examples for HomeBrew and MacPorts
      (though I don't know how we should find out the updated paths)

http://www.fast-downward.org/ForDevelopers/CodingConventions
  * change "features supported by GCC 4.8" to "features supported by GCC 5.4,
clang 3.9, and MSVC 19.15".

In case we revisit this for our next language requirement bump: I'm lukewarm
about the clang-3.9 changes (I looked at the general list, not just Jendrik's
examples). I don't think init-statements for if and switch are that useful.

However, the new features in clang-4.0 would be quite nice eventually. We could
use std::any in the option parser to get rid of our own version of any,
std::optional could be useful in quite a few places, and 4.0 also adds
structured bindings, the C++ equivalent of tuple unpacking in Python, which
would make it less tedious to use functions returning pairs.

In my opinion, these are the most interesting feature-additions in Clang 3.9, 4.0 
and 5.0 (https://en.cppreference.com/w/cpp/compiler_support):

clang-3.9: init-statements for if and switch
clang-4.0: std::any, std::optional, std::variant
clang-5.0: std::byte

However, we'll only be able to use them once we require GCC 7 and a newer 
AppleClang version. Therefore, I think clang-3.9 is cuurrently the best choice 
after all.

I don't see a strong connections between our clang-tidy calls and what we
requirements we want to impose on people to build the planner. Just because we
use a more modern version as a linter doesn't mean we have to lock out people
who use an older version.

Is there a practial advantage of raising our clang minimum version requirement
to 5.0.0? If we say "We don't support X any more", there should always be a
tangible reason (such as being able to use newer language features).

We had clang 3.9 on the buildbot previously, I just kept the version we had.
Looks like the default "clang" package on xenial is actually clang 3.8 but clang
3.9 and 4.0 are available there as well.

What is the reason for choosing version 3.9 of Clang? Is it only because 
AppleClang 8.0.0 is based on it? I don't think we need to keep the two in sync. 
Since we use clang-tidy-5.0 (which depends on clang-5.0) for the tests, I think 
it makes sense to also choose clang-5.0 as the minimum supported Clang version.

I added a buildbot worker image for Ubuntu 16.04. It is the same the existing
one except for replacing 18 with 16. The worker for g++-5 now uses this image
and because of this, it defaults to g++-5.4.

The builds are still running but the first one generated a warning about dlopen.
It sound related to the -ldl issue that recently came up. We should look into it
but it seems unrelated to this issue.

Other than that, are the compilers we test now the compilers we want to use as
the new minimum requirement (except for g++-8.2 of course)?

* clang-3.9
* g++-8.2
* g++-5.4
* MSVC 19.15 (comes with the latest Visual Studio 2017 version)
* AppleClang 8.0.0 (based on clang 3.9)

> Currently, we use a Ubuntu 18.04 base image for all buildbot workers.
> The image is parametrized so we can use different compiler packages
> in each worker. If we want a 16.04 base image, most of the code will
> be the same (setting up VAL, python, CPLEX, lab, ...). I don't see a good
> way to re-use this code, so we would probably have two Docker files which
> are mostly similar. If you don't mind the code duplication, it should not
> be a big problem.
>
> The alternative ("option 1") I saw, would be to see if we can get g++-5.4
> to install on a Ubuntu 18.04. Not sure if this is better.

I think the build bots should be set up as similarly as possible to setups that
users might actually use. I think an Ubuntu 18.04 setup with g++-5.4 would be
odd: if I have Ubuntu 18.04, it is very unlikely that I would like to you use
g++-5.4 because better compiler versions are readily available for my OS. In
general, I think it is more useful to think of us as supporting specific *OS*
versions, not specific compiler versions.

So I think there is significantly more value in having separate buildbots for
(e.g.) Ubuntu 16.04, Ubuntu 18.04 and Ubuntu 18.10 than trying to use the same
OS for all buildbots and "simulate" the other OSes through exotic package
installations that use specific compilers etc. For me being able to easily test
different OS versions is the most attractive aspect of using docker for the
buildbot workers.

I don't mind having largely duplicated setup scripts for the different OS
versions. I think being able to directly point to something and say "This is how
you can make things run on Ubuntu X" is a significant asset. Of course, we have
to take into account how much maintenance work it actually causes us in the end,
but the hope would be that for most OS versions, once we've got it running,
ongoing maintenance will be low.


All these are general considerations, and in practical terms we need to consider
the cost/benefit of supporting each OS. If we can support Ubuntu 16.04 with
minimal disruptions, I'd like us to do that because I expect it to still be in
common use. For example, I use it (on some machines). :-) And it still has an
official service life until 2021.

> > The main issue with option 2 is that it would duplicate some
> > Docker code.
>
> I don't know what you mean with "option 2" here.

This was referring to the second part of this sentence:

> Changing it would require some more work there to either get 5.4 on Ubuntu
> 18.04 or to use a different base OS.

Currently, we use a Ubuntu 18.04 base image for all buildbot workers. The image
is parametrized so we can use different compiler packages in each worker. If we
want a 16.04 base image, most of the code will be the same (setting up VAL,
python, CPLEX, lab, ...). I don't see a good way to re-use this code, so we
would probably have two Docker files which are mostly similar. If you don't mind
the code duplication, it should not be a big problem.

The alternative ("option 1") I saw, would be to see if we can get g++-5.4 to
install on a Ubuntu 18.04. Not sure if this is better.

Unless there is a compelling advantage of gcc-5.5 over gcc-5.4, I'd be happier
to use gcc-5.4 as our minimum supported version because it seems to be the most
modern version of gcc available on Ubuntu 16.04.

If there is a big difference between gcc-5.4 and gcc-5.5, I don't mind dropping
support for Ubuntu 16.04, but I think there should be a reason. I hope with
docker it shouldn't be too difficult to have an Ubuntu 16.04 buildbot?

> The main issue with option 2 is that it would duplicate some
> Docker code.

I don't know what you mean with "option 2" here.

I think the results in issue859 are already good enough to make a choice here.
Since we had to re-create the buildbot workers anyway, we already set them up
with new compiler versions. They are currently testing:

* clang-3.9
* g++-8.2
* g++-5.5
* MSVC 19.15 (comes with the latest Visual Studio 2017 version)
* AppleClang 8.0.0 (based on clang 3.9)

If we want to make g++-5.4 the default, maybe we should change the version in
the buildbot worker. I used 5.5 because all Linux workers are based on Ubuntu
18.04 images and 5.5 is the default g++-5 version on there. Changing it would
require some more work there to either get 5.4 on Ubuntu 18.04 or to use a
different base OS. The main issue with option 2 is that it would duplicate some
Docker code.

Alternatively, we could set 5.5 as our new minimum supported version. I couldn't
find a list of changes between 5.4 and 5.5 but I guess they will be fairly similar.

Updating the summary: 
 * Buildbot now has workers for g++ 5 and 8.
 * The grid modules for those compilers also exist
 * issue859 will test both and compare them to g++ 4.8

We created issue859 to analyze the performance of GCC 5.4 and 8.2 on the grid.

Sounds good.

After discussing with Florian, we are going to try this option of native build
options. This is also a less hacky solution for the "-m64" flag problem on
Windows. This is being discussed in issue857.

A less hacky way to deal with the warning about "-m64" would be to add build
configurations "release-native" and "debug-native" which are like "release64"
and "debug64" but without explicitly adding the flag. Once we get rid of the
distinction in the build system, this will all simplify a lot.

We won't forbid shadowing; see msg7954.

Unless it's really easy to do, I would not want us to spend resources on
investigating how to build in 32-bit vs. 64-bit mode under Windows. This choice
will go away, and Windows is not a platform where we care about efficiency to
the same degree we due under Linux.

For what it's worth, passing the "-m64" option to the Windows compiler that
doesn't make sense because it's specific to gcc/clang and then asking the
Windows compiler to ignore options that don't make sense seems less than clean.
;-) I think it's a decent practical solution for now, but I would appreciate if
we could somehow make sure we remove the "ignore unknown options" option again
once we no longer need -m64, i.e., once we change to only supporting native builds.

Regarding the warnings about shadowing: the discussion in issue850 is still 
ongoing and it's not clear whether we want to forbid shadowing.

I added two more items to the summary:

As Malte said, we should test GCC 5.4 on the grid before we decide to make it a
minimum requirement. It should be possible to do this with the foss-2016b
modules Thomas mentioned in msg7920.

I also asked sciCORE for a more recent toolchain (using GCC 8.2 if possible).
They are currently busy but will probably have time for it next week. Once this
is set up, we should also test it of course and make it our default if it works
as well as expected. (This is not really part of the issue but since we compare
compilers here anyway its probably easy to test this one as well).

Try adding this to src/cmake_modules/FastDownwardMacros.cmake (together with the
other ignored warnings):
        set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd9002") # ignore -m64 flag in
release64 build config

If you are modifying that part of the code anyway, could you also try if we can
stop ignoring some of the other warnings?
For example, d4458 and d4456 are about shadowing and I think we recently fixed
this on the Unix side as well.

We don't need the build for Visual Studio 2015 anymore but it would be good to
also test the 32-bit build (without LP support). Could you change it to  Visual
Studio 2017?

We updated the buildbot script for Windows. Now it uses VC15 instead of VC14.
However, we have an annoying warning message from the cl compiler complaining
about the "-m64" flag. This flag probably comes from "build_config.py" where we
set "-DCMAKE_CXX_FLAGS='-m64'" for all 64-bits build.
In any case, we should decide whether to ignore or to fix this warning (which
was not possible just with a 10 minutes search on Google :)
I guess we also want to remove the other Windows 32-bits compilations from the
buildbot scheduler, right?

Also, apparently the compilation on Windows does not use all available cores.
The script recognizes that it has 8 processors but when we check it in the Task
Manager of Windows, we can see that it definitely uses 4 cores or less. I will
investigate this issue later today.

To switch the windows buildbot to VS2017, we need to update the CPLEX find
script. This is issue853.

I tried the GCC/5.4 module on the grid. It works in 64bit mode but it doesn't
include g++-multilib, so a 32bit build fails.

I asked scicore if it's possible to install g++ 8.2 including multilib. If that
is not possible, I'll ask them to install multilib for g++ 5.4.

I recently needed gcc 5.4 on the grid (for Prost) and asked the scicore team to
install the required packages. They added "-foss-2016b" versions of some
packages, e.g. Python/2.7.12-foss-2016b and CMake/3.4.3-foss-2016b, which should
make things work (I am not sure if anything else is required for Fast Downward,
though).

I would prefer to test that we compile with a new gcc version on the grid before
we decide to make that version the minimum requirement, not afterwards. I
remember that Augusto wrote last week that he tried to use an existing gcc 5.4
module on the grid but wasn't able to make it work because of clashes with some
other module we needed (likely something to do with Python, or perhaps something
more specific like matplotlib).

In any case, do I understand correctly that setting up a modern compiler version
on the grid is not a big deal? In that case, perhaps we could ask them for the
latest stable gcc. (I think that's gcc 8.2?) In Guillem's experiments the impact
of a modern compiler on performance was substantial.

Obviously on the buildbots we don't want to test just the latest and greatest
because the code is supposed to compile on non-bleeding-edge systems, but for
our experiments, I'm not sure there's a good reason to use a compiler that is
older than necessary.

I don't worry too much about the grid. If there is no module that works for us,
we could ask sciCore to create a new one. As far as I know, this should not be
much work for them because the module system was designed to support just that.
Anyway, getting a suitable compiler running on the grid is the next step. I
already added this to the summary.

> is it fair to say that gcc is currently holding us back most, and once we
> bump gcc, Visual Studio 2015 will be the next bottleneck?

Yes, this is the case from the data in the table. The details might be a bit
more muddy because we had cases in the past where a compiler did not support a
certain way of implementing a constructor (IIRC). Bugs like these are more
fine-grained than the table and it is hard to find them before running into a
problem like this. I think running our tests on our machines, the build bot and
on the grid for the current revision is sufficient for now.

Thanks for starting this, Florian!

I think we should add to the discussion which gcc versions can be used on our
compute cluster (and how to set them up, if not obvious). I currently get gcc
4.8.4 there, and Augusto mentioned that getting 5.4 is not as easy as he thought.

I think the two main reasons to raise the minimum supported version is to save
us work and to increase the range of language features we can use. (Faster code
is another side benefit, but I wouldn't worry about that too much.)

Saving us work was what prompted the discussion regarding Visual Studio. It it
is difficult to test both Visual Studio 2015 and Visual Studio 2017, this is an
argument in favour of dropping Visual Studio 2015 support. Of course, there are
conter-arguments, too, and the costs need to be weighed against the benefits.

Regarding increasing the range of language features, we will always be bound by
the lowest common denominator of the supported compilers. Based on what you
write, is it fair to say that gcc is currently holding us back most, and once we
bump gcc, Visual Studio 2015 will be the next bottleneck? (I appreciate the link
you provided, but if you already looked into this, maybe you can already answer
the question. If not, I can look at the details behind the link.)

We recently discussed raising the versions of the oldest supported compilers. In
this issue we want to discuss the details and keep track of all the necessary
changes.

We currently support and test
 * GCC 4.8
 * Clang 3.5.2
 * Apple Clang 8.0.0 (based on Clang 3.9)
 * MSVC 19.0 (comes with Visual Studio 2015)

The last time we discussed GCC version, we ended up with GCC 4.8 because this
was the default compiler of Ubuntu in the oldest supported LTS version (14.04 at
the time). The corresponding compiler today would be the default compiler of
Ubuntu 16.04 which is GCC 5.4.

This brings us in the range of full C++-14 support and a small handful of C++-17
features. Unfortunately, the bulk of C++-17 features (including the "any" class)
requires GCC 7 which doesn't work out of the box on 16.04.

Considering Windows, MSVC 19.0 already supports most C++-14 features but
upgrading to Visual Studio 2017 would give us full support. There are different
compiler version for Visual Studio 2017 that changed between version 15.0 (MSVC
19.10) and 15.8 (MSVC 19.15). The latest one has full support for C++-17. As
there is no "default version" that comes with the OS, all versions are free, and
newer VS versions can be installed on older Windows versions, I suggest we
require and test only the latest one.

On the Mac, Clang 3.5.2 already has full C++-14 support and partial C++-17
support so we don't have to change it. We could raise it to Clang 4.0 to cover
most of C++-17 (including "any").

For reference, here is the list of C++ (11/14/17) features and which compiler
supports what:
https://en.cppreference.com/w/cpp/compiler_support#cpp14