-
-
Notifications
You must be signed in to change notification settings - Fork 222
Fixing failed CI builds caused by remove_dir_all
failure
#618
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
Conversation
d8f5136
to
cc1f412
Compare
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-618 |
There was a problem hiding this 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?
godot-bindings/src/lib.rs
Outdated
if retry_count >= 5 { | ||
panic!( | ||
"cannot remove directory: {path_display} after 5 tries with error: {err}", | ||
path_display = path.display() | ||
) | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!(...)
🙂
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() | |
) |
@Bromeon Yeah, this fixes it, as soon as I changed the dependencies in |
cc1f412
to
f125ce1
Compare
f125ce1
to
a4b1b1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Closes #616