Finish spacetimedb-update functionality #2126

coolreader18 · 2025-01-14T23:17:15Z

Description of Changes

based off #2011

Adds implementations to all the subcommands of spacetime version.

Expected github release artifact structure:

artifact filename: spacetime-{rustc-target-triple}.{ext}
- ext = if windows { "zip" } else { "tar.gz" }
artifact contents:
- spacetimedb-cli[.exe], spacetimedb-standalone[.exe], spacetimedb-update[.exe]

cc @clockworklabs/devops - feedback on this^^ would be nice. I kept the edge cases since that's how it already was and I figured that might make it easier in the release workflow, but honestly I'd prefer if it was just spacetime-{rustc-target-triple}.{ext} if possible. Not sure where the release flow is or if it's even automated. Implemented in #2193.

Expected complexity level and risk

3 - a complex bit of code that's implementing a theoretically perpetual contract between spacetimedb-update, github release artifacts, and the directory structure.

Testing

Manually tested that spacetime version install works
I'm working on an automated regrtest to make sure the functionality works.
reviewer: if you wanna, play around with it and try and catch out any edge cases?

bfops · 2025-01-18T02:05:24Z

(It looks like this currently isn't passing CI, so I'm holding off on review)

crates/auth/Cargo.toml

crates/cli/src/config.rs

crates/update/src/cli/upgrade.rs

crates/update/src/cli/list.rs

crates/cli/src/lib.rs

crates/paths/src/cli.rs

crates/cli/src/subcommands/generate/mod.rs

bfops · 2025-01-29T19:35:27Z

Review notes:

The previous functionality of spacetime version is now accomplished by spacetime --version (and spacetime version list indicates which version is the current one)

crates/cli/src/subcommands/generate/mod.rs

crates/core/src/auth/token_validation.rs

crates/data-structures/Cargo.toml

crates/standalone/src/lib.rs

bfops · 2025-01-29T21:23:20Z

crates/cli/src/subcommands/start.rs

-    let status = cmd
-        .status()
-        .with_context(|| format!("exec failed for {}", bin_path.display()))?;
+    let status = result.with_context(|| format!("exec failed for {}", bin_path.display()))?;


I believe these changes are to address #2133.

Given that that issue has a different purpose and surface area for testing than the update functionality, I would like to suggest addressing it in a separate PR.

Split out into #2200.

bfops · 2025-01-29T21:25:23Z

Note to self: I still need to:

review crates/cli/src/subcommands/version.rs
review crates/paths
review crates/update itself
jot down what I think the surface area for testing is here

crates/update/src/cli/install.rs

crates/paths/src/cli.rs

crates/update/src/cli.rs

coolreader18 · 2025-01-31T18:54:10Z

I removed the rm-core-as-dep-of-cli commit from this branch, I'll do it as a follow-up PR.

crates/update/src/cli/uninstall.rs

bfops · 2025-01-31T20:41:01Z

crates/cli/src/subcommands/version.rs

+    };
+    #[cfg(windows)]
+    let result = {
+        cmd.env("SPACETIMEDB_UPDATE_MULTICALL_APPLET", applet);


is this needed because the args[0] behavior is different on windows than on linux e.g. if going via a shortcut/symlink?

What's the advantage of doing this differently on windows vs unix than just always doing this env-based approach?

Ultimately this is a hack to get behavior like argv0 on windows. In the future, there'll hopefully be a function akin to argv0 on windows, but for now this is the best approximation. rust-lang/rust#66510

That makes sense. I think my question remains though: if the "hack" is good enough for windows, why not just use it on both platforms and have less surface area for edge cases?

I probably don't have an answer for that besides "that feels icky". would you like me to change it?

crates/paths/src/cli.rs

bfops · 2025-01-31T21:37:27Z

(I merged in master to bump CI. The Internal tests were failing due to them running before https://github.com/clockworklabs/SpacetimeDBPrivate/pull/1279 merged).

bfops · 2025-02-04T19:24:46Z

crates/update/src/main.rs

+        let args = args.collect::<Vec<_>>();
+        if args.first().is_some_and(|s| s == "version") {
+            // if the first arg is unambiguously `version`, go straight to `spacetime version`
+            spacetimedb_update_main()


Q: is this case necessary for some reason? Won't the CLI end up delegating back to spacetimedb-update anyway?

Yes, but there's no perfect way for the cli to find the update binary again. If you're running via cargo run or something, it'll find cli_bin_path in ~/.local, but then spacetime version will delegate to a different binary. It all sucks, but I feel this avoids the most bad edge cases.

bfops · 2025-02-04T23:21:42Z

crates/update/src/cli.rs

+        #[clap(allow_hyphen_values = true)]
+        args: Vec<OsString>,
+    },
+    SelfInstall {


I think this subcommand is new since my last review? What motivated this?

bfops · 2025-02-04T23:24:04Z

crates/update/build.rs

+fn main() {
+    let target = std::env::var("TARGET").unwrap();
+    println!("cargo::rustc-env=BUILD_TARGET={target}");
+}


I'm not very familiar with massaging the build process - what's this for?

bfops · 2025-02-04T23:34:12Z

crates/update/src/cli/link.rs

+
+/// Set a local installation of SpacetimeDB as a custom version.
+#[derive(clap::Args)]
+pub(super) struct Link {


I think this is new since my last review - why did we add this in here? It does seem convenient but it's whole extra bit of surface area that I think we could add in a backwards-compatible way later. Was it part of some spec somewhere or did Tyler request it or something?

coolreader18 force-pushed the noa/spacetime-update-impl branch 5 times, most recently from 621fba2 to 59ec2a0 Compare January 17, 2025 18:17

coolreader18 marked this pull request as ready for review January 17, 2025 20:43

coolreader18 requested review from bfops and cloutiertyler as code owners January 17, 2025 20:43

coolreader18 force-pushed the noa/spacetime-update-impl branch from 59ec2a0 to 35e5488 Compare January 21, 2025 18:37

bfops added release-1.0 backward-compatible labels Jan 21, 2025

bfops linked an issue Jan 24, 2025 that may be closed by this pull request

Directory structure - Implement new spacetimedb-update cli #1839

Open