feat: Add a plugins manifest to allow for more configuration of plugins #660

spacemaison · 2021-08-25T06:10:00Z

feat: Plugins can now be instantiated as headless (paneless)
feat: Config field is passed through from manifest into plugins

TheLostLambda · 2021-09-06T14:09:53Z

Hey @spacemaison ! Sorry for letting this sit around for so long! This looks like a quite promising PR, but, as you point out in your issue on the topic, there are a lot of design decisions to be made here! For example, @imsnif @a-kenji and I have recently come to the decision that we want to somewhat unify the config and layout files (so layouts should be able to contain configuration like keybindings and also this sort of plugin configuration). How exactly this all happens, however, is a bit up in the air still.

I agree also that plugins really need a better way to have a default configuration. We've tossed around the idea of making plugins ZIP files with the WASM and a manifest within them (a bit like a Java .jar) or just using a Default implementation in the code. It seems a bit odd to me that headless plugins need to be configured as such when it really doesn't make sense for them to ever run in a pane, for example. We've previously thought about those implementing a different trait since Plugin contains render and that really doesn't make much sense.

Overall, there is a lot to talk about here! If possible, I'd love to have you join us for tomorrow's maintainer meeting to chat about it over a video call, but I'd also understand if that's not something you're comfortable with. If you can make the meeting (18:00-19:00 BST), then just ping either myself or @imsnif probably on the Discord!

Hopefully chat soon, otherwise we can do some writing to think about an ideal configuration API!

spacemaison · 2021-09-07T07:50:05Z

@TheLostLambda

Sure I'll try and make it tomorrow. I'll ping you in the morning before the meeting.

TheLostLambda · 2021-09-07T18:14:27Z

Hi @spacemaison ! It was nice chatting! As discussed offline, if you're up for removing the Default config stuff that might be best for the moment, and does this new system require every plugin to be listed in the config file before use in a layout?

If I understand correctly, this would break layouts like this?
https://github.com/zellij-org/rust-plugin-template/blob/main/plugin.yaml
(That layout is actually a bit outdated for a couple of other reasons, but I do know that the ability to specify paths in the layout is still something that people are relying on)

Looking forward to getting this merged soon!

spacemaison · 2021-09-07T18:48:15Z

It was nice chatting with you as well. From what I recall, I thought I added a way to load plugins directly from the layout files as well, but maybe I'm misremembering that here. I'm busy for the rest of the day but I'll try and look into it tonight and work on it then.

spacemaison · 2021-09-08T18:06:16Z

Hey @TheLostLambda, you're right I did just abstract away looking up plugins to using a tag name. Speaking with you all about it really helped clarify how handy it might be to have the ability to load and configure plugins directly from the layout files, and as it stands this PR breaks that workflow. @imsnif suggested that we use URL's for plugin lookup and that seems like a good way to support both. For the plugin template we can just use a file URL like so:

---
direction: Horizontal
parts:
  - direction: Vertical
    split_size:
      Fixed: 1
    plugin: tab-bar
  - direction: Vertical
    plugin: "file:target/wasm32-wasi/debug/{{project-name}}.wasm"
  - direction: Vertical
    split_size:
      Fixed: 2
    plugin: status-bar

For plugin lookup from the manifest I suggest we just add a custom zellij: protocol for the URL's. So if you wanted to load the tab-bar plugin you would just specify plugin: "zellij:tab-bar". For this PR we can just limit the valid protocols to those two, or we could expand the scope of this thing and load http and https URL's as well. What do you think?

spacemaison · 2021-09-08T18:12:27Z

I should also add that we don't have to use URL's here. I could just use the tag lookup and then fallback to trying to directly looking up the PathBuf as well. URL's just seemed like an elegant way to support standalone plugin lookup in layout files.

TheLostLambda · 2021-09-08T19:41:37Z

Hi @spacemaison ! I personally like the idea of starting to use URLs. With the changes you are proposing, using the default plugins could still just be something like zellij:strider? If so, go for it!

a-kenji · 2021-09-12T18:39:47Z

