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.

Sign up for GitHub

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

Jump to bottom

Use utils::remove_file in utils::ensure_file_removed #2752

Merged
merged 1 commit into from
May 9, 2021
Merged

Use utils::remove_file in utils::ensure_file_removed #2752

merged 1 commit into from
May 9, 2021

Conversation

joshrotenberg
Copy link
Contributor

@joshrotenberg joshrotenberg commented May 5, 2021 •
edited
Loading

This change calls utils::remove_file in utils::ensure_file_removed instead of calling fs::remove_file in order to take advantage of the retry. The logic is essentially the same: if the result is an error but the error type is NotFound, return Ok(()), otherwise return whatever the actual result is. With this change, the error type is buried a little bit deeper in the result.

In addition, some basic tests have been added to verify the functionality of both remove_file and ensure_file_removed with regards to this change.

Fixes #2734.

@joshrotenberg joshrotenberg marked this pull request as ready for review May 6, 2021 16:28
@kinnison
Copy link
Contributor

kinnison commented May 7, 2021

Is there a good reason that you didn't add those unit-like tests as unit tests in utils.rs, rather than creating an integration test file?

@kinnison kinnison requested a review from rbtcollins May 7, 2021 07:47
@joshrotenberg
Copy link
Contributor Author

joshrotenberg commented May 7, 2021 •
edited
Loading

Is there a good reason that you didn't add those unit-like tests as unit tests in utils.rs, rather than creating an integration test file?

No, actually. I'll move them.

Rustin170506
Rustin170506 reviewed May 8, 2021
View reviewed changes
Copy link
Member

@Rustin170506 Rustin170506 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your contribution.
rest LGTM. It's better to squash your commits.

src/utils/utils.rs Outdated Show resolved Hide resolved
src/utils/utils.rs Outdated Show resolved Hide resolved
src/utils/utils.rs Outdated Show resolved Hide resolved
@joshrotenberg
Copy link
Contributor Author

Thanks for your contribution.
rest LGTM. It's better to squash your commits.

Thanks. I'll remember to squash next time.

@kinnison
Copy link
Contributor

kinnison commented May 8, 2021

I've approved the workflow to run. If everything goes green then I'd like to see a cleaned up commit series before I do a final pass

@joshrotenberg
use remove_file in ensure_file_removed
77eed8d 
unit test for utils::remove_file

add a test for utils::ensure_file_removed

newline

comment potential solution

drill down to find error type

remove integration tests, moved to utils.rs

moved tests here

Update src/utils/utils.rs

Co-authored-by: 二手掉包工程师 <rustin.liu@gmail.com>

Update src/utils/utils.rs

Co-authored-by: 二手掉包工程师 <rustin.liu@gmail.com>

add a message to panic

cargo fmt
@joshrotenberg joshrotenberg force-pushed the bug/ensure_file_removed_retry branch from acf88e8 to 77eed8d Compare May 8, 2021 21:34
@joshrotenberg
Copy link
Contributor Author

I've approved the workflow to run. If everything goes green then I'd like to see a cleaned up commit series before I do a final pass

fmt fixed and commits squashed, but ...

memory allocation of 16777216 bytes failed
Error: Process completed with exit code 1.

I'm guessing this is a transient error, but I don't know for sure.

@kinnison
Copy link
Contributor

kinnison commented May 9, 2021

I've approved the workflow to run. If everything goes green then I'd like to see a cleaned up commit series before I do a final pass

fmt fixed and commits squashed, but ...

memory allocation of 16777216 bytes failed
Error: Process completed with exit code 1.

I'm guessing this is a transient error, but I don't know for sure.

This is part of a problem with Rustup we're trying to bottom out on another PR, so don't worry that you might have caused it.

kinnison
kinnison approved these changes May 9, 2021
View reviewed changes
Copy link
Contributor

@kinnison kinnison 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 good to me, assuming a green CI (modulo any 1.24 related iffies) I'll merge.

@kinnison kinnison merged commit 242d6f6 into rust-lang:master May 9, 2021
@joshrotenberg joshrotenberg deleted the bug/ensure_file_removed_retry branch May 9, 2021 21:10
@rbtcollins
Copy link
Contributor

@joshrotenberg Thank you for picking this bug up and fixing it ! <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rustin170506 Rustin170506 Rustin170506 left review comments

@kinnison kinnison kinnison approved these changes

@rbtcollins rbtcollins Awaiting requested review from rbtcollins

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

latent file removal bug
4 participants
@joshrotenberg @kinnison @rbtcollins @Rustin170506