From a7de2fa380dd397a0a1c3e23629074adf06d2604 Mon Sep 17 00:00:00 2001 From: Betsy McPhail Date: Tue, 26 Jan 2021 14:30:32 -0500 Subject: Create rename utility function os.rename() fails on Windows if file already exists. Create getuid utility function (#21736) On Windows, replace os.getuid with ctypes.windll.shell32.IsUserAnAdmin(). Tests: Use getuid util function Co-authored-by: lou.lawrence@kitware.com Co-authored-by: Betsy McPhail --- lib/spack/llnl/util/filesystem.py | 22 +++++++++++++++++++++- lib/spack/spack/config.py | 4 ++-- lib/spack/spack/database.py | 3 ++- lib/spack/spack/directory_layout.py | 2 +- lib/spack/spack/fetch_strategy.py | 5 +++-- lib/spack/spack/test/install.py | 8 ++++---- lib/spack/spack/test/llnl/util/lock.py | 4 ++-- lib/spack/spack/test/stage.py | 6 +++--- lib/spack/spack/test/util/spack_lock_wrapper.py | 6 +++--- lib/spack/spack/util/file_cache.py | 4 ++-- lib/spack/spack/util/web.py | 4 ++-- 11 files changed, 45 insertions(+), 23 deletions(-) (limited to 'lib') diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index be24f9e53f..9ce56d91fa 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/lib/spack/llnl/util/filesystem.py @@ -6,6 +6,7 @@ import collections import errno import glob import grp +import ctypes import hashlib import itertools import numbers @@ -44,6 +45,7 @@ __all__ = [ 'fix_darwin_install_name', 'force_remove', 'force_symlink', + 'getuid', 'chgrp', 'chmod_x', 'copy', @@ -60,6 +62,7 @@ __all__ = [ 'remove_directory_contents', 'remove_if_dead_link', 'remove_linked_tree', + 'rename', 'set_executable', 'set_install_permissions', 'touch', @@ -71,6 +74,23 @@ __all__ = [ ] +def getuid(): + if _platform == "win32": + if ctypes.windll.shell32.IsUserAnAdmin() == 0: + return 1 + return 0 + else: + return os.getuid() + + +def rename(src, dst): + # On Windows, os.rename will fail if the destination file already exists + if is_windows: + if os.path.exists(dst): + os.remove(dst) + os.rename(src, dst) + + def path_contains_subdirectory(path, root): norm_root = os.path.abspath(root).rstrip(os.path.sep) + os.path.sep norm_path = os.path.abspath(path).rstrip(os.path.sep) + os.path.sep @@ -293,7 +313,7 @@ def group_ids(uid=None): (list of int): gids of groups the user is a member of """ if uid is None: - uid = os.getuid() + uid = getuid() user = pwd.getpwuid(uid).pw_name return [g.gr_gid for g in grp.getgrall() if user in g.gr_mem] diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py index f9e3f2125a..4048c3b192 100644 --- a/lib/spack/spack/config.py +++ b/lib/spack/spack/config.py @@ -45,7 +45,7 @@ from six import iteritems import llnl.util.lang import llnl.util.tty as tty -from llnl.util.filesystem import mkdirp +from llnl.util.filesystem import mkdirp, rename import spack.compilers import spack.paths @@ -292,7 +292,7 @@ class SingleFileScope(ConfigScope): with open(tmp, 'w') as f: syaml.dump_config(data_to_write, stream=f, default_flow_style=False) - os.rename(tmp, self.path) + rename(tmp, self.path) except (yaml.YAMLError, IOError) as e: raise ConfigFileError( "Error writing to config file: '%s'" % str(e)) diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py index 3c264c5b0e..7fe1b8fcef 100644 --- a/lib/spack/spack/database.py +++ b/lib/spack/spack/database.py @@ -1001,7 +1001,8 @@ class Database(object): try: with open(temp_file, 'w') as f: self._write_to_file(f) - os.rename(temp_file, self._index_path) + fs.rename(temp_file, self._index_path) + if _use_uuid: with open(self._verifier_path, 'w') as f: new_verifier = str(uuid.uuid4()) diff --git a/lib/spack/spack/directory_layout.py b/lib/spack/spack/directory_layout.py index 1351863f68..bfe7077547 100644 --- a/lib/spack/spack/directory_layout.py +++ b/lib/spack/spack/directory_layout.py @@ -565,7 +565,7 @@ class YamlViewExtensionsLayout(ExtensionsLayout): }, tmp, default_flow_style=False, encoding='utf-8') # Atomic update by moving tmpfile on top of old one. - os.rename(tmp.name, path) + fs.rename(tmp.name, path) class DirectoryLayoutError(SpackError): diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index bb7d446240..1204932dda 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -38,6 +38,7 @@ import llnl.util.tty as tty from llnl.util.filesystem import ( get_single_file, mkdirp, + rename, temp_cwd, temp_rename, working_dir, @@ -324,7 +325,7 @@ class URLFetchStrategy(FetchStrategy): try: partial_file, save_file = self._fetch_from_url(url) if save_file and (partial_file is not None): - os.rename(partial_file, save_file) + rename(partial_file, save_file) break except FailedDownloadError as e: errors.append(str(e)) @@ -1399,7 +1400,7 @@ class S3FetchStrategy(URLFetchStrategy): warn_content_type_mismatch(self.archive_file or "the archive") if self.stage.save_filename: - os.rename( + rename( os.path.join(self.stage.path, basename), self.stage.save_filename) diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py index a100d23aab..85865be0a3 100644 --- a/lib/spack/spack/test/install.py +++ b/lib/spack/spack/test/install.py @@ -456,12 +456,12 @@ def test_pkg_build_paths(install_mockery): # Now check the newer log filename last_log = 'spack-build.txt' - os.rename(older_log, last_log) + fs.rename(older_log, last_log) assert spec.package.log_path.endswith(last_log) # Check the old environment file last_env = 'spack-build.env' - os.rename(last_log, last_env) + fs.rename(last_log, last_env) assert spec.package.env_path.endswith(last_env) # Cleanup @@ -492,12 +492,12 @@ def test_pkg_install_paths(install_mockery): # Now check the newer install log filename last_log = 'build.txt' - os.rename(older_log, last_log) + fs.rename(older_log, last_log) assert spec.package.install_log_path.endswith(last_log) # Check the old install environment file last_env = 'build.env' - os.rename(last_log, last_env) + fs.rename(last_log, last_env) assert spec.package.install_env_path.endswith(last_env) # Cleanup diff --git a/lib/spack/spack/test/llnl/util/lock.py b/lib/spack/spack/test/llnl/util/lock.py index 4bb23540f6..a2c54e3968 100644 --- a/lib/spack/spack/test/llnl/util/lock.py +++ b/lib/spack/spack/test/llnl/util/lock.py @@ -59,7 +59,7 @@ import pytest import llnl.util.lock as lk import llnl.util.multiproc as mp -from llnl.util.filesystem import touch +from llnl.util.filesystem import getuid, touch # # This test can be run with MPI. MPI is "enabled" if we can import @@ -580,7 +580,7 @@ def test_write_lock_timeout_with_multiple_readers_3_2_ranges(lock_path): TimeoutWrite(lock_path, 5, 1)) -@pytest.mark.skipif(os.getuid() == 0, reason='user is root') +@pytest.mark.skipif(getuid() == 0, reason='user is root') def test_read_lock_on_read_only_lockfile(lock_dir, lock_path): """read-only directory, read-only lockfile.""" touch(lock_path) diff --git a/lib/spack/spack/test/stage.py b/lib/spack/spack/test/stage.py index 9d24bfc266..97462ac4f1 100644 --- a/lib/spack/spack/test/stage.py +++ b/lib/spack/spack/test/stage.py @@ -13,7 +13,7 @@ import stat import pytest -from llnl.util.filesystem import mkdirp, partition_path, touch, working_dir +from llnl.util.filesystem import getuid, mkdirp, partition_path, touch, working_dir import spack.paths import spack.stage @@ -357,7 +357,7 @@ def check_stage_dir_perms(prefix, path): user = getpass.getuser() prefix_status = os.stat(prefix) - uid = os.getuid() + uid = getuid() # Obtain lists of ancestor and descendant paths of the $user node, if any. # @@ -753,7 +753,7 @@ class TestStage(object): # The following check depends on the patched os.stat as a poor # substitute for confirming the generated warnings. - assert os.stat(user_path).st_uid != os.getuid() + assert os.stat(user_path).st_uid != getuid() def test_resolve_paths(self): """Test _resolve_paths.""" diff --git a/lib/spack/spack/test/util/spack_lock_wrapper.py b/lib/spack/spack/test/util/spack_lock_wrapper.py index 005154f3ce..7dd0212279 100644 --- a/lib/spack/spack/test/util/spack_lock_wrapper.py +++ b/lib/spack/spack/test/util/spack_lock_wrapper.py @@ -8,7 +8,7 @@ import os import pytest -from llnl.util.filesystem import group_ids +from llnl.util.filesystem import getuid, group_ids import spack.config import spack.util.lock as lk @@ -42,7 +42,7 @@ def test_disable_locking(tmpdir): @pytest.mark.nomockstage def test_lock_checks_user(tmpdir): """Ensure lock checks work with a self-owned, self-group repo.""" - uid = os.getuid() + uid = getuid() if uid not in group_ids(): pytest.skip("user has no group with gid == uid") @@ -76,7 +76,7 @@ def test_lock_checks_user(tmpdir): @pytest.mark.nomockstage def test_lock_checks_group(tmpdir): """Ensure lock checks work with a self-owned, non-self-group repo.""" - uid = os.getuid() + uid = getuid() gid = next((g for g in group_ids() if g != uid), None) if not gid: pytest.skip("user has no group with gid != uid") diff --git a/lib/spack/spack/util/file_cache.py b/lib/spack/spack/util/file_cache.py index 3ef46d3b6e..5f68911241 100644 --- a/lib/spack/spack/util/file_cache.py +++ b/lib/spack/spack/util/file_cache.py @@ -6,7 +6,7 @@ import os import shutil -from llnl.util.filesystem import mkdirp +from llnl.util.filesystem import mkdirp, rename from spack.error import SpackError from spack.util.lock import Lock, ReadTransaction, WriteTransaction @@ -145,7 +145,7 @@ class FileCache(object): shutil.rmtree(cm.tmp_filename, True) else: - os.rename(cm.tmp_filename, cm.orig_filename) + rename(cm.tmp_filename, cm.orig_filename) return WriteTransaction( self._get_lock(key), acquire=WriteContextManager) diff --git a/lib/spack/spack/util/web.py b/lib/spack/spack/util/web.py index 0f148a88f5..7786bed255 100644 --- a/lib/spack/spack/util/web.py +++ b/lib/spack/spack/util/web.py @@ -22,7 +22,7 @@ from six.moves.urllib.request import Request, urlopen import llnl.util.lang import llnl.util.tty as tty -from llnl.util.filesystem import mkdirp +from llnl.util.filesystem import mkdirp, rename import spack.config import spack.error @@ -173,7 +173,7 @@ def push_to_url( shutil.copy(local_file_path, remote_file_path) else: try: - os.rename(local_file_path, remote_file_path) + rename(local_file_path, remote_file_path) except OSError as e: if e.errno == errno.EXDEV: # NOTE(opadron): The above move failed because it crosses -- cgit v1.2.3-60-g2f50