Just to add, since I don't know how involved it would be.
I think it should be possible to add the configuration also directly in the layout itself.

---
direction: Horizontal
parts:
  - direction: Vertical
    split_size:
      Fixed: 1
    run:
      plugin: 
        path: "zellij:tab-bar"
        config:  ""
  - direction: Vertical
     run:
        plugin: 
          path: "file:target/wasm32-wasi/debug/{{project-name}}.wasm"
  - direction: Vertical
    split_size:
      Fixed: 2
    run:
      plugin: 
        path: "status-bar"

spacemaison · 2021-09-13T08:39:29Z

Absolutely @a-kenji, I was going to back out of adding the config field to this PR and then start a separate issue for it so we can be sure to get it right.

@TheLostLambda I've refactored everything to use URL's so the plugin templates will work again. I've just about finished rebasing the recent permissions changes into this PR as well, but it's super late for me now so I'm going to finish it off in the morning.

a-kenji · 2021-09-13T17:57:54Z

@spacemaison
No problem, just wanted to make sure it's still in the back of our heads!
Putting it in a separate pr sounds good.

TheLostLambda · 2021-09-13T19:04:15Z

@spacemaison Thanks for keeping at this! Once you feel it's all ready for review, feel free to ping me on Discord!

spacemaison · 2021-09-14T07:34:11Z

Hey @TheLostLambda, apparently I am the midnight coder here. I've finished rebasing main on top of this PR. I'll try and ping you in the morning on Discord to let you know as well.

TheLostLambda

Overall looks great! I left a couple of nitpicks if you are interested, but I'm happy to merge this whenever you'd like me to!

TheLostLambda · 2021-09-19T10:14:58Z

zellij-server/src/wasm_vm.rs

