-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Rework the plugin loading logic #1703
Conversation
7a00a9c
to
d974a91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick first look.
Maybe you can expand the description a bit to include what the specific issues were that you and @mavam were trying to solve with this redesign, otherwise its hard to tell if it succeeds at that or not.
As a general feeling, it seems like expanding the bundled
and all
keywords into a list of plugins could be done outside the routines that handle the actual loading of plugins, to disentangle the code.
changelog/unreleased/breaking-changes/1703--flexible-plugin-loading.md
Outdated
Show resolved
Hide resolved
d974a91
to
d382eb3
Compare
A few things that were all interrelated:
It cannot with the current behavior, unless |
Mixing
|
I'd like to avoid doing that, as adding more options that need special-case handling in the CAF configuration is a pain to maintain. I also think @lava was talking about the code itself more, where we can expand the |
b244b5c
to
11a4194
Compare
d0ec5d6
to
fdbb044
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks generally good to me; just a few stylistic comments below.
Also, I think it would be good to capture the motivation for this PR that you explained in a video call somewhere (maybe a new "feature" changelog entry?), since your previous answer just gave a summary of the PR contents.
This commit contains a set of changes related to how we load plugins. 1. `--plugins` and `--plugin-dirs` options may now be specified on the command line as well as the configuration under `vast.plugins` and `vast.plugin-dirs` respectively. 2. We no longer load static plugins by default. 3. The CMake build option `VAST_ENABLE_PLUGIN_AUTOLOADING` no longer exists. 4. Adding `bundled` to `vast.plugins` causes all bundled plugins, i.e., static or dynamic plugins built alongside VAST to be loaded. 5. Adding `all` to `vast.plugins` causes all bundled *and* external plugins to be loaded. To test this change I recommend the following steps: 1. Test various combinations of the new plugin option with a bundled plugin in a standard location. ```bash cmake -B build.vast -D "VAST_PLUGINS=${PWD}/plugins/pcap" cmake --build build.vast cmake --install build.vast --prefix "${PWD}/install.vast" vast -v --plugins=... version ``` 2. Test various combinations of the new plugin option with an external plugin in a non-standard location and a bundled plugin in a standard location. ```bash VAST_DIR="${PWD}/install.vast" -S example/plugins/analyzer -B build.analyzer cmake --build build.analyzer vast -v --plugins=... --plugin-dirs="${PWD}/build.analyzer/lib/vast/plugins" version ``` 3. Test various combinations of the new plugin option with an external plugin in a non-standard location and a bundled plugin in a standard location. ```bash cmake --install build.analyzer --prefix "${PWD}/install.vast" vast -v --plugins=... version ```
This implicitly enables all plugins in the Docker image.
This came up in a review, but was considered too complex of a change to fit in the scope of the existing set of changes. This ensures we don't forget about it.
93e6076
to
7e7b10f
Compare
📔 Description
This PR contains a set of changes related to how we load plugins.
--plugins
and--plugin-dirs
options may now be specified on the command line as well as the configuration undervast.plugins
andvast.plugin-dirs
respectively.VAST_ENABLE_PLUGIN_AUTOLOADING
no longer exists.bundled
tovast.plugins
causes all bundled plugins, i.e., static or dynamic plugins built alongside VAST, to be loaded.all
tovast.plugins
causes all bundled and external plugins to be loaded.📝 Checklist
🎯 Review Instructions
Review code file-by-file.
Regarding the design: I went over this with @mavam and we agreed this was a good UX improvement. I will wait until after the initial review with writing the documentation for this, though.
To test this change I recommend the following steps:
cmake --install build.analyzer --prefix "${PWD}/install.vast" vast -v --plugins=... version