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

in tests_acq/guide: close temp file before unlinking it #294

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

javierggt
Copy link
Contributor

@javierggt javierggt commented Feb 15, 2024

Description

In tests_acq/guide: close temp file before unlinking it.

From the docs:

mkstemp() returns a tuple containing an OS-level handle to an open file (as would be returned by os.open())

Since the file is open, it should be close before unlinking. Otherwise, one gets this error on windows:

        fh, fn = tempfile.mkstemp(suffix='.h5')
>       os.unlink(fn)
E       PermissionError: [WinError 32] The process cannot access the file because it is being used by another process

I know we would not use this on windows most probably, but still...

Interface impacts

Testing

Unit tests

  • No unit tests
  • Mac
  • Linux
  • Windows

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

After this PR, the test do not fail at this point in Windows. They fail later because I test on a VM, and the symbolic links in mica/archive/obspar are not supported in the VM

Copy link
Member

@taldcroft taldcroft 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. In modern code we'd probably use a pytest fixture to get a temporary directory, but this is fine here.

@javierggt javierggt merged commit 52d27f9 into master Feb 27, 2024
@jeanconn jeanconn deleted the close-before-unlink branch February 27, 2024 18:36
This was referenced Mar 6, 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.

2 participants