Test separately built catalogues #1748
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
bcumming
commented
Nov 8, 2021
•
bcumming
commented
- add a test that tests that a catalogue built separately can be loaded via the Python interface
- further simplification of dynamic library support
- move all platform-specific code into the cpp implementation and out of header.
- Merge branch 'test_catalogue'
- move code that directly handles dl_* inside a translation unit:
- avoid poluting other translation units with system headers.
- remove risk of multiple definitions of functions if the dl_* code
was ever used in more than one translation unit.
- make test that checks whether a catalogue can be loaded in python a stand alone script
- remove redundant CMake requirement that Arbor is being built on UNIX because
Arbor only supports UNIX platforms, and the ARB_HAVE_DL definition implied
that it was possible to build Arbor without it set (which wasn't true)
bcumming
added
the
test
thorstenhater
requested changes
Nov 8, 2021
arbor/util/dylib.cpp
Outdated
| namespace arb { | ||
| namespace util { | ||
|
|
||
| struct dl_handle { |
There was a problem hiding this comment.
This is redundant now, it was meant as an opaque handle, but if it's not exposed, it can go.
scripts/test-catalogue.py
Outdated
| print('ERROR: unable to open catalogue file') | ||
| sys.exit(1) | ||
|
|
||
| print(ctypes.CDLL(catname)) |
There was a problem hiding this comment.
This is also redundant: Debugging left-overs.
scripts/test-catalogue.py
Outdated
| #!/usr/bin/env python3 | ||
|
|
||
| import argparse | ||
| import ctypes |
|
This PR updates pybind11, can we add that to the PR message with the version info? |
|
@noraabiakar I can't understand why the PR shows pybind11 being updated: the pybind11 commit in the branch and in master are exactly the same, and the changes shown in the "Files changed" tab show pybind11 being updated to that same commit. So I don't think anything is actually being changed. It looks like GitHub is getting confused by the slightly convoluted history of the commits in this PR. |
thorstenhater
requested changes
Nov 19, 2021
arbor/util/dylib.hpp
Outdated
| } | ||
|
|
||
| } // namespace util | ||
| } // namespace arbo |
thorstenhater
approved these changes
Nov 26, 2021
bcumming
added a commit
to bcumming/arbor
that referenced
this pull request
Dec 3, 2021
* add a test that tests that a catalogue built separately can be loaded via the Python interface * further simplification of dynamic library support * move all platform-specific code into the cpp implementation and out of header.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.