Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Skip symlinked files when fixing permissions #1439

Merged
merged 5 commits into from Oct 28, 2015
Merged

Skip symlinked files when fixing permissions #1439

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
06abf77
Skip symlinked files when fixing permissions
geimer Oct 27, 2015
31b2a54
Fix @boegel's remarks
geimer Oct 27, 2015
61813d7
add test for adjust_permissions
boegel Oct 27, 2015
95c93cd
Merge pull request #3 from boegel/symlink_permissions
geimer Oct 27, 2015
8a16a77
Avoid negations in logic to make code more readable
geimer Oct 27, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 11 additions & 3 deletions easybuild/tools/filetools.py
View file
Open in desktop
Expand Up @@ -686,7 +686,7 @@ def convert_name(name, upper=False):


def adjust_permissions(name, permissionBits, add=True, onlyfiles=False, onlydirs=False, recursive=True,
group_id=None, relative=True, ignore_errors=False):
group_id=None, relative=True, ignore_errors=False, skip_symlinks=True):
"""
Add or remove (if add is False) permissionBits from all files (if onlydirs is False)
and directories (if onlyfiles is False) in path
Expand All @@ -700,9 +700,17 @@ def adjust_permissions(name, permissionBits, add=True, onlyfiles=False, onlydirs
for root, dirs, files in os.walk(name):
paths = []
if not onlydirs:
paths += files
if not skip_symlinks:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, maybe it's better to avoid 'double negation' (sort of):

if skip_symlinks:
   ...
else:
   # ...
   paths.extend(dirs)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes some sense. But after switching this, we have yet another negation inside the for loop -- which has now moved up. Though I would suggest to keep it that way to have the part where something has to be done first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it, it may actually make sense to switch that too (If we are skipping symlinks and if it is a link, issue a debug message; otherwise do it)

paths.extend(files)
else:
for path in files:
if not os.path.islink(os.path.join(root, path)):
paths.append(path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add an else: here with some logging?

else:
    _log.debug("Not adjusting permissions for symlink %s", path)

else:
_log.debug("Not adjusting permissions for symlink %s", path)
if not onlyfiles:
paths += dirs
# os.walk skips symlinked dirs by default, i.e., no special handling needed here
paths.extend(dirs)

for path in paths:
allpaths.append(os.path.join(root, path))
Expand Down
76 changes: 76 additions & 0 deletions test/framework/filetools.py
View file
Open in desktop
Expand Up @@ -453,6 +453,82 @@ def test_weld_paths(self):
self.assertEqual(ft.weld_paths('/foo', '/foo/bar/baz'), '/foo/bar/baz/')


def test_adjust_permissions(self):
"""Test adjust_permissions"""
# set umask hard to run test reliably
orig_umask = os.umask(0022)

# prep files/dirs/(broken) symlinks is test dir

# file: rw-r--r--
ft.write_file(os.path.join(self.test_prefix, 'foo'), 'foo')
foo_perms = os.stat(os.path.join(self.test_prefix, 'foo'))[stat.ST_MODE]
for bit in [stat.S_IRUSR, stat.S_IWUSR, stat.S_IRGRP, stat.S_IROTH]:
self.assertTrue(foo_perms & bit)
for bit in [stat.S_IXUSR, stat.S_IWGRP, stat.S_IXGRP, stat.S_IWOTH, stat.S_IXOTH]:
self.assertFalse(foo_perms & bit)

# dir: rwxr-xr-x
ft.mkdir(os.path.join(self.test_prefix, 'bar'))
bar_perms = os.stat(os.path.join(self.test_prefix, 'bar'))[stat.ST_MODE]
for bit in [stat.S_IRUSR, stat.S_IWUSR, stat.S_IXUSR, stat.S_IRGRP, stat.S_IXGRP, stat.S_IROTH, stat.S_IXOTH]:
self.assertTrue(bar_perms & bit)
for bit in [stat.S_IWGRP, stat.S_IWOTH]:
self.assertFalse(bar_perms & bit)

# file in dir: rw-r--r--
foobar_path = os.path.join(self.test_prefix, 'bar', 'foobar')
ft.write_file(foobar_path, 'foobar')
foobar_perms = os.stat(foobar_path)[stat.ST_MODE]
for bit in [stat.S_IRUSR, stat.S_IWUSR, stat.S_IRGRP, stat.S_IROTH]:
self.assertTrue(foobar_perms & bit)
for bit in [stat.S_IXUSR, stat.S_IWGRP, stat.S_IXGRP, stat.S_IWOTH, stat.S_IXOTH]:
self.assertFalse(foobar_perms & bit)

# include symlink
os.symlink(foobar_path, os.path.join(self.test_prefix, 'foobar_symlink'))

# include broken symlink (symlinks are skipped, so this shouldn't cause problems)
tmpfile = os.path.join(self.test_prefix, 'thiswontbetherelong')
ft.write_file(tmpfile, 'poof!')
os.symlink(tmpfile, os.path.join(self.test_prefix, 'broken_symlink'))
os.remove(tmpfile)

# test default behaviour:
# recursive, add permissions, relative to existing permissions, both files and dirs, skip symlinks
# add user execution, group write permissions
ft.adjust_permissions(self.test_prefix, stat.S_IXUSR|stat.S_IWGRP)

# foo file: rwxrw-r--
foo_perms = os.stat(os.path.join(self.test_prefix, 'foo'))[stat.ST_MODE]
for bit in [stat.S_IRUSR, stat.S_IWUSR, stat.S_IXUSR, stat.S_IRGRP, stat.S_IWGRP, stat.S_IROTH]:
self.assertTrue(foo_perms & bit)
for bit in [stat.S_IXGRP, stat.S_IWOTH, stat.S_IXOTH]:
self.assertFalse(foo_perms & bit)

# bar dir: rwxrwxr-x
bar_perms = os.stat(os.path.join(self.test_prefix, 'bar'))[stat.ST_MODE]
for bit in [stat.S_IRUSR, stat.S_IWUSR, stat.S_IXUSR, stat.S_IRGRP, stat.S_IWGRP, stat.S_IXGRP,
stat.S_IROTH, stat.S_IXOTH]:
self.assertTrue(bar_perms & bit)
self.assertFalse(bar_perms & stat.S_IWOTH)

# foo/foobar file: rwxrw-r--
for path in [os.path.join(self.test_prefix, 'bar', 'foobar'), os.path.join(self.test_prefix, 'foobar_symlink')]:
perms = os.stat(path)[stat.ST_MODE]
for bit in [stat.S_IRUSR, stat.S_IWUSR, stat.S_IXUSR, stat.S_IRGRP, stat.S_IWGRP, stat.S_IROTH]:
self.assertTrue(perms & bit)
for bit in [stat.S_IXGRP, stat.S_IWOTH, stat.S_IXOTH]:
self.assertFalse(perms & bit)

# broken symlinks are trouble if symlinks are not skipped
self.assertErrorRegex(EasyBuildError, "No such file or directory", ft.adjust_permissions, self.test_prefix,
stat.S_IXUSR, skip_symlinks=False)

# restore original umask
os.umask(orig_umask)


def suite():
""" returns all the testcases in this module """
return TestLoader().loadTestsFromTestCase(FileToolsTest)
Expand Down