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

t_pyLCIO_import test broken on some systems #134

Closed
tmadlener opened this issue May 4, 2021 · 5 comments · Fixed by #136
Closed

t_pyLCIO_import test broken on some systems #134

tmadlener opened this issue May 4, 2021 · 5 comments · Fixed by #136

Comments

@tmadlener
Copy link
Contributor

The t_pyLCIO_import test seems to be broken (at least partially). See, e.g. the CI workflow outputs of #132: e.g. centos

I can reproduce this locally on Ubuntu as well

  • OS version: Ubuntu 20.04
  • Compiler version: GCC 9.3
  • Package version: master
  • root version: 6.22/08
@tmadlener
Copy link
Contributor Author

This is because of the following loading logic:

lcioPath = os.environ.get( 'LCIO' )
if not lcioPath:
print('Environment variable $LCIO is not defined!', file=sys.stderr)
sys.exit( 2 )
if not os.path.exists( lcioPath ):
print('LCIO path %s does not exist' % (lcioPath))
sys.exit( 2 )
liblcioPath = os.path.join( lcioPath, 'lib', 'liblcio.so' )
# See http://root.cern.ch/root/html/TSystem.html#TSystem:Load for error codes
result = gSystem.Load( liblcioPath )
if result not in [0, 1]:
print('Error loading %s' % (liblcioPath), file=sys.stderr)
sys.exit( 2 )
liblcioDictPath = os.path.join( lcioPath, 'lib', 'liblcioDict.so' )
# See http://root.cern.ch/root/html/TSystem.html#TSystem:Load for error codes
result = gSystem.Load( liblcioDictPath )

This logic only works if LCIO is installed back into its source directory, because it implicitly assumes where the libraries should be located and tries to load them via a full path. However, this is not strictly necessary, as ROOT will look on LD_LIBRARY_PATH in any case if just the library name is provided.

@gaede
Copy link
Contributor

gaede commented May 4, 2021

Yes, we have in principle always made in-source installations for LCIO and iLCSoft releases. This is exactly the kind of stuff, we'd have to fix across all packages when modernizing the cmake...

@tmadlener
Copy link
Contributor Author

But even with the in-source installation the LD_LIBRARY_PATH points to the correct lib (or lib64) directory, right? So the lookup would not need to depend on the way things are installed as long as LD_LIBRARY_PATH is set correctly.

@gaede
Copy link
Contributor

gaede commented May 5, 2021

Yes, this was just meant as an explanation, why it was done that way. Please feel free to change as needed...

@tmadlener
Copy link
Contributor Author

OK. Very good, I just wanted to make sure my assumption about the LD_LIBRARY_PATH is correct. In #136 I have made the lookup such that we first try to use ROOT and its automatic loading via LD_LIBRARY_PATH, before we fall back to the in-source installation lookup logic.

As a nice side-effect this makes this test independent of the way LCIO is installed (or would be installed) and now all tests can be run without installing first.

@gaede gaede closed this as completed in #136 May 5, 2021
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 a pull request may close this issue.

2 participants