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

Fix/boost issue #713 again #732

Merged
merged 3 commits into from
Apr 8, 2021
Merged

Fix/boost issue #713 again #732

merged 3 commits into from
Apr 8, 2021

Conversation

Alevs2R
Copy link
Contributor

@Alevs2R Alevs2R commented Apr 6, 2021

Fixes issue #634
Fixes order of #include <boost/serialization/version.hpp> headers

My compilation on Ubuntu failed with the same error as in issue #634
Changing order of includes fixed my compilation.

<boost/serialization/version.hpp> should be included before <boost/serialization/list.hpp> and before <boost/shared_ptr.hpp>, not after.

Also it was needed to add includes to FastList.h to avoid the same compilation errors.

boost version: 1.74.0
gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)

Probably this issue will not appear in boost version 1.75, but I tested only with 1.74

Copy link
Collaborator

@ProfFan ProfFan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ProfFan
Copy link
Collaborator

ProfFan commented Apr 6, 2021

@varunagrawal What do you think?

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

Any reason you have added the <boost/serialization/optional.hpp> headers for FastList.h and SubgraphBuilder.h?

@Alevs2R
Copy link
Contributor Author

Alevs2R commented Apr 7, 2021

To be precise, I started again with the latest develop version and iteratively added changes to get rid of the errors:

  1. First error was this
In file included from /apollo/gtsam/gtsam/base/FastList.h:25:0:
/opt/apollo/sysroot/include/boost/serialization/list.hpp: In function 'void boost::serialization::load(Archive&, std::__cxx11::list<U, Allocator>&, unsigned int)':
/opt/apollo/sysroot/include/boost/serialization/list.hpp:53:33: error: 'library_version_type' in namespace 'boost::serialization' does not name a type
     const boost::serialization::library_version_type library_version(

I added #include <boost/serialization/version.hpp> into FastList.h, but still get the same error.
Then I added #include <boost/serialization/optional.hpp> and error disappeared.

  1. Second error looked like this
In file included from /apollo/gtsam/gtsam/linear/LossFunctions.h:28:0,
                 from /apollo/gtsam/gtsam/linear/LossFunctions.cpp:19:
/opt/apollo/sysroot/include/boost/serialization/optional.hpp:98:8: error: 'version' is not a class template
 struct version<boost::optional<T> > {
        ^~~~~~~
In file included from /opt/apollo/sysroot/include/boost/serialization/shared_ptr.hpp:32:0,
                 from /apollo/gtsam/gtsam/linear/LossFunctions.h:29,
                 from /apollo/gtsam/gtsam/linear/LossFunctions.cpp:19:
/opt/apollo/sysroot/include/boost/serialization/version.hpp:36:8: error: redefinition of 'struct boost::serialization::version<T>'
 struct version
        ^~~~~~~

I simply moved include of boost/serialization/version.hpp to line 28 from line 31 in LossFunctions.h and compilation succeeded.
3. I removed the line #include <boost/serialization/version.hpp> from gtsam/linear/SubgraphBuilder.h at all and compilation succeeded anyway. This line was added in MR #713 and it means that we don't really need this include. Why it was added here?

@Alevs2R
Copy link
Contributor Author

Alevs2R commented Apr 7, 2021

Any reason you have added the <boost/serialization/optional.hpp> headers for FastList.h and SubgraphBuilder.h?

As my inspection showed, it is needed for FastList.h, but is not needed for SubgraphBuilder.h, thank you for the comment. It would be great if someone with boost version 1.74.0 checked it also.

@varunagrawal
Copy link
Collaborator

@Alevs2R please add back the header to SubgraphBuilder.h since I believe that is required to work on Boost 1.74. Once you do that, I can approve.

Thanks a lot for the great work and explanation!!

@Alevs2R
Copy link
Contributor Author

Alevs2R commented Apr 8, 2021

@varunagrawal done!

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot @Alevs2R

@varunagrawal
Copy link
Collaborator

Please merge once the CI passes. :)

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.

3 participants