Fix for #1809 Python 2.7 incompatibility in the iframe renderer

[carthurs]

Fixes #1809

[jonmmease]

Thanks for catching this and for the fix @carthurs! One comment is that I just looked up the CPython 3 implementation, and it includes a comment that the EEXIST error code isn't necessarily reliable for detecting that mkdir failed due to the existence of a directory.

    try:
        mkdir(name, mode)
    except OSError:
        # Cannot rely on checking for EEXIST, since the operating system
        # could give priority to other errors like EACCES or EROFS
        if not exist_ok or not path.isdir(name):
            raise

Unless you have thoughts otherwise, I'd rather mimic this implementation to be on the safe side.

[carthurs]

Do we really care why it failed, if it failed for a reason other than the dir existing? I feel it'd be better to explicitly list the errors that are acceptable in the guard, rather than CPython's approach of blanket-accepting all errors.

(CPython's implementation is going to swallow any and all errors from mkdir in the case where exist_ok = True and the path exists already.)

With regards to this:

        # Cannot rely on checking for EEXIST, since the operating system
        # could give priority to other errors like EACCES or EROFS

If the OS is (in addition to EEXIST, but with priority) raising EACCES (permission denied) or EROFS (read-only file system) - or any other error really - then I don't see the problem with having that case pass the guard and the exception being re-raised.

Besides, I think mimicking CPython would probably involve writing the recursive function again ourselves - or using pathlib, plus the pathlib2 backport to manage this.

Here's my existing solution, for reference.

        try:
            os.makedirs(filedir)
        except OSError as error:
            if error.errno != errno.EEXIST:
                raise

[casperdcl]

nice python/cpython@a82642f it could be the case that e.g. EACCES is raised even if EACCES isn't the real problem.

I'd say it's an upstream bug @carthurs but we should probably just use the CPython hack.

[carthurs]

@casperdcl looks like it is due to how Windows functions in an edge-case https://bugs.python.org/issue25583

The original solution proposed on there was

if not (exist_ok and path.isdir(name) and
            e.errno in (errno.EEXIST, errno.EACCES)):

But then this was rejected because "mkdirs shouldn't use e.errno at all. Because, as said in docstring, makedirs must first check whether a folder exists, and only then call mkdir."

So I think the fix of checking EACCES and EEXIST here should do the job.

[jonmmease]

I would be more comfortable with the following:

        try:
            os.makedirs(filedir)
        except OSError as error:
            if not path.isdir(name):
                raise

I think this captures the intent of "only raise if we don't end up with the directory existing", without including logic for handling platform differences. I'd rather not need to worry about which error code is raised on, for example, Windows 10 running on a raspberry pi 🙂

[jonmmease]

We're going to release 4.2.0 shortly, so I'll go ahead and merge this and then follow up with a small PR updating the except block as I described above. Thanks again for the report and contribution @carthurs!

[carthurs]

@jonmmease Glad to be of help here! I agree that your suggestion,

try:
           os.makedirs(filedir)
       except OSError as error:
           if not path.isdir(name):
               raise

...explicitly defining what is acceptable for all possible cases - is exactly the right solution in the end. Thanks for the discussion!