Opened 6 years ago

Closed 5 years ago

#22438 closed defect (fixed)

docbuild: Use lexists when testing whether a symlink exists

Reported by: Ximin Luo Owned by:
Priority: major Milestone: sage-7.6
Component: build Keywords:
Cc: Merged in:
Authors: Ximin Luo Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 05ee235 (Commits, GitHub, GitLab) Commit: 05ee235086d919597e984cb77d0d242ffb855096
Dependencies: Stopgaps:

Status badges

Description (last modified by Ximin Luo)

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

Change History (15)

comment:1 Changed 6 years ago by Ximin Luo

Branch: u/infinity0/docbuild__use_lexists_when_testing_whether_a_symlink_exists

comment:2 Changed 6 years ago by Ximin Luo

Authors: Ximin Luo
Commit: 0c8139dcef25ad108c19672e1e569fb24eea0c2e
Component: PLEASE CHANGEbuild
Description: modified (diff)
Status: newneeds_review
Type: PLEASE CHANGEdefect

New commits:

0c8139dUse lexists instead of exists

comment:3 Changed 6 years ago by Vincent Delecroix

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

comment:4 Changed 6 years ago by Ximin Luo

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.

comment:5 Changed 6 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

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.

comment:6 Changed 6 years ago by git

Commit: 0c8139dcef25ad108c19672e1e569fb24eea0c2e077f8bc9b554e961134b3e5ef923e315790ab5e8

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

077f8bcdocbuild: More reliable way of trying to remove static_dir

comment:7 Changed 6 years ago by git

Commit: 077f8bc9b554e961134b3e5ef923e315790ab5e8c4860bd7e81dacdaef31804dd216896fa99d95ab

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

c4860bddocbuild: More reliable way of trying to remove static_dir

comment:8 Changed 6 years ago by Ximin Luo

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.

comment:9 Changed 6 years ago by Ximin Luo

Status: needs_workneeds_review

comment:10 Changed 6 years ago by git

Commit: c4860bd7e81dacdaef31804dd216896fa99d95ab58c1e3f435cfe2d0c297efc71deae54661e900f5

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

58c1e3fdocbuild: More reliable way of trying to remove static_dir

comment:11 Changed 6 years ago by Ximin Luo

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

comment:12 Changed 6 years ago by git

Commit: 58c1e3f435cfe2d0c297efc71deae54661e900f505ee235086d919597e984cb77d0d242ffb855096

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

05ee235docbuild: More reliable way of trying to remove static_dir

comment:13 Changed 6 years ago by Ximin Luo

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

comment:14 Changed 5 years ago by Jeroen Demeyer

Reviewers: Jeroen Demeyer
Status: needs_reviewpositive_review

comment:15 Changed 5 years ago by Volker Braun

Branch: u/infinity0/docbuild__use_lexists_when_testing_whether_a_symlink_exists05ee235086d919597e984cb77d0d242ffb855096
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.