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(service): get rid of cargo dependency #922

Merged
merged 44 commits into from
Jun 6, 2023

Conversation

iamwacko
Copy link
Contributor

Description of change

is_next, is_alpha, ensure_bin, ensure_cdylib, and clean_crate no longer require cargo.

partially closes #756, particularly service part 1.

How Has This Been Tested (if applicable)?

I ran cargo test, cargo fmt, and cargo clippy.

@iamwacko
Copy link
Contributor Author

I have time, so I can do service part 2 as well.

@iulianbarbu
Copy link
Contributor

I have time, so I can do service part 2 as well.

Sounds good @iamwacko ! Let us know if you need help. We will review this whenever it's ready.

@iulianbarbu iulianbarbu added the M label May 16, 2023
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.

It's hard to accurately review Rust code without rust-analyzer at hand, but here are some thoughts I had. Don't worry if I'm wrong about something.

service/src/builder.rs Outdated Show resolved Hide resolved
service/src/builder.rs Outdated Show resolved Hide resolved
service/src/builder.rs Outdated Show resolved Hide resolved
service/src/builder.rs Show resolved Hide resolved
service/src/builder.rs Show resolved Hide resolved
service/src/builder.rs Outdated Show resolved Hide resolved
service/src/builder.rs Show resolved Hide resolved
service/src/builder.rs Outdated Show resolved Hide resolved
service/src/builder.rs Outdated Show resolved Hide resolved
service/src/builder.rs Outdated Show resolved Hide resolved
service/src/builder.rs Show resolved Hide resolved
service/src/builder.rs Outdated Show resolved Hide resolved
service/src/builder.rs Outdated Show resolved Hide resolved
service/src/builder.rs Show resolved Hide resolved
@iulianbarbu
Copy link
Contributor

@iamwacko let me know if you have questions on the changes I proposed and we can set up a call to get through them. The PR is on the right path and we'll just need to polish a bit the code async-wise, while keeping it on par with the old functionality.

We have a codebase walkthrough session next Monday (if that helps) and a mentoring session next Wednesday, but feel free to hit me up with qs in private on Discord if you feel stuck.

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.

I found some more small things.

I read some more about tokio's Command.
It sounds like it is the proper replacement for spawn_blocking + std Command, so that should be fine.

Cargo.toml Outdated Show resolved Hide resolved
service/src/builder.rs Outdated Show resolved Hide resolved
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.

This looks pretty close @iamwacko! I left a few small comments, but I think the main thing we need to fix is sending the build logs to the build_workspace caller, which Iulian pointed out. Let us know if you are stuck on this and need any help!

Cargo.lock Outdated Show resolved Hide resolved
service/src/builder.rs Show resolved Hide resolved
service/src/builder.rs Outdated Show resolved Hide resolved
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.

Nice progress! I took a look at your latest changes and saw what may be an easier way to read the output, let me know if you think it could work!

service/Cargo.toml Outdated Show resolved Hide resolved
service/src/builder.rs Outdated Show resolved Hide resolved
@iamwacko iamwacko marked this pull request as ready for review June 4, 2023 20:08
@iamwacko iamwacko requested a review from iulianbarbu June 4, 2023 20:09
iulianbarbu
iulianbarbu previously approved these changes Jun 5, 2023
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.

Thanks @iamwacko ! This is great 🥳 .

@iulianbarbu
Copy link
Contributor

I think there is a regression on shuttle-next. I am not able to run the next/hello-world because of:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: status: Unknown, message: "failed to read input file", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc", "date": "Mon, 05 Jun 2023 20:43:08 GMT", "content-length": "0"} }', /Users/iulianbarbu/repos/shuttle/cargo-shuttle/src/lib.rs:779:51

@iulianbarbu iulianbarbu dismissed their stale review June 5, 2023 20:43

We must investigate the failing integration test on cargo-shuttle.

@iamwacko
Copy link
Contributor Author

iamwacko commented Jun 5, 2023

I see the problem. - gets turned into _ by cargo, but I didn't account for that so it tries to execute hello-world.wasm instead of hello_world.wasm. I also didn't catch it because for a while cargo-shuttle was failing for unrelated reasons.

@iamwacko
Copy link
Contributor Author

iamwacko commented Jun 6, 2023

Now the cargo-shuttle test is failing with thread 'init::non_interactive_basic_init' panicked at 'Could not query the latest version of shuttle-runtime', cargo-shuttle/src/init.rs:994:33. It works locally, so I have no idea what has gone wrong. I haven't even touched the code that would make this happen.

@iamwacko
Copy link
Contributor Author

iamwacko commented Jun 6, 2023

Looks like the only test failures I have are #821

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.

Thanks @iamwacko for fixing the shuttle-next issue. Yes, it seems we're getting the error corresponding to #821 in CI. The tests pass locally for me as well.

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.

[Enhancement]: Get rid of cargo dependency
5 participants