Skip to content

always skip symlinks in adjust_permissions#2644

Merged
akesandgren merged 9 commits intoeasybuilders:developfrom
boegel:lchmod
Nov 5, 2018
Merged

always skip symlinks in adjust_permissions#2644
akesandgren merged 9 commits intoeasybuilders:developfrom
boegel:lchmod

Conversation

@boegel
Copy link
Copy Markdown
Member

@boegel boegel commented Nov 1, 2018

This was pointed out by @akesandgren.

Using os.lchmod & os.lchown prevents EasyBuild from trying to change permissions on files it has no business in changing, in case a symlink points to a file outside of the build/install directory...

It probably also helps in avoiding problems like easybuilders/easybuild-easyblocks#1564

@boegel boegel added the change label Nov 1, 2018
@boegel boegel added this to the 3.8.0 milestone Nov 1, 2018
Comment thread easybuild/tools/filetools.py Outdated
@@ -1098,20 +1098,20 @@ def adjust_permissions(name, permissionBits, add=True, onlyfiles=False, onlydirs
perms = os.stat(path)[stat.ST_MODE]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs to be os.lstat to match the os.lchmod below

@akesandgren
Copy link
Copy Markdown
Contributor

Have you had time to test this besides the test in the framework/filetools ?

@akesandgren
Copy link
Copy Markdown
Contributor

Hmmm, hasattr(os, 'lchmod') returns False on my Ubuntu 16.04 Python 2.7.12 system.
In fact it seems to be a BSD only function.
And thus chmod should not be performed on symlinks, on Linux it is useless and wrong.

lchown, lstat however should still be used.

symlinks should just be ignored for chmod.

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Nov 3, 2018

@akesandgren Symbolic links are already skipped by default by adjust_permissions...

So should we be changing anything?

@easybuilders easybuilders deleted a comment from boegelbot Nov 3, 2018
@akesandgren
Copy link
Copy Markdown
Contributor

Would have to look at the code properly again.... I'll get back on it.

Comment thread easybuild/tools/filetools.py Outdated
@boegel boegel changed the title use os.lchmod rather than os.chmod in adjust_permissions function use os.lchown and os.lstat rather than os.chown & os.stat in adjust_permissions function Nov 3, 2018
@boegel boegel changed the title use os.lchown and os.lstat rather than os.chown & os.stat in adjust_permissions function always skip symlinks in adjust_permissions Nov 4, 2018
@boegel
Copy link
Copy Markdown
Member Author

boegel commented Nov 4, 2018

@akesandgren deprecation of the now useless skip_symlinks is documented in easybuilders/easybuild#466

Comment thread easybuild/tools/filetools.py Outdated
Comment thread test/framework/filetools.py Outdated
Copy link
Copy Markdown
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

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

LGTM

@akesandgren
Copy link
Copy Markdown
Contributor

should we test this on all easyblocks that uses it? Worst case is that some block uses it with "follow symlinks" to use that sideeffect. And would then need two invocations instead.

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Nov 5, 2018

@akesandgren There are no easyblocks that use skip_symlinks=False, and the current behavior matches the default behavior of skip_symlinks=True, so this can't affect easyblocks that use adjust_permissions?

In any case, this will be tested thoroughly during the next regression test for EasyBuild v3.8.0...

@akesandgren
Copy link
Copy Markdown
Contributor

Ah yes, monday morning...

@akesandgren akesandgren merged commit 5afe187 into easybuilders:develop Nov 5, 2018
@boegel boegel deleted the lchmod branch November 5, 2018 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants