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 printout on import when not built with data source #679

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

m-fila
Copy link
Contributor

@m-fila m-fila commented Sep 18, 2024

BEGINRELEASENOTES

  • Fixed missing libpodioDataSourceDict printout on import podio when built without data source

ENDRELEASENOTES

Missed from #674. When library is not found root will make a long printout. This is fixed by first checking if the file exists which can be actually silenced to just return value

@tmadlener tmadlener merged commit 03a75ee into AIDASoft:master Sep 18, 2024
18 checks passed
@m-fila m-fila deleted the fix_py_datasource branch September 18, 2024 18:20
@jmcarcell
Copy link
Member

But the printout is useful because it shows you where ROOT looked so even if you are inexperienced it's possible to infer that LD_LIBRARY_PATH is contained there in case there is some issue with that, now it's an error that doesn't tell you anything.

@tmadlener
Copy link
Collaborator

We are a bit all over the place at the moment here. In some modules we let it fail with printouts, in others we check first like in this PR. Sometimes we make it an ImportError, sometimes we don't (which will probably lead to runtime errors later). I think we should decide what we actually want to do here and then make all of it consistent.

@m-fila
Copy link
Contributor Author

m-fila commented Sep 18, 2024

Here the library looked for is optional and building is enabled with a cmake flag. I wanted to do the same thing as is done with bindings for sio_io
I think getting a printout on each import podio that some library that I intentionally didn't built and I'll not use it - it's not very nice

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