Makefile support to statically link external plugin code #7918
Conversation
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
I am a little confused as to how this is supposed to work. I am not certain I understand how adding the plugin to the master library solves anything (how do any of the function in the plugin get called in something like db_bench), and I do not understand how the directory tree gets established.
There was a problem hiding this comment.
I am not certain I understand how adding the plugin to the master library solves anything (how do any of the function in the plugin get called in something like db_bench)
This is exactly demonstrated in the example: https://github.com/ajkr/dedupfs
I do not understand how the directory tree gets established.
By the user. See the example
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
2 similar comments
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Makefile
Outdated
| # To achieve this on Mac OS X, the static library must be preceded by `-force_load` | ||
| # linker flag. Elsewhere, the library must be surrounded by `--whole-archive` and | ||
| # `--no-whole-archive`. | ||
| $(warning "platform $(PLATFORM)") |
There was a problem hiding this comment.
Good catch, will remove it.
Makefile
Outdated
| # To achieve this on Mac OS X, the static library must be preceded by `-force_load` | ||
| # linker flag. Elsewhere, the library must be surrounded by `--whole-archive` and | ||
| # `--no-whole-archive`. | ||
| $(warning "platform $(PLATFORM)") |
There was a problem hiding this comment.
Good catch, will remove it.
|
I agree that this is a good first pass and is something that could potentially be marked "experimental". There are quite a few issues I see before we could mark this is "ready":
This work dovetails with some of the work I have been doing in #6591. I will write something up to bring those two efforts together and try to address some of these issues. |
ajkr
force-pushed
the
prototype-external-plugin
branch
from
c072f14
to
3a906c2
Compare
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
|
Should there also be a way to add/build custom tests? For example, if one viewed the "BackupableDB" suite as a plugin, how would one build those tests using this model? |
The release note claims a "mechanism for building external plugin code into the RocksDB libraries/binaries". In my view, the public interface consists of the
This is addressed by no longer adding
I agree these would be nice to have. However, the release note only claims a "mechanism for building external plugin code into the RocksDB libraries/binaries".
I think it is a great long-term direction. As it relates to this PR, I expect the "building-in" mechanism introduced here will still be needed. edit: Actually, the release note should be constrained further to say the mechanism is only available to Makefile users. I will fix that before landing. |
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Added support for detecting plugins linked in the "plugin/" directory and building them from our Makefile in a standardized way. See "plugin/README.md" for details. An example of a plugin that can be built in this way can be found in https://github.com/ajkr/dedupfs. There will be more to do in terms of making this process more convenient and adding support for CMake. Pull Request resolved: facebook#7918 Test Plan: my own plugin (https://github.com/ajkr/dedupfs) and also heard this patch worked with ZenFS. Reviewed By: pdillinger Differential Revision: D26189969 Pulled By: ajkr fbshipit-source-id: 6624d4357d0ffbaedb42f0d12a3fcb737c78f758
Summary: Added support for detecting plugins linked in the "plugin/" directory and building them from our Makefile in a standardized way. See "plugin/README.md" for details. An example of a plugin that can be built in this way can be found in https://github.com/ajkr/dedupfs. There will be more to do in terms of making this process more convenient and adding support for CMake. Pull Request resolved: facebook#7918 Test Plan: my own plugin (https://github.com/ajkr/dedupfs) and also heard this patch worked with ZenFS. Reviewed By: pdillinger Differential Revision: D26189969 Pulled By: ajkr fbshipit-source-id: 6624d4357d0ffbaedb42f0d12a3fcb737c78f758 (cherry picked from commit c16d5a4)
Added support for detecting plugins linked in the "plugin/" directory and building them from our Makefile in a standardized way. See "plugin/README.md" for details. An example of a plugin that can be built in this way can be found in https://github.com/ajkr/dedupfs.
There will be more to do in terms of making this process more convenient and adding support for CMake.
Test Plan: my own plugin (https://github.com/ajkr/dedupfs) and also heard this patch worked with ZenFS.