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

Add utility functions to rename a file or folder src to dst with retrying. #1666

Merged
merged 1 commit into from
Mar 29, 2024
Merged

Add utility functions to rename a file or folder src to dst with retrying. #1666

merged 1 commit into from
Mar 29, 2024

Conversation

loonghao
Copy link
Contributor

This PR is try to fix #1665

@loonghao loonghao requested a review from a team as a code owner February 19, 2024 08:03
@loonghao loonghao force-pushed the fix_reanme_in_windows branch from 2ce256f to 4669f95 Compare February 19, 2024 08:05
@JeanChristopheMorinPerso
Copy link
Member

Thanks @loonghao for submitting this PR. My main concern right now is speed. Starting a subprocess on Windows usually adds a noticeable overhead.

Could the code first try to os.rename and only fallback to the subprocess method if it's running on windows and the error is "WinError 5" or something like that?

Also, it would be great if you could add tests to cover all cases covered by the change.

@loonghao loonghao force-pushed the fix_reanme_in_windows branch from 4669f95 to 2972852 Compare March 2, 2024 16:18
@codecov Codecov
Copy link

codecov bot commented Mar 2, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.03%. Comparing base (91db537) to head (dafcd6c).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1666      +/-   ##
==========================================
+ Coverage   57.99%   58.03%   +0.03%     
==========================================
  Files         128      128              
  Lines       17088    17102      +14     
  Branches     3502     3504       +2     
==========================================
+ Hits         9911     9925      +14     
  Misses       6509     6509              
  Partials      668      668

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@loonghao loonghao force-pushed the fix_reanme_in_windows branch from 2972852 to ffd6f30 Compare March 2, 2024 16:31
@loonghao
Copy link
Contributor Author

loonghao commented Mar 2, 2024

@JeanChristopheMorinPerso I have updated the code and added a unit test for this function. Please take another look. Thanks.

JeanChristopheMorinPerso
JeanChristopheMorinPerso requested changes Mar 2, 2024
View reviewed changes
Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso left a comment

Choose a reason for hiding this comment

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

Thanks! There is a bug in the code and I think we can add more tests.

src/rez/utils/filesystem.py Show resolved Hide resolved
src/rez/tests/test_utils.py Outdated Show resolved Hide resolved
@loonghao loonghao force-pushed the fix_reanme_in_windows branch 5 times, most recently from f2977d8 to aae54ed Compare March 3, 2024 05:14
@loonghao
Copy link
Contributor Author

loonghao commented Mar 4, 2024

Thanks! There is a bug in the code and I think we can add more tests.

@JeanChristopheMorinPerso I have updated the code and added more unit tests. Please take another look.

Thanks.

JeanChristopheMorinPerso
JeanChristopheMorinPerso reviewed Mar 4, 2024
View reviewed changes
src/rez/tests/test_utils_filesystem.py Outdated Show resolved Hide resolved
src/rez/tests/test_utils_filesystem.py Outdated Show resolved Hide resolved
src/rez/tests/test_utils_filesystem.py Outdated Show resolved Hide resolved
src/rez/tests/test_utils_filesystem.py Outdated Show resolved Hide resolved
src/rez/tests/test_utils_filesystem.py Outdated Show resolved Hide resolved
src/rez/tests/test_utils_filesystem.py Outdated Show resolved Hide resolved
src/rez/tests/test_utils_filesystem.py Outdated Show resolved Hide resolved
src/rez/utils/filesystem.py Outdated Show resolved Hide resolved
src/rez/utils/filesystem.py Outdated Show resolved Hide resolved
src/rez/utils/filesystem.py Outdated Show resolved Hide resolved
@JeanChristopheMorinPerso
Copy link
Member

Thanks @loonghao ! This should hopefully be my last batch of comments.

@loonghao loonghao force-pushed the fix_reanme_in_windows branch from aae54ed to f81d94d Compare March 5, 2024 01:41
@loonghao
Add utility functions to rename a file or folder src to dst with retr…
dafcd6c 
…ying.

Signed-off-by: loonghao <hal.long@outlook.com>
@loonghao loonghao force-pushed the fix_reanme_in_windows branch from f81d94d to dafcd6c Compare March 5, 2024 01:44
@loonghao
Copy link
Contributor Author

loonghao commented Mar 5, 2024

Thanks @loonghao ! This should hopefully be my last batch of comments.

@JeanChristopheMorinPerso I have made all the changes based on your feedback and addressed all the comments. Could you please take another look. Thanks!

JeanChristopheMorinPerso
JeanChristopheMorinPerso approved these changes Mar 5, 2024
View reviewed changes
Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for taking the time to fix this and address all my comments! If everything goes as planned, this will be released in the next release.

@JeanChristopheMorinPerso JeanChristopheMorinPerso added this to the Next milestone Mar 5, 2024
@JeanChristopheMorinPerso JeanChristopheMorinPerso merged commit d4a2daf into AcademySoftwareFoundation:main Mar 29, 2024
53 checks passed
Pixel-Minions added a commit to Pixel-Minions/rez that referenced this pull request Sep 26, 2024
@Pixel-Minions
Add retry mechanism when renaming folders on Windows (AcademySoftware…
df4cf31 
…Foundation#1666)

This will fix issues when rez-cp could fail in some situations on WIndows.

Signed-off-by: loonghao <hal.long@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeanChristopheMorinPerso JeanChristopheMorinPerso JeanChristopheMorinPerso approved these changes

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

Successfully merging this pull request may close these issues.

Access denied while renaming folder via rez cp from Windows
2 participants
@loonghao @JeanChristopheMorinPerso