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

Better handling of errors during dynamic catalogue loading #1702

Merged
merged 10 commits into from
Oct 12, 2021

Conversation

thorstenhater
Copy link
Contributor

  • Add platform specific handling of shared object loading
    • handle, dl_open, dl_get_symbol, dl_close
    • exceptions
    • currently POSIX only (BSD, Mac and Linux)
      • These have well-behaved const char* error messages
  • All of the above may be depending on the platform, so wrap into a header util/dl.hpp and select correct platform by CPP.
    • Currently Mac and Linux are detected, everything else throws a compile error.
    • BSD should be trivial, but we cannot test.
  • Refine error handling when loading catalogues
    • Test for file existence first
    • catch platform dependent errors from dl.hpp and wrap them in bad_catalogue_error

#endif

#ifndef DL
#error "No platform with dynamic loading found"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that's true: Windows also has the concept of dynamic loading, right? I can ctypes.CDLL("my.dll") or LoadLibrary("my.dll"); on Windows.

Copy link
Contributor Author

@thorstenhater thorstenhater Oct 7, 2021

Choose a reason for hiding this comment

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

Yes it does. However, it does not use the POSIX/Unix way using dlopen, but rather the Win32 LoadLibrary. So, the compiler error is the prompt for us implement the API outlined in dl_platform_posix.hpp when the time should come to port to Windows. Same story for BSD: Once we compile there, we need to test dl_platform_posix.hpp against that OS, add an ifdef, and move on. But as long as we do not regularly test on those platforms, I would rather not enable them.

The error message should probably say "No platform support for dynamic loading found" though

@brenthuisman
Copy link
Contributor

Might need to namespace the expected error: arbor._arbor.FileNotFoundError.

@thorstenhater
Copy link
Contributor Author

thorstenhater commented Oct 8, 2021

Might need to namespace the expected error: arbor._arbor.FileNotFoundError.

Issue is the semantics of register_exception. It will create a new Py Exception in the module.
We need however the global one.

Update: Fix is to use register_exception("Name", PyC_FileNotFoundError), making Name but inheriting from FNFE.
Working in CI right now.

@brenthuisman brenthuisman self-requested a review October 8, 2021 11:02
Copy link
Contributor

@brenthuisman brenthuisman left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@noraabiakar noraabiakar left a comment

Choose a reason for hiding this comment

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

Looks great thanks! I have a few questions about that code and the possible problems we can run in to.

@thorstenhater thorstenhater merged commit 71b9515 into arbor-sim:master Oct 12, 2021
@thorstenhater thorstenhater deleted the feature/dl-ng branch October 12, 2021 08:12
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