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

Fix #1348 #1349

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

Fix #1348 #1349

wants to merge 1 commit into from

Conversation

Octoslav
Copy link

Related Issue \ discussion

Fix #1348

os.replace in bundle with mkstemp could raise PermissionError: [WinError 5] on windows systems.

Patch Description

Replaced os.replace with shutils.move.

Other Information

See details in the issue

@jkhenning
Copy link
Member

@Octoslav, since os.replace should be atomic in Linux, and shutil.move can have different behaviors (such as copying between devices), can you please make this change specific to windows only?

@Octoslav
Copy link
Author

@jkhenning Maybe in this case it is better to just wrap os.replace in a try\except block? Like:

try:
    os.replace(local_filename, temp_filename)
except PermissionError:
    os.replace(local_filename, temp_filename)  # Try again, and if it fails, throw an exception.

What do you think?

@jkhenning
Copy link
Member

If that addresses the issue, I would be fine with it 🙂

@Octoslav Octoslav changed the title Fix #1348 by replacing os.replace with shutils.move Fix #1348 Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: PermissionError: [WinError 5] while uploading dataset
2 participants