@@ -50,77 +54,48 @@ impl From<&PluginInstruction> for PluginContext {
 #[derive(WasmerEnv, Clone)]
 pub(crate) struct PluginEnv {
    pub plugin_id: u32,
-    pub tab_index: usize,
+    pub plugin: Plugin,


As an extraordinarily small nitpick, I might change the name of the Plugin struct to something like PluginConfig? What do you think?

Sure, you're right it's confusing.

TheLostLambda · 2021-09-19T10:17:35Z

zellij-server/src/wasm_vm.rs

-    let mut plugin_map = HashMap::new();
+    let mut plugin_map = HashMap::<u32, (Instance, PluginEnv)>::new();
+
+    for plugin in plugins.iter() {


I'd personally prefer if this loading was also done through the Load call, but if that's too tricky of a change, it's something I'm happy merging now and cleaning up later :)

TheLostLambda · 2021-09-19T10:19:26Z

zellij-server/src/wasm_vm.rs

-                if _allow_exec_host_cmd {
-                    info!("Plugin({:?}) is able to run any host command, this may lead to some security issues!", path);
+            PluginInstruction::Load(pid_tx, run, tab_index) => {
+                if run._allow_exec_host_cmd {


Especially because I don't think this logging would be printed if the Headless plugins were loaded with permissions? Again, if loading headless plugins through this code-path is too much work, we could just move this if to start_plugin()

Yeah the "when" and "where" of starting headless plugins was something I took into consideration. Starting them right after the wasm thread is forked was just the simplest and most obvious choice for a first pass at it. I absolutely could start them via PluginInstruction::Load message, but I'm worried it might just obfuscate what's happening. Right after the call to init_session in zellij-server/src/lib.rs maybe? Is there a reason you want it changed aside from the aesthetics of it being a bit ugly?

In any event I moved the security warning into the call to start_plugin so that headless plugins get the warning as well.

Mostly just aesthetics to be honest :) Always nice to have one clear code-path as well, so if someone needs to change the loading of plugins in the future, they only have to do so in one place. With that being said, this is certainly something that can wait until a different PR!

TheLostLambda · 2021-09-19T10:21:20Z

zellij-server/src/wasm_vm.rs

-        ))
-        .unwrap()
+    match plugin_env.plugin.run {
+        PluginType::OncePerPane(Some(tab_index)) => {


Another minor nitpick, would you be up for calling this something like PluginType::Pane?

TheLostLambda · 2021-09-19T10:22:22Z

zellij-server/src/wasm_vm.rs

+        }
+        _ => {
+            debug!(
+                "{} - Calling method 'host_set_selectable' does nothing for service plugins",


Would probably be good to standardise on either "Headless" or "Service" plugins – I'd be happy with either!

Thanks for catching this, it was a hold over from the first pass at this when I still called them service plugins

TheLostLambda · 2021-09-19T10:46:16Z

zellij-utils/src/input/layout.rs

+            run: tab
+                .run
+                .map(Run::try_from)
+                // FIXME: This is just Result::transpose but that method is unstable, when it


Is it not already stable? Not sure if this is the one you were talking about: https://doc.rust-lang.org/std/result/enum.Result.html#method.transpose

Yup, it is. I think I was thinking of Result::flatten or something. I changed it, thanks

TheLostLambda · 2021-09-19T10:48:37Z

zellij-utils/src/input/plugins.rs

+
+/// Used in the config struct for plugin metadata
+#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
+pub struct Plugins(HashMap<PluginTag, Plugin>);


Like mentioned before, I'd probably rename this to PluginConfig or PluginMetadata :)

TheLostLambda · 2021-09-19T10:49:56Z

zellij-utils/src/input/plugins.rs

+pub struct Plugins(HashMap<PluginTag, Plugin>);
+
+impl Plugins {
+    pub fn new() -> Self {


Could derive Default if you'd like :)

TheLostLambda · 2021-09-19T10:55:50Z

zellij-utils/src/input/plugins.rs

+    pub fn get(&self, run: impl Borrow<RunPlugin>) -> Option<Plugin> {
+        let run = run.borrow();
+        match &run.location {
+            // FIXME


I assume the // FIXME is about eventually allowing file:... with Headless plugins, but that might be worth writing for whomever stumbles across this in the future!

Yeah more or less. That's where the run configuration from the layout files meets the plugins configuration, the FIXME tag was more of a general reminder to not screw up merging configuration metadata from the layout files, which i promptly ignored when merging in the command execution permission. I added documentation on what the method does at the top to clarify it. Hopefully it will help when extending the layouts API to accept headless plugins among other things.

TheLostLambda · 2021-09-19T11:01:46Z

zellij-utils/src/input/plugins.rs

+#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
+#[serde(rename_all = "kebab-case")]
+pub enum PluginType {
+    // TODO: A plugin with output thats cloned across every pane in a tab, or across the entire


Agreed! I think this might be helpful as another type of Pane as opposed to just a plugin type :) Happy to chat about it down the line!

Yup! Someone in the Discord was doing a pomodoro timer the other day, and they we're asking for what was essentially a "static" plugin pane, or a plugin that runs once and mirrors itself to every pane. I'll open an issue to discuss it after this gets merged.

TheLostLambda · 2021-09-19T11:13:15Z

@spacemaison Oh, actually, could we fix this?

The pane frames get their names from the plugin URL, it might be worth stripping away the zellij: and just using the tag name. For file: it's probably fine (and good) to display it's a non-default plugin (though I suppose you could also strip the file: and just display the path there.

TheLostLambda · 2021-09-19T11:25:46Z

Whoops, okay, maybe I didn't test enough 😅 Running commands also seems broken? Even when _allow_exec_host_cmd is set, it seems to deny permissions. I think that run._allow... is correctly set, but that must get overridden somehow when turning it into a Plugin

This is the test layout I was using:

---
template:
  direction: Horizontal
  parts:
    - direction: Vertical
      borderless: true
      split_size:
        Fixed: 1
      run:
        plugin:
          location: "zellij:tab-bar"
    - direction: Vertical
      body: true
    - direction: Vertical
      borderless: true
      split_size:
        Fixed: 2
      run:
        plugin:
          location: "zellij:status-bar"
tabs:
  - direction: Vertical
    parts:
      - direction: Horizontal
        split_size:
          Percent: 20
        run:
          plugin:
            location: "zellij:strider"
            _allow_exec_host_cmd: true
      - direction: Horizontal

This lets you see the permission messages: tail -f /tmp/zellij-1000/zellij-log/zellij.log

I didn't test things quite enough...

spacemaison · 2021-09-20T08:56:22Z

I fixed displaying the full URL in the pane frames by just displaying the paths of the URL's.

The permission for executing commands should be fixed as well. I wasn't copying over the permission from the layout config when generating the plugin config for plugins with the zellij: URL protocol. I added a test to assert that it's doing the correct thing now.

TheLostLambda · 2021-09-21T14:00:51Z

Thanks for all of the swift and excellent changes! I'll do a bit of testing now and hopefully we can merge things today! (I'm happy to resolve the merge conflicts if you'd like me to as well!)

TheLostLambda · 2021-09-21T14:42:35Z

@spacemaison I'm happy to merge things when you are ready! It actually looks like resolving those conflicts was slightly more complex than I thought (something I probably shouldn't try in the Github web editor 😅), so it would be great if you could take a swing at them! Otherwise I can do so tonight or tomorrow :)

spacemaison · 2021-09-21T15:00:03Z

Hey, sure I'll get to it over the next few hours.

…iguration of plugins

- The Plugins/Plugin structs had "Config" appended to them to clarify that they're metadata about plugins, and not the plugins themselves. - The PluginType::OncePerPane variant was renamed to be just PluginType::Pane, and the documentation clarified to explain what it is. - The "service" nomenclature was completely removed in favor of "headless".

The only time that a plugin location is displayed in Zellij is the border of the pane. Having `zellij:strider` display instead of just `strider` was a little annoying, so we're stripping out the scheme information from a locations display.

spacemaison · 2021-09-21T22:19:33Z

Hey @TheLostLambda, I rebased main back into the PR. In doing so I actually found a bug in the wasm_vm's handling of data directories for plugins. We were taking just the filename of each plugin to create data directories. That means that two different plugins with separate paths but the same filename would share a data directory. For instance, file:/foo/tab-bar.wasm and file:/boo/tab-bar.wasm would clash. To correct that I took the full URL each plugin in order to create the data directories. Aside from that the codes pretty unchanged.

TheLostLambda · 2021-09-22T17:15:16Z

@spacemaison Thanks for the fixes! I actually wanted to change those data directories to be the full paths, but I must have missed that in my earlier reviewing! Thanks for tweaking that!

I'll add some info to the changelog :)

Thank you for all of your hard work here!

… how many hidden panes are (#450) * fix(ui): offset content after viewport construction * Added the feature to display fullscreen information on the second line of the status-bar. * fix(strider): update host mount-point * fix(plugin): create missing data directories as needed * feat(layout): specify only tab name in `tabs` section (#722) Allow specifying only the tab name in the `tabs` section - For example this is now possible: ``` tabs: - name: first parts: - direction: Vertical - direction: Vertical - name: second - name: third ``` For that the tab section defaults the direction to `direction::Horizontal` - Adds an error upon specifying a tab name inside the `parts` section of the tab-layout * docs(changelog): Solely name tab in `tabs` section * feature(release): Copy default config to the examples folder on release (#736) fixes #733 * docs(changelog): Copy example config on release * Update default config (#737) * feat(plugin): add manifest to allow for plugin configuration (#660) * feat(plugins-manifest): Add a plugins manifest to allow for more configuration of plugins * refactor(plugins-manifest): Better storage of plugin metadata in wasm_vm * fix(plugins-manifest): Inherit permissions from run configuration * refactor(plugins-manifest): Rename things for more clarity - The Plugins/Plugin structs had "Config" appended to them to clarify that they're metadata about plugins, and not the plugins themselves. - The PluginType::OncePerPane variant was renamed to be just PluginType::Pane, and the documentation clarified to explain what it is. - The "service" nomenclature was completely removed in favor of "headless". * refactor(plugins-manifest): Move security warning into start plugin * refactor(plugins-manifest): Remove hack in favor of standard method * refactor(plugins-manifest): Change display of plugin location The only time that a plugin location is displayed in Zellij is the border of the pane. Having `zellij:strider` display instead of just `strider` was a little annoying, so we're stripping out the scheme information from a locations display. * refactor(plugins-manifest): Add a little more documentation * fix(plugins-manifest): Formatting Co-authored-by: Jesse Tuchsen <not@disclosing> * chore(docs): update changelog * feat(sessions): mirrored sessions (#740) * feat(sessions): mirrored sessions * fix(tests): input units * style(fmt): make rustfmt happy * fix(tests): make mirrored sessions e2e test more robust * refactor(sessions): remove force attach * style(fmt): rustfmtify * docs(changelog): update change * fix(e2e): retry on all errors Co-authored-by: Brooks J Rady <b.j.rady@gmail.com> Co-authored-by: a-kenji <aks.kenji@protonmail.com> Co-authored-by: spacemaison <tuchsen@protonmail.com> Co-authored-by: Jesse Tuchsen <not@disclosing> Co-authored-by: Aram Drevekenin <aram@poor.dev>

This was referenced Aug 25, 2021

Better plugin configuration #661

Closed

Feature(Plugins): Make Plugins Configurable. #317

Closed

spacemaison force-pushed the plugins-manifest branch from b907afb to 62d7b35 Compare September 6, 2021 06:44

imsnif requested a review from TheLostLambda September 6, 2021 09:32

spacemaison force-pushed the plugins-manifest branch 2 times, most recently from 4b1a37e to 6db5c0f Compare September 13, 2021 23:57

spacemaison mentioned this pull request Sep 14, 2021

fix: Refactor plugin template to use newer API's zellij-org/rust-plugin-template#1

Merged

TheLostLambda previously approved these changes Sep 19, 2021

View reviewed changes

Jesse Tuchsen added 3 commits September 21, 2021 12:04

feat(plugins-manifest): Add a plugins manifest to allow for more conf…

c4b407f

…iguration of plugins

refactor(plugins-manifest): Better storage of plugin metadata in wasm_vm

8baa0c5

fix(plugins-manifest): Inherit permissions from run configuration

311b0e9

Jesse Tuchsen added 5 commits September 21, 2021 12:55

refactor(plugins-manifest): Move security warning into start plugin

320f55a

refactor(plugins-manifest): Remove hack in favor of standard method

fece012

refactor(plugins-manifest): Add a little more documentation

975cbb9

spacemaison force-pushed the plugins-manifest branch from 74297dc to 975cbb9 Compare September 21, 2021 21:55

fix(plugins-manifest): Formatting

a3a14ed

TheLostLambda merged commit c937221 into zellij-org:main Sep 22, 2021

feat: Add a plugins manifest to allow for more configuration of plugins #660

feat: Add a plugins manifest to allow for more configuration of plugins #660

Conversation

spacemaison commented Aug 25, 2021

TheLostLambda commented Sep 6, 2021

spacemaison commented Sep 7, 2021

TheLostLambda commented Sep 7, 2021

spacemaison commented Sep 7, 2021

spacemaison commented Sep 8, 2021

spacemaison commented Sep 8, 2021

TheLostLambda commented Sep 8, 2021

a-kenji commented Sep 12, 2021

spacemaison commented Sep 13, 2021

a-kenji commented Sep 13, 2021

TheLostLambda commented Sep 13, 2021

spacemaison commented Sep 14, 2021

TheLostLambda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacemaison Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacemaison Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

TheLostLambda commented Sep 19, 2021

TheLostLambda commented Sep 19, 2021

spacemaison commented Sep 20, 2021 • edited Loading

TheLostLambda commented Sep 21, 2021

TheLostLambda commented Sep 21, 2021

spacemaison commented Sep 21, 2021

spacemaison commented Sep 21, 2021 • edited Loading

TheLostLambda commented Sep 22, 2021

spacemaison Sep 20, 2021 •

edited

Loading

spacemaison Sep 20, 2021 •

edited

Loading

spacemaison commented Sep 20, 2021 •

edited

Loading

spacemaison commented Sep 21, 2021 •

edited

Loading