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

Remove shared_ptr support from EOS Portable Archive #6369

Merged

Conversation

davidlt
Copy link
Contributor

@davidlt davidlt commented Nov 13, 2014

boost::archive::detail::shared_ptr_helper has been replaced by
boost::serialization::shared_ptr_helper. Thus shared_ptr_helper
ctor/dtor symbols are missing from Boost Serialization library.

undefined reference to `boost::archive::detail::shared_ptr_helper
::shared_ptr_helper()'
undefined reference to `boost::archive::detail::shared_ptr_helper
::~shared_ptr_helper()'

The new shared_ptr_helper implementation is in
shared_ptr_helper.cpp, but the file is not listes in SOURCES in
libs/serialization/build/Jamfile.v2.

Adding it would break Boost Serialization.

Signed-off-by: David Abdurachmanov David.Abdurachmanov@cern.ch

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @davidlt for CMSSW_7_3_X.

Remove shared_ptr support from EOS Portable Archive

It involves the following packages:

CondFormats/Serialization

@ggovi, @cmsbuild, @apfeiffer1, @nclopezo can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@apfeiffer1
Copy link
Contributor

+1
assuming Jenkins will agree.

On Thu, Nov 13, 2014 at 11:51 AM, cmsbuild notifications@github.com wrote:

A new Pull Request was created by @davidlt https://github.com/davidlt
for CMSSW_7_3_X.

Remove shared_ptr support from EOS Portable Archive

It involves the following packages:

CondFormats/Serialization

@ggovi https://github.com/ggovi, @cmsbuild https://github.com/cmsbuild,
@apfeiffer1 https://github.com/apfeiffer1, @nclopezo
https://github.com/nclopezo can you please review it and eventually
sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line
of your reply.
You can reject by replying to this message having '-1' in the first line
of your reply.
@nclopezo https://github.com/nclopezo you are the release manager for
this.
You can merge this pull request by typing 'merge' in the first line of
your comment.


Reply to this email directly or view it on GitHub
#6369 (comment).

Thanks,
cheers, andreas

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes or unless it breaks tests.

@davidlt
Copy link
Contributor Author

davidlt commented Nov 13, 2014

This is the only solution I can currently offer without digging to deep. I wrote to boost mailing list, but I don't expect the answer soonish (or at all).

@davidlt
Copy link
Contributor Author

davidlt commented Nov 13, 2014

I already see in Jenkins that this is failing with Boost 1.51.0 and I only tested with 1.57.0. I will look once the results are ready.

@cmsbuild
Copy link
Contributor

-1
Tested at: 579b481
I found an error when building:

/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc481/external/boost/1.51.0-cms2/include/boost/archive/basic_binary_iarchive.hpp:70:7:   required from 'void boost::archive::basic_binary_iarchive::load_override(T&, int) [with T = const boost::serialization::nvp > > > >; Archive = eos::portable_iarchive]'
/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc481/external/boost/1.51.0-cms2/include/boost/archive/detail/interface_iarchive.hpp:60:9:   required from 'Archive& boost::archive::detail::interface_iarchive::operator>>(T&) [with T = const boost::serialization::nvp > > > >; Archive = eos::portable_iarchive]'
/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc481/external/boost/1.51.0-cms2/include/boost/archive/detail/interface_iarchive.hpp:67:32:   required from 'Archive& boost::archive::detail::interface_iarchive::operator&(T&) [with T = const boost::serialization::nvp > > > >; Archive = eos::portable_iarchive]'
tmp/slc6_amd64_gcc481/src/CondFormats/RPCObjects/src/CondFormatsRPCObjects/a/Serialization.cc:66:8:   required from 'void L1RPCConeBuilder::serialize(Archive&, unsigned int) [with Archive = eos::portable_iarchive]'
tmp/slc6_amd64_gcc481/src/CondFormats/RPCObjects/src/CondFormatsRPCObjects/a/Serialization.cc:69:1:   required from here
/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc481/external/boost/1.51.0-cms2/include/boost/serialization/shared_ptr.hpp:155:5: error: 'class eos::portable_iarchive' has no member named 'reset'
     ar.reset(t,r);
     ^
