Use C++17 in O2

[dberzano]

No description provided.

[dberzano]

Let's see if tests are OK. Will be discussed tomorrow at WP3.

[ktf]

Yes we can!

[dberzano]

@davidrohr can you please have a look at the error log?

[ktf]

Regarding the register keyword the motivation for the deprecation can be found at:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4193.html#809

I suggest we simply drop it.

[dberzano]

@sawenzel as discussed privately, Vc has been bumped to three commits ahead of 1.3.3 to fix the std::for_each_n problem that we see in the logs.

[davidrohr]

@ktf @dberzano : your fix is correct. Below is the full story:

The register keyword was inserted some time ago as a workaround for a bug either in ROOT5 or in OpenCL. Essentially, the compiler required a keyword at that place before any of the GPUsharedref() macros, so we had to place a keyword there with "no meaning". The proper way would be to use the typename keyword, but that was not understood by one of the interpreters/compilers (don't remember which), so we used the deprecated register keyword, which is ignored by all compilers.
Anyway, I checked the GPU-build based on ROOT6 with both CUDA and OpenCL and they run fine. So I think the bug has been fixed in the meantime and we can do away with the deprecated keyword.

[awegrzyn]

This should be fixed as well: https://github.com/alisw/apmon-cpp/issues/2

[dberzano]

We also need AliceO2Group/AliceO2#1352 (ready to be merged but currently on hold). I am working on the Vc issue now.

[dberzano]

The Vc issue required some thinking. The fix is there. Note: a full build on CC7 works, locally, but it won't work here until AliceO2Group/AliceO2#1352 is merged.

[dberzano]

@alisw/mrrtf any impediment? Can you approve if it's OK for you?

[aphecetche]

no objection. will deal with any changes required in alo, if any, please proceed.

[dberzano]

Hold your horses. Does not work on macOS Mojave, breaks on GEANT4:

[...]/v10.3.3/source/processes/hadronic/models/cascade/cascade/src/G4NucleiModel.cc:1267:9: error: no member named 'bind2nd' in namespace 'std'

[dberzano]

@awegrzyn while compiling Monitoring v1.9.2, I get on Mojave:

Undefined symbols for architecture x86_64:
  "boost::system::detail::system_category_instance", referenced from:
      ___cxx_global_var_init.20 in InfluxDB.cxx.o
      ___cxx_global_var_init.24 in InfluxDB.cxx.o
      ___cxx_global_var_init.29 in InfluxDB.cxx.o
      ___cxx_global_var_init.19 in Flume.cxx.o
      ___cxx_global_var_init.23 in Flume.cxx.o
      ___cxx_global_var_init.28 in Flume.cxx.o
      o2::monitoring::transports::UDP::UDP(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int) in UDP.cxx.o
      ...
  "boost::system::detail::generic_category_instance", referenced from:
      boost::system::error_category::std_category::equivalent(int, std::__1::error_condition const&) const in InfluxDB.cxx.o
      boost::system::error_category::std_category::equivalent(std::__1::error_code const&, int) const in InfluxDB.cxx.o
      boost::system::error_category::std_category::equivalent(int, std::__1::error_condition const&) const in Flume.cxx.o
      boost::system::error_category::std_category::equivalent(std::__1::error_code const&, int) const in Flume.cxx.o
      boost::system::error_category::std_category::equivalent(int, std::__1::error_condition const&) const in UDP.cxx.o
      boost::system::error_category::std_category::equivalent(std::__1::error_code const&, int) const in UDP.cxx.o
      boost::system::error_category::std_category::equivalent(int, std::__1::error_condition const&) const in TCP.cxx.o
      ...
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [lib/libMonitoring.dylib] Error 1

I am using our boost, v1.68.0 (from this pull request). The problem is not seen on CC7. Do you have any clue?

For the record, the failing command is:

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -fPIC -O2 -std=c++17 -Wall -pedantic -Wextra -O2 -g -DNDEBUG -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -dynamiclib -Wl,-headerpad_max_install_names  -o lib/libMonitoring.dylib -install_name @rpath/libMonitoring.dylib CMakeFiles/Monitoring.dir/src/Monitoring.cxx.o CMakeFiles/Monitoring.dir/src/Metric.cxx.o CMakeFiles/Monitoring.dir/src/Backends/InfoLoggerBackend.cxx.o CMakeFiles/Monitoring.dir/src/Backends/InfluxDB.cxx.o CMakeFiles/Monitoring.dir/src/Backends/Flume.cxx.o CMakeFiles/Monitoring.dir/src/Backends/StdOut.cxx.o CMakeFiles/Monitoring.dir/src/DerivedMetrics.cxx.o CMakeFiles/Monitoring.dir/src/ProcessMonitor.cxx.o CMakeFiles/Monitoring.dir/src/ProcessDetails.cxx.o CMakeFiles/Monitoring.dir/src/MonitoringFactory.cxx.o CMakeFiles/Monitoring.dir/src/Transports/UDP.cxx.o CMakeFiles/Monitoring.dir/src/Transports/TCP.cxx.o CMakeFiles/Monitoring.dir/src/Transports/HTTP.cxx.o CMakeFiles/Monitoring.dir/src/Exceptions/MonitoringException.cxx.o /Users/volpe/.alibuildsw/osx_x86-64/boost/v1.68.0-1/lib/libboost_system.dylib /usr/lib/libcurl.dylib

[awegrzyn]

@dberzano Either adding cxxstd=17 to b2 command or using boost provided by brew (although it's 1.67 not 1.68) solves this problem.

[dberzano]

OK so - I had done that, however - as you can see - I had to disable boost_python (do we really need it?) otherwise boost cannot compile if the linked Python version has headers containing unsupported C++17 stuff (like the register keyword for instance).

[dberzano]

So, everything works, except for tests depending on boost, see #1305. We need to fix this.

[davidrohr]

So, everything works, except for tests depending on boost, see #1305. We need to fix this.

So as this issue is popping up again, shouldn't we take Ruben's approach and just add boost to ROOT_INCLUDE_PATH. We anyway have it that way for setups where we use system root. Why should it be different if we compile ROOT with alidist?

[dberzano]

Hi @davidrohr, I believe the reasons were clearly explained in #1305, I don't want to discuss it again. This PR failing is not a good argument in favor of that solution. The issue is popping up because, incidentally, the PR was first built on a certain machine with certain paths. The builds were cached, and when it reran, the cached builds were relocated to different paths.

I am pushing @shahor02's solution here just because I am tired of this C++17 PR, but this is not an endorsement nor a solution. And it may break at any point in the future.

[davidrohr]

@dberzano : We will need a new version of AliTPCCommon after this PR to fix builds with GPU.
This affects only me (and perhaps Sandro), so we can go ahead with this c++17 PR.
I'll create a new AliTPCCommon tag once I have fixed the issue and verified it.