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

Fixing failed CI builds caused by remove_dir_all failure #618

Merged
merged 1 commit into from Feb 21, 2024

Conversation

StatisMike
Copy link
Contributor

Closes #616

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-618

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 🙂
This PR in its current form fixes your downstream race condition?

Comment on lines 174 to 179
if retry_count >= 5 {
panic!(
"cannot remove directory: {path_display} after 5 tries with error: {err}",
path_display = path.display()
)
}
Copy link
Member

Choose a reason for hiding this comment

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

You can use assert_ne! here -- it's impossible that this is ever larger than 5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shoot, botched it a little, and with such straightforward logic. Never gone up to 5 retries yet, but I will change the while loop condition for this to happen (better error message)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested after changing the while loop condition, all green still

Copy link
Member

@Bromeon Bromeon Feb 21, 2024

Choose a reason for hiding this comment

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

Still, the if retry_count >= 5 { panic!(...) } can be simplified using assert_ne!(...) 🙂

Suggested change
if retry_count >= 5 {
panic!(
"cannot remove directory: {path_display} after 5 tries with error: {err}",
path_display = path.display()
)
}
assert_ne!(
retry_count, 5,
"cannot remove directory {path} after 5 tries; error: {err}",
path = path.display()
)

godot-bindings/src/lib.rs Outdated Show resolved Hide resolved
@Bromeon Bromeon added bug c: tooling CI, automation, tools labels Feb 20, 2024
@StatisMike
Copy link
Contributor Author

@Bromeon Yeah, this fixes it, as soon as I changed the dependencies in gd-props branch to the current gdext master the error came back, so this solution seems to work.

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thank you!

@Bromeon Bromeon added this pull request to the merge queue Feb 21, 2024
Merged via the queue into godot-rust:master with commit 6614030 Feb 21, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: tooling CI, automation, tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gdext-dependent crate fails during Github Actions CI build
3 participants