Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

update rust version to 1.85.0 #3085

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

YJDoc2
Copy link
Collaborator

@YJDoc2 YJDoc2 commented Feb 25, 2025

Description

This updates rust version to latest stable 1.85.0. This also aligns with the new rust editions, and some deps like wasmtime require us to update our rust version to 1.82+ anyways.

The code changes are simply fixes suggested by clippy.

NOTE: it seems cross-rs has not yet caught up with latest rust version, so it is failing to compile. Will need to wait for them, or use pre-compiled binaries to avoid this.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test updates
  • CI/CD related changes
  • Other (please describe): Rust version update

Testing

  • Added new unit tests
  • Added new integration tests
  • Ran existing test suite
  • Tested manually (please provide steps)

Related Issues

N/A

Additional Context

Even though the fix suggestions by clippy should be correct, we should have another look for confirmation and sanity checks.

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>
@YJDoc2 YJDoc2 added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Feb 25, 2025
Comment on lines +226 to +229
// given that start has failed, we can be pretty sure that create has either failed
// or completed already, so we wait on it so it does not become a zombie process
let _ = create_process.wait_with_output();
return TestResult::Failed(anyhow!("container start failed : {:?}", e));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would like someone to take another look at this, the clippy error was that in the early return codepath here, we do not wait on the create process, hence it can become a zombie process. Hence I added a wait here with assumption that if the start_container has failed, we can be fairly certain that the original create has also errored or has already exited.

However, would like someone else to confirm the logic.

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we create a different PR for only this part? This could be an important change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey, ok, I'll create a separate PR just for this change, and once that is resolved I can rebase this PR.

@YJDoc2 YJDoc2 requested a review from a team February 25, 2025 10:35
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants