diff --git a/fsspec/implementations/local.py b/fsspec/implementations/local.py index 417447e5b..e9da2f083 100644 --- a/fsspec/implementations/local.py +++ b/fsspec/implementations/local.py @@ -399,7 +399,10 @@ def _open(self): self.f = compress(self.f, mode=self.mode) else: # TODO: check if path is writable? - i, name = tempfile.mkstemp() + i, name = tempfile.mkstemp( + dir=os.path.dirname(self.path) or None, + prefix=os.path.basename(self.path) + "-", + ) os.close(i) # we want normal open and normal buffered file self.temp = name self.f = open(name, mode=self.mode) @@ -438,22 +441,12 @@ def __getstate__(self): def commit(self): if self.autocommit: raise RuntimeError("Can only commit if not already set to autocommit") + shutil.move(self.temp, self.path) try: - shutil.move(self.temp, self.path) - except PermissionError as e: - # shutil.move raises PermissionError if os.rename - # and the default copy2 fallback with shutil.copystats fail. - # The file should be there nonetheless, but without copied permissions. - # If it doesn't exist, there was no permission to create the file. - if not os.path.exists(self.path): - raise e - else: - # If PermissionError is not raised, permissions can be set. - try: - mask = 0o666 - os.chmod(self.path, mask & ~get_umask(mask)) - except RuntimeError: - pass + mask = 0o666 + os.chmod(self.path, mask & ~get_umask(mask)) + except (RuntimeError, PermissionError): + pass def discard(self): if self.autocommit: diff --git a/fsspec/implementations/tests/test_local.py b/fsspec/implementations/tests/test_local.py index c3caf1ddb..53553ec16 100644 --- a/fsspec/implementations/tests/test_local.py +++ b/fsspec/implementations/tests/test_local.py @@ -1,5 +1,4 @@ import bz2 -import errno import gzip import os import os.path @@ -563,24 +562,22 @@ def test_multiple_filesystems_use_umask_cache(tmpdir): assert get_umask.cache_info().hits == 1 -def test_transaction_cross_device_but_mock_temp_dir_on_wrong_device(tmpdir): - # If the temporary file for a transaction is not on the correct device, - # os.rename in shutil.move will raise EXDEV and lookup('chmod') will raise - # a PermissionError. +def test_transaction_temp_file_in_target_dir(tmpdir): + # Temporary file should be created in the same directory as the target + # to avoid cross-device link errors when committing. fs = LocalFileSystem() - with ( - patch( - "os.rename", - side_effect=OSError(errno.EXDEV, "Invalid cross-device link"), - ), - patch( - "os.chmod", - side_effect=PermissionError("Operation not permitted"), - ), - ): - with fs.transaction, fs.open(tmpdir + "/afile", "wb") as f: + target = str(tmpdir) + "/subdir/file.txt" + os.makedirs(os.path.dirname(target)) + + with fs.transaction: + with fs.open(target, "wb") as f: + assert os.path.dirname(f.temp) == os.path.dirname(target) + assert os.path.basename(f.temp).startswith("file.txt-") f.write(b"data") + with fs.open(target, "rb") as f: + assert f.read() == b"data" + def test_make_path_posix(): cwd = os.getcwd()