Fails to compile with Boost 1.56.0+.

saurabhshri commented 7 years ago

sudo apt-get -y install libboost-all-dev fetches Boost 1.58 and there was change in some templates in Boost 1.56 thus compilation fails.

Relevant links : http://lists.boost.org/boost-users/2014/08/82693.php and https://www.facebook.com/libracode/posts/1339039419469305

[ 13%] Building CXX object CMakeFiles/rhubarb-exporters.dir/src/exporters/XmlExporter.cpp.o
In file included from /usr/include/boost/property_tree/detail/xml_parser_utils.hpp:15:0,
                 from /usr/include/boost/property_tree/detail/xml_parser_write.hpp:15,
                 from /usr/include/boost/property_tree/xml_parser.hpp:15,
                 from /home/ubuntu/rhubarb-lip-sync/src/exporters/XmlExporter.cpp:3:
/usr/include/boost/property_tree/detail/xml_parser_writer_settings.hpp: In instantiation of ‘class boost::property_tree::xml_parser::xml_writer_settings<char>’:
/home/ubuntu/rhubarb-lip-sync/src/exporters/XmlExporter.cpp:28:53:   required from here
/usr/include/boost/property_tree/detail/xml_parser_writer_settings.hpp:38:35: error: ‘char’ is not a class, struct, or union type
  typedef typename Str::value_type Ch;
                                   ^
/usr/include/boost/property_tree/detail/xml_parser_writer_settings.hpp:40:9: error: ‘char’ is not a class, struct, or union type
         xml_writer_settings(Ch inchar = Ch(' '),
         ^
/usr/include/boost/property_tree/detail/xml_parser_writer_settings.hpp:50:33: error: ‘char’ is not a class, struct, or union type
         typename Str::size_type indent_count;
                                 ^
/home/ubuntu/rhubarb-lip-sync/src/exporters/XmlExporter.cpp: In member function ‘virtual void XmlExporter::exportAnimation(const boost::filesystem::path&, JoiningContinuousTimeline<Shape>&, const ShapeSet&, std::ostream&)’:
/home/ubuntu/rhubarb-lip-sync/src/exporters/XmlExporter.cpp:28:53: error: no matching function for call to ‘boost::property_tree::xml_parser::xml_writer_settings<char>::xml_writer_settings(char, int)’
  write_xml(outputStream, tree, writer_setting(' ', 2));
                                                     ^
In file included from /usr/include/boost/property_tree/detail/xml_parser_utils.hpp:15:0,
                 from /usr/include/boost/property_tree/detail/xml_parser_write.hpp:15,
                 from /usr/include/boost/property_tree/xml_parser.hpp:15,
                 from /home/ubuntu/rhubarb-lip-sync/src/exporters/XmlExporter.cpp:3:
/usr/include/boost/property_tree/detail/xml_parser_writer_settings.hpp:36:11: note: candidate: boost::property_tree::xml_parser::xml_writer_settings<char>::xml_writer_settings()
     class xml_writer_settings
           ^
/usr/include/boost/property_tree/detail/xml_parser_writer_settings.hpp:36:11: note:   candidate expects 0 arguments, 2 provided
/usr/include/boost/property_tree/detail/xml_parser_writer_settings.hpp:36:11: note: candidate: constexpr boost::property_tree::xml_parser::xml_writer_settings<char>::xml_writer_settings(const boost::property_tree::xml_parser::xml_writer_settings<char>&)
/usr/include/boost/property_tree/detail/xml_parser_writer_settings.hpp:36:11: note:   candidate expects 1 argument, 2 provided
/usr/include/boost/property_tree/detail/xml_parser_writer_settings.hpp:36:11: note: candidate: constexpr boost::property_tree::xml_parser::xml_writer_settings<char>::xml_writer_settings(boost::property_tree::xml_parser::xml_writer_settings<char>&&)
/usr/include/boost/property_tree/detail/xml_parser_writer_settings.hpp:36:11: note:   candidate expects 1 argument, 2 provided
CMakeFiles/rhubarb-exporters.dir/build.make:134: recipe for target 'CMakeFiles/rhubarb-exporters.dir/src/exporters/XmlExporter.cpp.o' failed
make[2]: *** [CMakeFiles/rhubarb-exporters.dir/src/exporters/XmlExporter.cpp.o] Error 1
CMakeFiles/Makefile2:73: recipe for target 'CMakeFiles/rhubarb-exporters.dir/all' failed
make[1]: *** [CMakeFiles/rhubarb-exporters.dir/all] Error 2
Makefile:149: recipe for target 'all' failed
make: *** [all] Error 2

DanielSWolf commented 7 years ago

That's strange. I'm aware of the breaking change in Boost 1.56. As a matter of fact, there is already code that should handle both syntaxes in XmlExporter.cpp. I'll look into it.

saurabhshri commented 7 years ago

Thanks. Also, if it helps, I compiled using the same steps as mentioned in the Tarvis configuration.

DanielSWolf commented 7 years ago

Here's what I found out:

There has been a breaking change between 1.55 and 1.56. Ever since that, the API has remained stable.
Rhubarb already checks (in src/exporters/XmlExporter.cpp, line 23) whether the Boost version is at least 1.56; in this case, it uses the new syntax.
On my machine, I have Boost 1.61, and it's building fine. Since both 1.61 (mine) and 1.58 (yours) are >= 1.56, they should both have the same API and should both work fine.
According to your error log, Rhubarb is trying to instantiate xml_writer_settings<char>. Looking again at XmlExporter.cpp, I see that this is the syntax Rhubarb uses for Boost version 1.55 and below. So my guess is that you are in fact using Boost 1.58, but BOOST_VERSION falsely contains the ID of an older Boost version. Maybe you have multiple Boost versions installed, and for some reason are referencing the wrong version.hpp.

Could you check the value of BOOST_VERSION on your machine during compilation of Rhubarb?

saurabhshri commented 7 years ago

I tried the installation on a freshly created instance. So, there was no prior boost library present. Also, when doing make, it shows that the Boost Library is in fact 1.58.

ubuntu@ip-172-31-21-31:~/rhubarb-lip-sync/build$ cmake -DCMAKE_CXX_COMPILER=g++-5 .. && make
-- Boost version: 1.58.0
-- Found the following Boost libraries:
--   filesystem
--   locale
--   system
-- Configuring done
-- Generating done
-- Build files have been written to: /home/ubuntu/rhubarb-lip-sync

I then wrote a few lines of code to check if it indeed it works or not :

#include <boost/version.hpp>
#include <iostream>

using namespace std;

int main()
{
        cout << "BOOST_LIB_VERSION : " << BOOST_LIB_VERSION << endl;
        cout << "BOOST_VERSION : " << BOOST_VERSION<<endl;

        #if BOOST_VERSION >= 105600
                cout<<"Fix works"<<endl;
        #else
                cout<<"Should not be displayed"<<endl;
        #endif

        #if (BOOST_VERSION >= 105600)
                cout<<"Bracket is needed"<<endl;
        #endif

        //try changing order :

        #if BOOST_VERSION < 105600
                cout<<"Should not be displayed"<<endl;
        #else
                cout<<"Should be displayed"<<endl;
        #endif

return 0;
}

Output :

BOOST_LIB_VERSION : 1_58
BOOST_VERSION : 105800
Fix works
Bracket is needed
Should be displayed

That means the fix should have worked and I am not sure why it is still failing.

I have some work being processed on that instance, and will delete and try again on new instance in the evening. I'll post results after that.

DanielSWolf commented 7 years ago

Let me know what results you get.

Also, try adding your test code to XmlExporter.cpp, so we learn what's happening differently within Rhubarb. But replace the couts with #warning or a similar statement that gets executed at compile time.

saurabhshri commented 7 years ago

Found it! The header file boost/version.hpp containing the macro BOOST_VERSION is missing in the file. Adding it solves the bug. :)

