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

Split binaries #2011

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Split binaries #2011

wants to merge 2 commits into from

Conversation

coolreader18
Copy link
Collaborator

@coolreader18 coolreader18 commented Nov 26, 2024

Description of Changes

Resolves #1838

Adds a spacetimedb-update multicall binary that proxies to spacetimedb-cli, and makes spacetime run call out to the spacetimedb-standalone binary instead of embedding the whole server in the cli. Functionality for the update binary itself forthcoming in another PR.

Expected complexity level and risk

3 - adds the proxy binary and changes how run works under-the-hood, but the deeper changes will come in the follow-up.

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

  • Smoketests use new proxy binary and work.
  • Manually set up a test environment and spacetimedb-update proxied to the cli binary.

@coolreader18 coolreader18 force-pushed the noa/split-binaries branch 2 times, most recently from e1cfc8b to 3c2be0c Compare December 9, 2024 18:54
@coolreader18 coolreader18 marked this pull request as ready for review December 10, 2024 00:19
}
proxy::run_cli(&paths, None, cli_args)
}
_ => unimplemented!(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that we'll implement these in another PR? Note that spacetimedb upgrade is already a thing. We probably want to remove that with that PR.

@gefjon
Copy link
Contributor

gefjon commented Dec 30, 2024

Needs PR description according to template.

@gefjon
Copy link
Contributor

gefjon commented Dec 30, 2024

PR description should include what the correct way for a reviewer to invoke the CLI is now. In the past I've been doing cargo run --manifest-path ~/clockworklabs/SpacetimeDB/Cargo.toml -- $@. This appears to not hit the new functionality. Changing the manifest-path to SpaceitmeDB/crates/update/Cargo.toml appears to not exercise the new functionality either, as that binary is looking to be invoked as spacetime. cargo install --path SpacetimeDB/crates/update also does not allow me to test the new functionality, as it installs as spacetimedb-update.

I also notice that you've updated the smoketests to invoke the inner spacetimedb-cli rather than the spacetime wrapper, which concerns me, as it seems the new functionality is untested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Directory structure - Split spacetime into multiple binaries
4 participants