Skip to content

os.remove is documented to raise IsADirectoryError but macOS raises PermissionError #95815

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

Closed
chrisburr opened this issue Aug 9, 2022 · 4 comments
Labels
docs Documentation in the Doc dir OS-mac

Comments

@chrisburr
Copy link
Contributor

chrisburr commented Aug 9, 2022

Documentation

In the documentation for os.remove it states:

Remove (delete) the file path. If path is a directory, an IsADirectoryError is raised. Use rmdir() to remove directories. If the file does not exist, a FileNotFoundError is raised.

This is the case on Linux:

$ python -c 'import os, tempfile; os.remove(tempfile.mkdtemp())'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
IsADirectoryError: [Errno 21] Is a directory: '/tmp/tmphxrd1p6n'

However on macOS this results in a PermissionError:

$ python3 -c 'import os, tempfile; os.remove(tempfile.mkdtemp())'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
PermissionError: [Errno 1] Operation not permitted: '/var/folders/1g/q2y74s696ng0v_cmb9sv7qgr0000gn/T/tmpawxhz8wm'

Linked PRs

@chrisburr chrisburr added the docs Documentation in the Doc dir label Aug 9, 2022
@mdboom mdboom added the OS-mac label Aug 9, 2022
@eryksun
Copy link
Contributor

eryksun commented Aug 12, 2022

In Windows, calling os.unlink() or os.remove() on a directory also raises PermissionError. It can be clarified by checking the last status value from the internal NT API. Unfortunately, Microsoft doesn't document either the function to return this value or when it's valid. For example:

import ctypes
import os
import tempfile

RtlGetLastNtStatus = ctypes.WinDLL('ntdll').RtlGetLastNtStatus
STATUS_FILE_IS_A_DIRECTORY = ctypes.c_long(0xC000_00BA).value

def test():
    path = tempfile.mkdtemp()
    try:
        os.remove(path)
    except PermissionError as e:
        if RtlGetLastNtStatus() != STATUS_FILE_IS_A_DIRECTORY:
            raise
        raise IsADirectoryError(errno.EISDIR,
                                os.strerror(errno.EISDIR),
                                e.filename,
                                e.winerror)
>>> try: test()
... except OSError as ex: e = ex
...
>>> e
IsADirectoryError(13, 'Is a directory')
>>> e.__context__
PermissionError(13, 'Access is denied')

>>> os.rmdir(e.filename)

@ronaldoussoren
Copy link
Contributor

The behaviour on macOS matches the documentation of the underlying os, the manpage for unlink(2) and unlinkat(2) mention:

     [EPERM]            The named file is a directory and the effective user ID of the process is not the super-user.

IMHO we shouldn't try to work around this, but should mention this in the documentation.

@ronaldoussoren
Copy link
Contributor

The current text was introduced in #14262, replacing OSError in the text. In this case the new more specific exception is incorrect.

ronaldoussoren added a commit to ronaldoussoren/cpython that referenced this issue Nov 18, 2022
os.remove can raise PermissionError instead of IsADirectoryError,
when the object to be removed is a directory (in particular on
macOS).

This reverts a change done in python#14262.
@ronaldoussoren
Copy link
Contributor

I've added a PR that reverts the text back to mentioning OSError instead of IsADirectoryError, but I'm not convinced yet that this is the best way to change the documentation. An alternative is to document both exceptions.

I'm -1 on changing the implementation to raise the documented exception, that's not possible to do without a race condition because there are other reasons for getting EPERM (e.g. the folder in which the to-be-removed object is located is not writable).

ronaldoussoren added a commit that referenced this issue Nov 21, 2022
os.remove can raise PermissionError instead of IsADirectoryError,
when the object to be removed is a directory (in particular on
macOS).

This reverts a change done in #14262.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 21, 2022
…99571)

os.remove can raise PermissionError instead of IsADirectoryError,
when the object to be removed is a directory (in particular on
macOS).

This reverts a change done in pythonGH-14262.
(cherry picked from commit 1cae31d)

Co-authored-by: Ronald Oussoren <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 21, 2022
…99571)

os.remove can raise PermissionError instead of IsADirectoryError,
when the object to be removed is a directory (in particular on
macOS).

This reverts a change done in pythonGH-14262.
(cherry picked from commit 1cae31d)

Co-authored-by: Ronald Oussoren <[email protected]>
ronaldoussoren added a commit that referenced this issue Nov 21, 2022
#99641)

GH-95815: Document less specific error for os.remove (GH-99571)

os.remove can raise PermissionError instead of IsADirectoryError,
when the object to be removed is a directory (in particular on
macOS).

This reverts a change done in GH-14262.
(cherry picked from commit 1cae31d)

Co-authored-by: Ronald Oussoren <[email protected]>

Co-authored-by: Ronald Oussoren <[email protected]>
ronaldoussoren added a commit that referenced this issue Nov 21, 2022
#99639)

GH-95815: Document less specific error for os.remove (GH-99571)

os.remove can raise PermissionError instead of IsADirectoryError,
when the object to be removed is a directory (in particular on
macOS).

This reverts a change done in GH-14262.
(cherry picked from commit 1cae31d)


Co-authored-by: Ronald Oussoren <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir OS-mac
Projects
None yet
Development

No branches or pull requests

4 participants