/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc481/external/boost/1.51.0-cms2/include/boost/serialization/shared_ptr.hpp: In instantiation of 'void boost::serialization::load(Archive&, boost::shared_ptr&, unsigned int) [with Archive = eos::portable_iarchive; T = std::map >]':
/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc481/external/boost/1.51.0-cms2/include/boost/serialization/split_free.hpp:58:22:   required from 'static void boost::serialization::free_loader::invoke(Archive&, T&, unsigned int) [with Archive = eos::portable_iarchive; T = boost::shared_ptr > >]'
/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc481/external/boost/1.51.0-cms2/include/boost/serialization/split_free.hpp:74:38:   required from 'void boost::serialization::split_free(Archive&, T&, unsigned int) [with Archive = eos::portable_iarchive; T = boost::shared_ptr > >]'


you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6369/849/summary.html

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (but tests are reportedly failing).

`boost::archive::detail::shared_ptr_helper` has been replaced by
`boost::serialization::shared_ptr_helper`. Thus `shared_ptr_helper`
ctor/dtor symbols are missing from Boost Serialization library.

    undefined reference to `boost::archive::detail::shared_ptr_helper
    ::shared_ptr_helper()'
    undefined reference to `boost::archive::detail::shared_ptr_helper
    ::~shared_ptr_helper()'

The new `shared_ptr_helper` implementation is in
`shared_ptr_helper.cpp`, but the file is not listes in `SOURCES` in
`libs/serialization/build/Jamfile.v2`.

Adding it would break Boost Serialization.

`boost::archive::detail::shared_ptr_helper` is valid until 1.55.0
(incl.). The patch hardcodes this fact, meaning that 1.56.0+ will not
have `shared_ptr` support.

Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
@davidlt davidlt force-pushed the remove-eos-portable-archive-shared_ptr branch from 579b481 to 385a325 Compare November 13, 2014 12:41
@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes or unless it breaks tests.

@davidlt
Copy link
Contributor Author

davidlt commented Nov 13, 2014

Yes, it's a correct one. My mind wants to rush things up already :)
Tested locally on GCC 4.8.1 with Boost 1.51.0 and 1.57.0. Will merge it.

### CondFormats/RPCObjects/interface/L1RPCConeBuilder.h

139    private:
140          
141      int m_firstTower;
142      int m_lastTower;
143 
144      boost::shared_ptr< TConMap > m_coneConnectionMap; 
145      boost::shared_ptr< TCompressedConMap >  m_compressedConeConnectionMap;
146 
147    COND_SERIALIZABLE;
148 };

### tmp/slc6_amd64_gcc481/src/CondFormats/RPCObjects/src/CondFormatsRPCObjects/a/Serialization.cc

61 template <class Archive>
62 void L1RPCConeBuilder::serialize(Archive & ar, const unsigned int)
63 {
64     ar & BOOST_SERIALIZATION_NVP(m_firstTower);
65     ar & BOOST_SERIALIZATION_NVP(m_lastTower);
66     ar & BOOST_SERIALIZATION_NVP(m_coneConnectionMap);
67     ar & BOOST_SERIALIZATION_NVP(m_compressedConeConnectionMap);
68 }
69 COND_SERIALIZATION_INSTANTIATE(L1RPCConeBuilder);

You are serializing boost::shared_ptr. I have no idea what kid of behavior difference this will create on Boost 1.57.0. It seems to resolve undefined symbols issue for now, still need an IB to fully test.

Seems that your first message to Boost mailing list needs to be approved by a moderator, so I don't know when exactly I will receive some feedback (if any).

@davidlt
Copy link
Contributor Author

davidlt commented Nov 13, 2014

Seems I missed 1400, so I will let Jenkins test && merge it.

@apfeiffer1
Copy link
Contributor

