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

Allow plugins to report their own version and store it in the registry #12883

Merged
merged 6 commits into from
Jun 21, 2024

Conversation

devyn
Copy link
Contributor

@devyn devyn commented May 16, 2024

Description

This allows plugins to report their version (and potentially other metadata in the future). The version is shown in plugin list and in version.

The metadata is stored in the registry file, and reflects whatever was retrieved on plugin add, not necessarily the running binary. This can help you to diagnose if there's some kind of mismatch with what you expect. We could potentially use this functionality to show a warning or error if a plugin being run does not have the same version as what was in the cache file, suggesting plugin add be run again, but I haven't done that at this point.

It is optional, and it requires the plugin author to make some code changes if they want to provide it, since I can't automatically determine the version of the calling crate or anything tricky like that to do it.

Example:

> plugin list | select name version is_running pid
╭───┬────────────────┬─────────┬────────────┬─────╮
│ # │      name      │ version │ is_running │ pid │
├───┼────────────────┼─────────┼────────────┼─────┤
│ 0 │ example        │ 0.93.1  │ false      │     │
│ 1 │ gstat          │ 0.93.1  │ false      │     │
│ 2 │ inc            │ 0.93.1  │ false      │     │
│ 3 │ python_example │ 0.1.0   │ false      │     │
╰───┴────────────────┴─────────┴────────────┴─────╯

cc @maxim-uvarov (he asked for it)

User-Facing Changes

  • plugin list gets a version column
  • version shows plugin versions when available
  • plugin authors must add fn version() to their impl Plugin

Tests + Formatting

Tested the low level stuff and also the plugin list column.

After Submitting

  • update plugin guide docs
  • update plugin protocol docs (Metadata call & response)
  • update plugin template (fn version() should be easy)
  • release notes

```
> plugin list | select name version is_running pid
╭───┬────────────────┬─────────┬────────────┬─────╮
│ # │      name      │ version │ is_running │ pid │
├───┼────────────────┼─────────┼────────────┼─────┤
│ 0 │ example        │ 0.93.1  │ false      │     │
│ 1 │ gstat          │ 0.93.1  │ false      │     │
│ 2 │ inc            │ 0.93.1  │ false      │     │
│ 3 │ python_example │ 0.1.0   │ false      │     │
╰───┴────────────────┴─────────┴────────────┴─────╯
```
@fdncred
Copy link
Collaborator

fdncred commented May 16, 2024

I think it's a great idea to have plugin metadata for version and other potential stuff but I'm on the fence about making it optional. If someone wants to rely on it and it's optional, it makes it not as helpful because people can opt-out.

@devyn
Copy link
Contributor Author

devyn commented May 16, 2024

I can design the rust API to make it required then, e.g. with a fn version() For others it should be obvious because they'll have to implement Metadata.

@fdncred
Copy link
Collaborator

fdncred commented May 17, 2024

I could be wrong, but I think that sounds better.

@devyn
Copy link
Contributor Author

devyn commented May 17, 2024

I think this does that, then. The fn version(&self) -> String is required to be implemented in Rust, but it's technically an optional field still as far as the protocol goes.

@fdncred fdncred added the plugins This issue is about plugins label Jun 15, 2024
@devyn
Copy link
Contributor Author

devyn commented Jun 21, 2024

@fdncred I'm up for giving this a try for this release if you don't mind. It's nothing big and scary but does create a small breaking change for plugin developers.

@fdncred
Copy link
Collaborator

fdncred commented Jun 21, 2024

ok, here's to breaking everyone's plugins again. 🍺

@fdncred fdncred merged commit 91d44f1 into nushell:main Jun 21, 2024
13 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Jun 21, 2024

After changing a bunch of plugins, shouldn't this just default to

    fn version(&self) -> String {
        env!("CARGO_PKG_VERSION").into()
    }

and not be a breaking change. If people want a different version, they could override it.

@hustcer hustcer added this to the v0.95.0 milestone Jun 21, 2024
@devyn
Copy link
Contributor Author

devyn commented Jun 22, 2024

After changing a bunch of plugins, shouldn't this just default to

    fn version(&self) -> String {
        env!("CARGO_PKG_VERSION").into()
    }

and not be a breaking change. If people want a different version, they could override it.

I wish we could, but the env! macro is local to the crate that's compiling it, so a default implementation would pick up the version of nu-plugin-core rather than the plugin version. 😢

@devyn devyn deleted the plugin-version-reporting branch June 22, 2024 20:35
@fdncred
Copy link
Collaborator

fdncred commented Jun 22, 2024

boo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugins This issue is about plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants