-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat(cli): check plugin versions for incompatibilities #13993
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
Conversation
check core plugin versions for incompatibilities between Cargo and NPM releases a plugin NPM/cargo version is considered "incompatible" if their major or minor versions are not equal on dev we show an warning on build we error out (with a `--ignore-incompatible-plugins` flag to prevent that) this is an idea from @oscartbeaumont we've seen several plugin changes that require updates for both the cargo and the NPM releases of a plugin, and if they are not in sync, the functionality does not work e.g. tauri-apps/plugins-workspace#2573 where the change actually breaks the app updater if you miss the NPM update
We also talked about this briefly here #13960 (and in like a hundred discord threads 😂 ) tldr: Tony is mostly concerned about start up time. Similar for me, after all we removed the cli update check for that reason. |
oh and technically we sync plugins on patch versions at the moment. if we want the tool to only check minor then we need to make sure we don't have ipc breaking changes in patch releases (even if we consider it a fix). I don't mind either, we just need to agree on something and i guess document it somewhere. |
Well, should have probably tested the PR first before that. At least from my testing this seems to be pretty much instant. Too fast to measure its impact x) Nice job! Let's wait for the critics ( @Legend-Master ) though. |
Looks really good from a quick glance, offloading to a thread so we don't block startup and using rayon to parallelize the js package manager calls, I'll give it a try as well see if I notice the speed differences |
After some testing, the bottle neck is at getting the javascript packages, I'll see if I could push a change here |
Package Changes Through e66fbb3There are 10 changes which include tauri with minor, tauri-cli with minor, @tauri-apps/cli with minor, tauri-utils with minor, tauri-bundler with minor, tauri-macos-sign with minor, tauri-runtime-wry with minor, tauri-runtime with minor, @tauri-apps/api with minor, tauri-plugin with minor Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
Pushed c6e63a0 I have tested this with Edit: |
.and_then(|p| fs::read_to_string(p.join("Cargo.lock")).ok()) | ||
.and_then(|s| toml::from_str(&s).ok()); | ||
|
||
let know_plugins = helpers::plugins::known_plugins(); |
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.
I think we want to also include @tauri-app/api
here? And maybe also @tauri-app/cli
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.
well we do not have a good way to match those..
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.
we'd need to agree on whether to match the tauri package versions and which packages and patch or minor and then actually do that 🙃
sooner or later we'll probably have to tackle this.
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.
@tauri-app/api
and tauri
will need to match in minor versions at least
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.
except that we failed with that multiple times (especially in v1 at the end). again, i'm not against it but if we add the check here we must start to take this serious.
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.
i think before we actually run the check, we should sync our versions for a while, otherwise we suddenly break most builds
let's sync tauri, api and cli
and maybe pin minors for inner dependencies too
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.
I think we could at least add a check that checks the version of tauri
and @tauri-app/api
here?
} | ||
} | ||
|
||
pub fn current_package_versions( |
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.
Probably want to migrate the other ones to use this one as much as possible to speed things up, could push that to the future though
With the intention being that we want to include tauri versions in the future as well we're a bit in a weird situation here because not checking the patch versions for plugins can be problematic since plugins are synced on the patch level. Some patch releases contained IPC sensitive changes. We could take a step back and require patches to not make any ipc breaking changes (probably means that we mostly get js-only or rust-only PRs) but still keep syncing on patch releases anyway. |
Personally, I think we should sync the versions for the tauri packages as well, like we're currently doing for the plugins, it is easier to understand, that if the versions exactly matches, it will definitely work (if you see the versions being different, your first reaction would be am I doing something wrong that they don't fully match? instead of knowing it's fine as long as they're on the same minor version) For the check and IPC breaking changes though, I think we should keep them in minors only (except for cases where the last minor broke the IPC in some ways and it's for fixing that), so if people want (like if they rely on the Technically, if we do IPC breaking changes in tauri, plugin's global init script ( Footnotes
|
I agree with what you said overall. |
can I merge this? I kinda want to release tauri soon |
Yeah, same thoughts, we could leave this one for now
Yeah sure, I just want to also add the |
we pulled out the core stuff into plugins to get around exactly that lol edit: i think i misunderstood what you said, just act like i'm not here :) |
Added in cac22e4, and LOL, found out we got a mismatch in |
crates/tauri-cli/src/build.rs
Outdated
if let Err(error) = check_mismatched_packages(frontend_dir(), tauri_path) { | ||
log::error!("{error}"); | ||
if !ignore_version_mismatches { | ||
std::process::exit(1); |
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.
This doesn't seem to stop the spawned processes (e.g. vite
, cargo
) though, feels like a pain to deal with 😂
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.
ah right.. no easy way to deal with this unless we go with tokio tasks, easier to have graceful shutdown :/
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.
it's either that or get back to sync (which is ok imo, build isn't run as often - and let's not forget cargo takes 500s while our check takes just 1 😂 )
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.
it's either that or get back to sync (which is ok imo, build isn't run as often - and let's not forget cargo takes 500s while our check takes just 1 😂 )
Yeah, ~1 second is acceptable, let's use the sync version for now, sigh, I wish the cargo build doesn't take that long 😂
Just for the reference time with release build, it took around 500ms to check in plugins-workspace
and ~700ms to check in my own project (with npm list
taking up almost all the time) on my machine with an 11th gen i5 11400
I honestly don't know why those js package managers are so slow (maybe because the js file sizes are too big so parsing them could take a while?), pnpm --version
takes 300-400ms, similar numbers for npm, with our cli, tauri --version
takes only 20-50ms
Thank you everyone for getting this through! So happy to know it's going to be much less likely anyone is going to end up in the same unfortunate position as us with a broken autoupdater in production caused by an |
After NixOS#438726 updated `tauri` to >=2.8.0, it now checks for mismatched versions (tauri-apps/tauri#13993). This was always a problem, it just wasn't checked until now. This patch bumps the NPM versions to match the newer Cargo versions.
After NixOS#438726 updated `tauri` to >=2.8.0, it now checks for mismatched versions (tauri-apps/tauri#13993). This was always a problem, it just wasn't checked until now. This patch bumps the NPM versions to match the newer Cargo versions.
After NixOS#438726 updated `tauri` to >=2.8.0, it now checks for mismatched versions (tauri-apps/tauri#13993). This was always a problem, it just wasn't checked until now. This patch bumps the NPM versions to match the newer Cargo versions.
After NixOS#438726 updated `tauri` to >=2.8.0, it now checks for mismatched versions (tauri-apps/tauri#13993). This was always a problem, it just wasn't checked until now. This patch bumps the NPM versions to match the newer Cargo versions.
After NixOS#438726 updated `tauri` to >=2.8.0, it now checks for mismatched versions (tauri-apps/tauri#13993). This was always a problem, it just wasn't checked until now. This patch bumps the NPM versions to match the newer Cargo versions.
After #438726 updated `tauri` to >=2.8.0, it now checks for mismatched versions (tauri-apps/tauri#13993). This was always a problem, it just wasn't checked until now. This patch bumps the NPM versions to match the newer Cargo versions.
check core plugin versions for incompatibilities between Cargo and NPM releases
a plugin NPM/cargo version is considered "incompatible" if their major or minor versions are not equal
on dev we show an warning
on build we error out (with a
--ignore-incompatible-plugins
flag to prevent that)this is an idea from @oscartbeaumont
we've seen several plugin changes that require updates for both the cargo and the NPM releases of a plugin, and if they are not in sync, the functionality does not work e.g. tauri-apps/plugins-workspace#2573 where the change actually breaks the app updater if you miss the NPM update