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

moving param binding helper implementations into header #61

Merged
merged 1 commit into from
Feb 5, 2020

Conversation

laroque
Copy link
Member

@laroque laroque commented Feb 5, 2020

Moving the two helper function implementations back into the headers.

This is because as source, they are used to build libraries of type MODULE_LIBRARY (which are not able to be linked). That isn't a problem for scarab itself, but means that packages which include scarab cannot link those implementations for other libs.

There is some discussion elsewhere (pybind/cmake_example#11 (comment)) about how to achieve a more desirable pattern (with headers defining the bindings themselves, but required non-trivial implementations in source files and building a supporting library that can be linked elsewhere). We don't currently have bandwidth to continue exploring those patterns.

@laroque
Copy link
Member Author

laroque commented Feb 5, 2020

As an extra note: The motivation for this change was so that dripline-python could convert back and forth between param_node and python dictionary types and thus be able to pass content into the param_node& a_config argument to many dripline-cpp classes. I tested this by assigning the alerts-exchange in a Service config in the YAML and it worked as expected.

Copy link
Member

@nsoblath nsoblath left a comment

Choose a reason for hiding this comment

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

Looks good to me

@nsoblath nsoblath merged commit 0821454 into master Feb 5, 2020
@nsoblath nsoblath deleted the hotfix/linking-fails branch February 5, 2020 17:48
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.

2 participants