Skip to content

Commit

Permalink
Fix local target permission handling.
Browse files Browse the repository at this point in the history
  • Loading branch information
riga committed Jan 16, 2025
1 parent feef358 commit e8829c6
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 10 deletions.
29 changes: 22 additions & 7 deletions law/target/file.py
Original file line number Diff line number Diff line change
Expand Up @@ -324,10 +324,15 @@ def chmod(self, perm, *, silent: bool = False, **kwargs) -> bool:
return self.fs.chmod(self.path, perm, silent=silent, **kwargs)

def makedirs(self, *args, **kwargs) -> None:
# overwrites luigi's makedirs method
parent = self.parent
if parent:
parent.touch(*args, **kwargs)

def _prepare_dir(self, **kwargs) -> None:
dir_target = self if isinstance(self, self.directory_class) else self.parent
dir_target.touch(**kwargs) # type: ignore[union-attr]

@abstractproperty
def fs(self) -> FileSystem:
...
Expand Down Expand Up @@ -478,8 +483,11 @@ def copy_to(
dir_perm: int | None = None,
**kwargs,
) -> str:
if isinstance(dst, FileSystemTarget):
dst._prepare_dir(perm=dir_perm, **kwargs)

# TODO: complain when dst not local? forward to copy_from request depending on protocol?
return self.fs.copy(self.path, get_path(dst), perm=perm, dir_perm=dir_perm, **kwargs)
return self.fs.copy(self.path, get_path(dst), perm=perm, **kwargs)

def copy_from(
self,
Expand All @@ -489,12 +497,14 @@ def copy_from(
dir_perm: int | None = None,
**kwargs,
) -> str:
self._prepare_dir(perm=dir_perm, **kwargs)

if isinstance(src, FileSystemFileTarget):
return src.copy_to(self.abspath, perm=perm, dir_perm=dir_perm, **kwargs)
return src.copy_to(self.abspath, perm=perm or self.fs.default_file_perm, **kwargs)

# when src is a plain string, let the fs handle it
# TODO: complain when src not local? forward to copy_to request depending on protocol?
return self.fs.copy(get_path(src), self.path, perm=perm, dir_perm=dir_perm, **kwargs)
# when src is a plain string, let the fs handle it
return self.fs.copy(get_path(src), self.path, perm=perm, **kwargs)

def move_to(
self,
Expand All @@ -504,8 +514,11 @@ def move_to(
dir_perm: int | None = None,
**kwargs,
) -> str:
if isinstance(dst, FileSystemTarget):
dst._prepare_dir(perm=dir_perm, **kwargs)

# TODO: complain when dst not local? forward to copy_from request depending on protocol?
return self.fs.move(self.path, get_path(dst), perm=perm, dir_perm=dir_perm, **kwargs)
return self.fs.move(self.path, get_path(dst), perm=perm, **kwargs)

def move_from(
self,
Expand All @@ -515,12 +528,14 @@ def move_from(
dir_perm: int | None = None,
**kwargs,
) -> str:
self._prepare_dir(perm=dir_perm, **kwargs)

if isinstance(src, FileSystemFileTarget):
return src.move_to(self.abspath, perm=perm, dir_perm=dir_perm, **kwargs)
return src.move_to(self.abspath, perm=perm or self.fs.default_file_perm, **kwargs)

# when src is a plain string, let the fs handle it
# TODO: complain when src not local? forward to copy_to request depending on protocol?
return self.fs.move(get_path(src), self.path, perm=perm, dir_perm=dir_perm, **kwargs)
return self.fs.move(get_path(src), self.path, perm=perm, **kwargs)


class FileSystemDirectoryTarget(FileSystemTarget):
Expand Down
6 changes: 3 additions & 3 deletions law/target/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ def mkdir( # type: ignore[override]

# the mode passed to os.mkdir or os.makedirs is ignored on some systems, so the strategy
# here is to disable the process' current umask, create the directories and use chmod again
orig = os.umask(0) if perm is not None else None
orig = os.umask(0o0770 - perm) if perm is not None else None
func = os.makedirs if recursive else os.mkdir
try:
try:
Expand Down Expand Up @@ -578,7 +578,7 @@ def localize(

# move back again
if tmp.exists():
tmp.copy_to_local(self, perm=perm, dir_perm=dir_perm)
self.copy_from_local(tmp, perm=perm, dir_perm=dir_perm)
else:
logger.warning(
"cannot move non-existing localized target to actual representation "
Expand Down Expand Up @@ -663,7 +663,7 @@ def localize(
# TODO: keep track of changed contents in "a" mode and copy only those?
if tmp.exists():
self.remove()
tmp.copy_to_local(self, perm=perm, dir_perm=dir_perm)
self.copy_from_local(tmp, perm=perm, dir_perm=dir_perm)
else:
logger.warning(
"cannot move non-existing localized target to actual representation "
Expand Down

0 comments on commit e8829c6

Please sign in to comment.