docbuild: Use lexists when testing whether a symlink exists

[infinity0]

Otherwise the build fails if the symlink is already there but points to a non-existing path.

Component: build

Author: Ximin Luo

Branch/Commit: 05ee235

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/22438

[infinity0]

Branch: u/infinity0/docbuild__use_lexists_when_testing_whether_a_symlink_exists

[infinity0]

New commits:

0c8139d

Use lexists instead of exists

[infinity0]

Author: Ximin Luo

[infinity0]

Commit: 0c8139d

[videlec]

comment:3

This looks annoying. Why the symlink is pointing to a non-existing directory in the first place?

[infinity0]

comment:4

I can't remember now. Possibly I was messing around with a solution to #21732 or #22088 or just getting the Debian package working in the first place.

But the logic shouldn't break just because I might've been doing unusual things. There's already logic to remove a regular file or a working symlink to a file, so we should also remove broken symlinks.

[jdemeyer]

comment:5

Instead of trying to figure out whether to run unlink() or rmtree(), why not use EAFP principles and use try/except? Something like

try:
    os.unlink(path)
except OSError:
    try:
        shutil.rmtree(path)
    except OSError:
        if os.path.lexists(path):
            raise

PS: I sometimes wonder why Python makes it so hard to implement common shell commands like rm -rf or mkdir -p correctly.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

077f8bc

docbuild: More reliable way of trying to remove static_dir

[sagetrac-git]

Changed commit from 0c8139d to 077f8bc

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c4860bd

docbuild: More reliable way of trying to remove static_dir

[sagetrac-git]

Changed commit from 077f8bc to c4860bd

[infinity0]

comment:8

I tweaked what you suggested to be a bit shorter. It's not fully EAFP but I think the brevity makes up for it.

Interestingly on Python3 they have a special subclass of OSError called IsADirectoryError that would make this code even shorter, but never mind.

[sagetrac-git]

Changed commit from c4860bd to 58c1e3f

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

58c1e3f

docbuild: More reliable way of trying to remove static_dir

[infinity0]

comment:11

whoops, I forgot to re-raise for other types of errors. ¬.¬

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

05ee235

docbuild: More reliable way of trying to remove static_dir

[sagetrac-git]

Changed commit from 58c1e3f to 05ee235

[infinity0]

comment:13

I realised the previous version (in comment:10) is wrong, here is an updated version. If you don't like it, I can also do the longer EAFP version that you posted earlier.

I actually think the first one (my original commit) is best, it's slightly long-winded but at least the error messages are exact. With both this recent version and your EAFP version, the error messages could be misleading, e.g.

Recent version:

• real problem: grand-child file can't be removed because child directory is not writable
• raised error: static_dir is a directory

EAFP version:

• real problem: parent directory is not writable
• raised error: static_dir is not a directory

[jdemeyer]

Reviewer: Jeroen Demeyer

[vbraun]

Changed branch from u/infinity0/docbuild__use_lexists_when_testing_whether_a_symlink_exists to 05ee235