Is it okay for me to send the PR for same?

PS I wasn't aware about #warning, thanks for that. :)

DanielSWolf commented 7 years ago

Thanks for finding the problem! I guess that by (un-)lucky accident, version.hpp is indirectly included in my version of Boost, so I never realized the include is missing.

A pull request would be great. However, I'd like to make sure that nobody accidentally removes the include in the future. Could you add a static assertion just before checking BOOST_VERSION? Something like this:

static_assert(BOOST_VERSION, "Error detecting Boost version.");

saurabhshri commented 7 years ago

If someone removes the include in future, static_assert will yield error about not being able to find BOOST_VERSION instead of error: static assertion failed: {our message}. Moreover static_assert will probably be needing it's own header file too, at least in g++, though it should be fine in our case. How about maybe checking if the macro is defined or not using #ifndef ?

#ifndef BOOST_VERSION
        #error "Could not detect Boost version."
#endif

This will give :

ubuntu@ip-172-31-21-31:~/rhubarb-lip-sync/build$ g++ boost_c.cpp
boost_c.cpp:13:4: error: #error "Could not detect Boost version."
   #error "Could not detect Boost version."
    ^

While static_assert will give :

error: ‘BOOST_VERSION’ was not declared in this scope
static_assert(BOOST_VERSION, "Error detecting Boost version.");
               ^

Though to think of it, the latter gives more clear message :) . I'll do whatever you suggest :)

DanielSWolf commented 7 years ago

Good point. Your #error version is more correct -- let's us it.

DanielSWolf / rhubarb-lip-sync

Fails to compile with Boost 1.56.0+. #9