Yes, it's a correct one. My mind wants to rush things up already :)
Tested locally on GCC 4.8.1 with Boost 1.51.0 and 1.57.0. Will merge it.

CondFormats/RPCObjects/interface/L1RPCConeBuilder.h

139 private:
140
141 int m_firstTower;
142 int m_lastTower;
143
144 boost::shared_ptr< TConMap > m_coneConnectionMap;
145 boost::shared_ptr< TCompressedConMap > m_compressedConeConnectionMap;
146
147 COND_SERIALIZABLE;
148 };

tmp/slc6_amd64_gcc481/src/CondFormats/RPCObjects/src/CondFormatsRPCObjects/a/Serialization.cc

61 template
62 void L1RPCConeBuilder::serialize(Archive & ar, const unsigned int)
63 {
64 ar & BOOST_SERIALIZATION_NVP(m_firstTower);
65 ar & BOOST_SERIALIZATION_NVP(m_lastTower);
66 ar & BOOST_SERIALIZATION_NVP(m_coneConnectionMap);
67 ar & BOOST_SERIALIZATION_NVP(m_compressedConeConnectionMap);
68 }
69 COND_SERIALIZATION_INSTANTIATE(L1RPCConeBuilder);

You are serializing boost::shared_ptr. I have no idea what kid of
behavior difference this will create on Boost 1.57.0. It seems to resolve
undefined symbols issue for now, still need an IB to fully test.

Seems that your first message to Boost mailing list needs to be approved
by a moderator, so I don't know when exactly I will receive some feedback
(if any).

yep, we'll need a full build with boost 1.57 to check anything. I tried
already earlier with a private build and only some packages, and this
failed as there are incompatibilities between 1.51 and 1.57 which showed up
in other places - and I did not have the time to make a full rebuild of
everything for this. Maybe I will have some time for this in the second
half of next week, but not earlier. So any IB with this would be helpful
for testing.

Thanks,
cheers, andreas

@davidlt
Copy link
Contributor Author

davidlt commented Nov 13, 2014

If all goes fine, this is the last piece needed (to compile). I could provide you a local build of CMSSW on some machine for testing. You find no issues, then we move ahead.

My personal test criteria will be: compilation passing with 4.8.1 and 4.9.1, full RelVals and unit tests. This would take roughly a day to do.

@apfeiffer1
Copy link
Contributor

If all goes fine, this is the last piece needed (to compile). I could
provide you a local build of CMSSW on some machine for testing. You find no
issues, then we move ahead.

My personal test criteria will be: compilation passing with 4.8.1 and
4.9.1, full RelVals and unit tests. This would take roughly a day to do.

sounds good to me - if you have the local build, let me know. I would also
like to try and see if we can make some more unit-tests which test for
strange behaviour esp. with the shared_ptr there. Maybe we can even move to
std ones.

Thanks,
cheers, andreas

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). This pull request will be automatically merged.

cmsbuild added a commit that referenced this pull request Nov 13, 2014
…red_ptr

Remove shared_ptr support from EOS Portable Archive
@cmsbuild cmsbuild merged commit 0db89a2 into cms-sw:CMSSW_7_3_X Nov 13, 2014
@cmsbuild
Copy link
Contributor

@davidlt
Copy link
Contributor Author

davidlt commented Nov 13, 2014

Reply from Boost devs:

You need to update your implementation of portable_binary_iarchive 
to not include boost/archive/shared_ptr_helper.hpp It should not be 
necessary now 

Seems the fix is very correct :)

@apfeiffer1
Copy link
Contributor

Reply from Boost devs:

You need to update your implementation of portable_binary_iarchive
to not include boost/archive/shared_ptr_helper.hpp It should not be
necessary now

Seems the fix is very correct :)

great !! :) So we're good to go ?

Thanks,
cheers, andreas

@davidlt
Copy link
Contributor Author

davidlt commented Nov 13, 2014

Tomorrow morning I will attempt to build (locally) 0200 IB with Boost 1.57.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants