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

[Enhancement]: Get rid of cargo dependency #756

Closed
jonaro00 opened this issue Mar 27, 2023 · 8 comments · Fixed by #922
Closed

[Enhancement]: Get rid of cargo dependency #756

jonaro00 opened this issue Mar 27, 2023 · 8 comments · Fixed by #922
Labels
T-Improvement Improvement or addition to existing features

Comments

@jonaro00
Copy link
Member

jonaro00 commented Mar 27, 2023

Current uses of bundled cargo (three subtasks):

  • cargo-shuttle - Done Remove cargo from cargo shuttle #765
    • When using shuttle init. This can be replaced with a Command call.
    • ToSemver when checking versions of cargo-shuttle and runtime. This is not strictly needed, a == is probably sufficient. (or just semver crate)
  • deployer - Done feat: show output of failed tests #907
    • CliError is used when pre deploy testing. (can be replaced)
    • Various enums and config struct used to construct the test command. All options used are easily used from CLI args.
  • service part 1 - Done feat(service): get rid of cargo dependency #922
    • is_next and ensure_{bin,cdylib}: can be done through cargo_metadata.
    • clean_crate: should be easily done with a command.
  • service part 2 - Done feat(service): get rid of cargo dependency #922
    • get_config: gets cargo config, but is never modified before use, so it is not needed if bundled cargo is not used.
    • get_compile_options: uses standard CLI options. (minor TODO here is to not limit cpus to 4 on local release run)
    • check_no_panic: could be done by parsing toml i guess... (main challenge here is that cargo config is nightly) -- can probably be dropped instead [Enhancement]: Get rid of cargo dependency #756 (comment)
    • build_workspace: figure out another way to find the crates of the workspace, as well as getting the data we need for the BuiltService struct.

All done 🥳

@oddgrd oddgrd added the T-Improvement Improvement or addition to existing features label Mar 27, 2023
@oddgrd
Copy link
Contributor

oddgrd commented Mar 27, 2023

Thanks again for this writeup @jonaro00, this looks good to me! For anyone that wants to pick this one up, it would be preferable to split this up into separate PRs, one for each crate outlined in the issue.

One thing we might want to consider as well is giving a warning if the users version of rustc is higher than the one in our deployer images, since it might then fail to compile if the users source-code uses new language features that are not available in our version of cargo. 🤔

As for your additional suggestion, I'll need to think more about that one.

@jonaro00
Copy link
Member Author

Yeah, at this point it is mainly a thing that could be kept in mind when designing this rewrite.

@oddgrd
Copy link
Contributor

oddgrd commented Mar 28, 2023

Before anyone takes this one further, we are currently working on supporting workspaces, so it may be good to hold off on this until we can verify that we can do that without the cargo library.

@chesedo
Copy link
Contributor

chesedo commented Mar 29, 2023

The workspace support changes are in #767

@chesedo
Copy link
Contributor

chesedo commented Mar 29, 2023

With the new runtime, I think we can also drop check_no_panic when making this change

@gautamprikshit1
Copy link
Contributor

How do I test the deployer?

@oddgrd
Copy link
Contributor

oddgrd commented Apr 24, 2023

First run USE_PANAMAX=disable make images, then follow these steps: https://github.com/shuttle-hq/shuttle/blob/main/CONTRIBUTING.md#testing-deployer-only

You can also follow the contributing guide to set up the full shuttle environment locally, but following this guide to setup just deployer without gateway allows much faster iteration.

@oddgrd
Copy link
Contributor

oddgrd commented Apr 24, 2023

I am working on a refactor of the local deployer workflow by the way, which should be merged to main within a few days: #810 This will make it easier to run deployer locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Improvement Improvement or addition to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants