From d1d668a9d57e984a225eeef153d93cb315e03dfb Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Wed, 9 Nov 2022 10:31:27 +0100 Subject: Revert "fix racy sbang (#33549)" (#33778) This reverts commit 4d28a6466188ea9fe3b55a4c3d7690dd66e0dc8f. --- lib/spack/llnl/util/filesystem.py | 45 ++++++------------------------- lib/spack/spack/config.py | 6 +++-- lib/spack/spack/hooks/sbang.py | 57 +++++++++++++++++++++------------------ 3 files changed, 43 insertions(+), 65 deletions(-) (limited to 'lib') diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index d122280e99..aece52f843 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/lib/spack/llnl/util/filesystem.py @@ -1000,45 +1000,16 @@ def hash_directory(directory, ignore=[]): return md5_hash.hexdigest() -def _try_unlink(path): - try: - os.unlink(path) - except (IOError, OSError): - # But if that fails, that's OK. - pass - - @contextmanager @system_path_filter -def write_tmp_and_move(path, mode="w"): - """Write to a temporary file in the same directory, then move into place.""" - # Rely on NamedTemporaryFile to give a unique file without races - # in the directory of the target file. - file = tempfile.NamedTemporaryFile( - prefix="." + os.path.basename(path), - suffix=".tmp", - dir=os.path.dirname(path), - mode=mode, - delete=False, # we delete it ourselves - ) - tmp_path = file.name - - try: - yield file - except BaseException: - # On any failure, try to remove the temporary file. - _try_unlink(tmp_path) - raise - finally: - # Always close the file decriptor - file.close() - - # Atomically move into existence. - try: - os.rename(tmp_path, path) - except (IOError, OSError): - _try_unlink(tmp_path) - raise +def write_tmp_and_move(filename): + """Write to a temporary file, then move into place.""" + dirname = os.path.dirname(filename) + basename = os.path.basename(filename) + tmp = os.path.join(dirname, ".%s.tmp" % basename) + with open(tmp, "w") as f: + yield f + shutil.move(tmp, filename) @contextmanager diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py index f35310986e..e9a3b1d956 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, write_tmp_and_move +from llnl.util.filesystem import mkdirp, rename import spack.compilers import spack.paths @@ -287,8 +287,10 @@ class SingleFileScope(ConfigScope): parent = os.path.dirname(self.path) mkdirp(parent) - with write_tmp_and_move(self.path) as f: + tmp = os.path.join(parent, ".%s.tmp" % os.path.basename(self.path)) + with open(tmp, "w") as f: syaml.dump_config(data_to_write, stream=f, default_flow_style=False) + 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/hooks/sbang.py b/lib/spack/spack/hooks/sbang.py index e22c2929c2..5d2de2b865 100644 --- a/lib/spack/spack/hooks/sbang.py +++ b/lib/spack/spack/hooks/sbang.py @@ -186,39 +186,44 @@ def install_sbang(): ``sbang`` here ensures that users can access the script and that ``sbang`` itself is in a short path. """ + # copy in a new version of sbang if it differs from what's in spack sbang_path = sbang_install_path() + if os.path.exists(sbang_path) and filecmp.cmp(spack.paths.sbang_script, sbang_path): + return - all = spack.spec.Spec("all") - group_name = spack.package_prefs.get_package_group(all) - config_mode = spack.package_prefs.get_package_dir_permissions(all) - group_id = grp.getgrnam(group_name).gr_gid if group_name else None - - # First setup the bin dir correctly. + # make $install_tree/bin sbang_bin_dir = os.path.dirname(sbang_path) - if not os.path.isdir(sbang_bin_dir): - fs.mkdirp(sbang_bin_dir) + fs.mkdirp(sbang_bin_dir) + + # get permissions for bin dir from configuration files + group_name = spack.package_prefs.get_package_group(spack.spec.Spec("all")) + config_mode = spack.package_prefs.get_package_dir_permissions(spack.spec.Spec("all")) - # Set group and ownership like we do on package directories - if group_id: - os.chown(sbang_bin_dir, os.stat(sbang_bin_dir).st_uid, group_id) - os.chmod(sbang_bin_dir, config_mode) + if group_name: + os.chmod(sbang_bin_dir, config_mode) # Use package directory permissions else: fs.set_install_permissions(sbang_bin_dir) - # Then check if we need to install sbang itself. - try: - already_installed = filecmp.cmp(spack.paths.sbang_script, sbang_path) - except (IOError, OSError): - already_installed = False - - if not already_installed: - with fs.write_tmp_and_move(sbang_path) as f: - shutil.copy(spack.paths.sbang_script, f.name) - - # Set permissions on `sbang` (including group if set in configuration) - os.chmod(sbang_path, config_mode) - if group_id: - os.chown(sbang_path, os.stat(sbang_path).st_uid, group_id) + # set group on sbang_bin_dir if not already set (only if set in configuration) + # TODO: after we drop python2 support, use shutil.chown to avoid gid lookups that + # can fail for remote groups + if group_name and os.stat(sbang_bin_dir).st_gid != grp.getgrnam(group_name).gr_gid: + os.chown(sbang_bin_dir, os.stat(sbang_bin_dir).st_uid, grp.getgrnam(group_name).gr_gid) + + # copy over the fresh copy of `sbang` + sbang_tmp_path = os.path.join( + os.path.dirname(sbang_path), + ".%s.tmp" % os.path.basename(sbang_path), + ) + shutil.copy(spack.paths.sbang_script, sbang_tmp_path) + + # set permissions on `sbang` (including group if set in configuration) + os.chmod(sbang_tmp_path, config_mode) + if group_name: + os.chown(sbang_tmp_path, os.stat(sbang_tmp_path).st_uid, grp.getgrnam(group_name).gr_gid) + + # Finally, move the new `sbang` into place atomically + os.rename(sbang_tmp_path, sbang_path) def post_install(spec): -- cgit v1.2.3-60-g2f50