Skip to content
This repository has been archived by the owner on Dec 10, 2020. It is now read-only.

Commit

Permalink
Prevent cleanup error at end of pants test with --test-junit-html-rep…
Browse files Browse the repository at this point in the history
…ort option, update safe_rmtree to be symlink aware

If you run ./pants test in our repo twice on JVM targets without cleaning up in between you'll see this error:

```
  ...
  File ".../pants/backend/jvm/tasks/junit_run.py", line 667, in _isolation
    shutil.rmtree(path)
  File ".../Versions/2.7/lib/python2.7/shutil.py", line 232, in rmtree
    onerror(os.path.islink, path, sys.exc_info())
  File ".../Versions/2.7/lib/python2.7/shutil.py", line 230, in rmtree
    raise OSError("Cannot call rmtree on a symbolic link")
```

To repro in the pants repo, run:

```
./pants clean-all
./pants test tests/java/org/pantsbuild/args4j --test-junit-html-report
./pants test tests/java/org/pantsbuild/args4j --test-junit-html-report
```

Fails on the second time running the test goal.

During debugging, I found the path its trying to remove is `.pants.d/test/junit/reports`

```
$ ls -l .pants.d/test/junit/reports
.pants.d/test/junit/reports -> /Users/zundel/Src/pants-release/.pants.d/test/junit/_runs/68509c3d71dcf475c816117b0b4130f9547cab8b/reports
```

This link is safe to delete at this point, it gets recreated later.

I fixed this by substituting `safe_rmtree` for `shutil.rmtree` and adding symlink awareness into that library call

Testing Done:
CI running at https://travis-ci.org/pantsbuild/pants/builds/168062004

Tested locally and the symlink is recreated and the HTML report is still available.

Bugs closed: 3974

Reviewed at https://rbcommons.com/s/twitter/r/4319/
  • Loading branch information
ericzundel authored and stuhood committed Oct 18, 2016
1 parent cf3864e commit f1988e7
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 4 deletions.
5 changes: 2 additions & 3 deletions src/python/pants/backend/jvm/tasks/junit_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import os
import re
import shutil
import sys
from abc import abstractmethod
from collections import defaultdict
Expand Down Expand Up @@ -39,7 +38,7 @@
from pants.util import desktop
from pants.util.argutil import ensure_arg, remove_arg
from pants.util.contextutil import environment_as
from pants.util.dirutil import safe_mkdir
from pants.util.dirutil import safe_mkdir, safe_rmtree
from pants.util.memo import memoized_method
from pants.util.meta import AbstractClass
from pants.util.objects import datatype
Expand Down Expand Up @@ -664,7 +663,7 @@ def do_report(exc=None):
path = os.path.join(self.workdir, name)
if name not in (run_dir, lock_file):
if os.path.isdir(path):
shutil.rmtree(path)
safe_rmtree(path)
else:
os.unlink(path)

Expand Down
8 changes: 7 additions & 1 deletion src/python/pants/util/dirutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,15 @@ def register_rmtree(directory, cleaner=_mkdtemp_atexit_cleaner):
def safe_rmtree(directory):
"""Delete a directory if it's present. If it's not present, no-op.
Note that if the directory argument is a symlink, only the symlink will
be deleted.
:API: public
"""
shutil.rmtree(directory, ignore_errors=True)
if os.path.islink(directory):
safe_delete(directory)
else:
shutil.rmtree(directory, ignore_errors=True)


def safe_open(filename, *args, **kwargs):
Expand Down
12 changes: 12 additions & 0 deletions tests/python/pants_test/util/test_dirutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,18 @@ def test_safe_rm_oldest_items_in_dir_noop(self):
touch(os.path.join(td, 'file1'))
self.assertEqual(len(os.listdir(td)), 1)

def test_safe_rmtree_link(self):
with temporary_dir() as td:
real = os.path.join(td, 'real')
link = os.path.join(td, 'link')
os.mkdir(real)
os.symlink(real, link)
self.assertTrue(os.path.exists(real))
self.assertTrue(os.path.exists(link))
safe_rmtree(link);
self.assertTrue(os.path.exists(real))
self.assertFalse(os.path.exists(link))


class AbsoluteSymlinkTest(unittest.TestCase):
def setUp(self):
Expand Down

0 comments on commit f1988e7

Please sign in to comment.