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

feat: compile an entire workspace #767

Merged

Conversation

chesedo
Copy link
Contributor

@chesedo chesedo commented Mar 29, 2023

Description of change

Allows compiling of an entire workspace. With this, a deploy / local run works from a workspace. For the time being a Shuttle.toml is needed to know the name of the project.

Closes #417

How Has This Been Tested (if applicable)?

New tests have been added. A local deployer and the local runner was also used to make sure everything still works.

Copy link
Member

@jonaro00 jonaro00 left a comment

Choose a reason for hiding this comment

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

Cool stuff. Some questions:

@@ -473,7 +473,7 @@ impl Shuttle {

let service_name = self.ctx.project_name().to_string();

let (is_wasm, executable_path) = match runtime {
let (is_wasm, executable_path) = match runtimes[0].clone() {
Copy link
Member

Choose a reason for hiding this comment

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

Vector can be empty? Also, how does selection of which package to run work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, there will be a follow-up PR to select the correct service to start up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of which package to run, we want to run all of them with one command 😄

Copy link
Member

Choose a reason for hiding this comment

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

Then my prediction is that people will request ways to run only one package, aka cargo shuttle run -p abc 😉


if !alpha_packages.is_empty() {
let opts = get_compile_options(&config, alpha_packages, release_mode, false)?;
let compilation = compile(&ws, &opts)?;
Copy link
Member

@jonaro00 jonaro00 Mar 29, 2023

Choose a reason for hiding this comment

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

Why should all packages be compiled if user only wants to run one of them?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious too if we can build only what's required for the package that will run (meaning its code and its dependencies). I haven't found a way to do it though, from quickly scrapping cargo APIs. @jonaro00 , do you have something in mind for this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently this function checks if a crate in the workspace is a shuttle service (next or alpha), and if it is it compiles it and it's dependencies. This PR is a first step that is focused on compiling the workspace, so it simply starts the first service in the vector. That the user may not want to compile and start all the shuttle services in the workspace is a good point, and we should consider how to handle this in the PRs where we enable starting multiple services in local runs and in deployer.

Copy link
Member

Choose a reason for hiding this comment

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

In a local run in a workspace you would write cargo run -p abc to run the binary for that member. An equivalent way in cargo shuttle would be good, right?

And if running multiple services at once is a goal (as @oddgrd's comment suggests), perhaps some sort of --all flag on shuttle run?

I'm not aware of the intended way of structuring a workspace with this feature: Is each workspace member supposed to be its own shuttle project, or will all members be grouped into the same project?

Copy link
Contributor Author

@chesedo chesedo Mar 29, 2023

Choose a reason for hiding this comment

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

An equivalent way in cargo shuttle would be good, right?

The longer goal will be for shuttle to start all the services in the correct order. The idea is when all the workspace members relate to a single project - think of a workspace crate a calling workspace crate b over http. So for a local run, this will also ensure than an entire project can be developed and tested with a single command

Copy link
Member

Choose a reason for hiding this comment

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

@chesedo So crates a and b would be deployed as two separate shuttle projects (containers)?

Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

LGTM! I've started looking into starting all the services for local runs, and I think we'll need to get the workspace manifest for each crate from build_workspace too, and the name of the service (maybe just the package name to start), since each crate will have their own resources. But I can do this in the local run PR.

Copy link
Contributor

@iulianbarbu iulianbarbu left a comment

Choose a reason for hiding this comment

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

LGTM overall, but I am interested too about @jonaro00's questions.


if !alpha_packages.is_empty() {
let opts = get_compile_options(&config, alpha_packages, release_mode, false)?;
let compilation = compile(&ws, &opts)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious too if we can build only what's required for the package that will run (meaning its code and its dependencies). I haven't found a way to do it though, from quickly scrapping cargo APIs. @jonaro00 , do you have something in mind for this one?

@chesedo chesedo merged commit 36edf0a into shuttle-hq:main Mar 29, 2023
@chesedo chesedo deleted the feature/eng-560-compile-with-multiple-crates branch March 29, 2023 14:43
oddgrd pushed a commit to oddgrd/shuttle that referenced this pull request Mar 31, 2023
* feat: compile an entire workspace

* feat: run workspace tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error deploying workspace with library crate.
4 participants