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.

Sign up for GitHub

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

Jump to bottom

Add/remove namespaces from Linkdef #10306

Merged
merged 3 commits into from Apr 19, 2022
Merged

Add/remove namespaces from Linkdef #10306

merged 3 commits into from Apr 19, 2022

Conversation

ellert
Copy link
Contributor

@ellert ellert commented Apr 4, 2022

This Pull request:

Changes or fixes:

See the commit messages for details.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@ellert
Add namespace TMVA::Experimental::SOFIE to LinkDef
c66ae9d 
Fixes error:

IncrementalExecutor::executeFunction: symbol '_ZN4TMVA12Experimental5SOFIE19ConvertTypeToStringB5cxx11ENS1_11ETensorTypeE' unresolved while linking function '_GLOBAL__sub_I_cling_module_8'!
You are probably missing the definition of TMVA::Experimental::SOFIE::ConvertTypeToString[abi:cxx11](TMVA::Experimental::SOFIE::ETensorType)
Maybe you need to load the corresponding shared library?
Symbol found in '/builddir/build/BUILD/root-6.26.00/redhat-linux-build/lib/libROOTTMVASofie.so.6.26.00'; did you mean to load it with '.L /builddir/build/BUILD/root-6.26.00/redhat-linux-build/lib/libROOTTMVASofie.so.6.26.00'?
@ellert
Remove namespace RooFit from libRooFitCore's LinkDef
4a4df01 
The namespace is listed in the LinkDef's for both libRooFit and libRooFitCore.

ROOT only autoloads one of the libraries, for some reason usually
libRooFitCore. After this only symbols in the namespace from that
library are present. By removing the namespace from liRooFitCore's
LinkDef, only libRooFit can be chosen when autoloading. Since
libRooFit depends on libRooFitCore, both libraries are loaded, and the
symbols from the namespace in both libraries are available.

Fixes error:

IncrementalExecutor::executeFunction: symbol '_ZN6RooFit12bindFunctionEPKcPFddER10RooAbsReal' unresolved while linking function '_GLOBAL__sub_I_cling_module_217'!
You are probably missing the definition of RooFit::bindFunction(char const*, double (*)(double), RooAbsReal&)
Maybe you need to load the corresponding shared library?
Symbol found in '/builddir/build/BUILD/root-6.26.00/redhat-linux-build/lib/libRooFit.so.6.26.00'; did you mean to load it with '.L /builddir/build/BUILD/root-6.26.00/redhat-linux-build/lib/libRooFit.so.6.26.00'?
@ellert
Add namespaces to LinkDef
0a969f5 
Fixes error:

IncrementalExecutor::executeFunction: symbol '_ZN4TMVA12Experimental5SOFIE7PyTorch5ParseENSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESt6vectorIS9_ImSaImEESaISB_EE' unresolved while linking function '_GLOBAL__sub_I_cling_module_8'!
You are probably missing the definition of TMVA::Experimental::SOFIE::PyTorch::Parse(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::vector<std::vector<unsigned long, std::allocator<unsigned long> >, std::allocator<std::vector<unsigned long, std::allocator<unsigned long> > > >)
Maybe you need to load the corresponding shared library?
Symbol found in '/builddir/build/BUILD/root-6.26.00/redhat-linux-build/lib/libPyMVA.so.6.26.00'; did you mean to load it with '.L /builddir/build/BUILD/root-6.26.00/redhat-linux-build/lib/libPyMVA.so.6.26.00'?
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

  • [2022-04-04T08:40:31.796Z] CMake Error at C:/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1083 (message):

guitargeek
guitargeek reviewed Apr 4, 2022
View reviewed changes
roofit/roofitcore/inc/LinkDef.h
Comment on lines +108 to +111

// "namespace RooFit" is in roofit/roofit/inc/Linkdef1.h
// should not be in the dictionary for two different libraries
// #pragma link C++ namespace RooFit ;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, thanks for the fix! But can you add back this line and instead remove it from roofit/roofit/inc/Linkdef1.h? Because the RooFit namespace is introduced in RooFitCore, and RooFit depends on RooFitCore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that both RooFit and RooFitCore define symbols in the RooFit namespace.
If "#pragma link C++ namespace RooFit" is in RooFitCore, the autoloading when requested to load the RooFit namespece will then load RooFitCore. However, if the symbol needed was one of the symbols in the RooFit namespace defined in RooFit it will not have been loaded and you get a failure.
If "#pragma link C++ namespace RooFit" is in RooFit, the autoloader when requested to load the RooFit namespace will load RooFit, which will also load RooFitCore, since RooFit depends on RooFitCore. And irrespectively on in which of the libraries the symbol is, it will have been loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The best solution would be to use different namespaces in the two libraries, but that would rename things and could cause backward compatibility problems. If the names in one of the libraries are "internal" or "experimental" they can be renamed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok I get it then, so it has to be like that. Thanks 👍

guitargeek
guitargeek requested changes Apr 4, 2022
View reviewed changes
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes! The TMVA part is OK (unless @lmoneta has objections), I just have one request for the RooFit part.

guitargeek
guitargeek approved these changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@guitargeek guitargeek 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 for addressing my comments! Let's do a final round of testing and then merge

@guitargeek
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@guitargeek guitargeek merged commit 943c973 into root-project:master Apr 19, 2022
@ellert ellert deleted the linkdef branch April 29, 2022 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guitargeek guitargeek guitargeek approved these changes

@lmoneta lmoneta Awaiting requested review from lmoneta lmoneta is a code owner

@sitongan sitongan Awaiting requested review from sitongan

@hageboeck hageboeck Awaiting requested review from hageboeck

Assignees

@lmoneta lmoneta

@guitargeek guitargeek

Labels
in:RooFit/RooStats in:TMVA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ellert @phsft-bot @guitargeek @lmoneta