Revert "fix racy sbang (#33549)" (#33778)

This reverts commit 4d28a64661.

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"):
+def write_tmp_and_move(filename):
-    """Write to a temporary file in the same directory, then move into place."""
+    """Write to a temporary file, then move into place."""
-    # Rely on NamedTemporaryFile to give a unique file without races
+    dirname = os.path.dirname(filename)
-    # in the directory of the target file.
+    basename = os.path.basename(filename)
-    file = tempfile.NamedTemporaryFile(
+    tmp = os.path.join(dirname, ".%s.tmp" % basename)
-        prefix="." + os.path.basename(path),
+    with open(tmp, "w") as f:
-        suffix=".tmp",
+        yield f
-        dir=os.path.dirname(path),
+    shutil.move(tmp, filename)
-        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
 
 
 @contextmanager

lib/spack/spack/config.py

@@ -45,7 +45,7 @@
 
 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 @@ def _write_section(self, section):
             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))

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")
+    # make $install_tree/bin
-    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.
     sbang_bin_dir = os.path.dirname(sbang_path)
-    if not os.path.isdir(sbang_bin_dir):
     fs.mkdirp(sbang_bin_dir)
 
-    # Set group and ownership like we do on package directories
+    # get permissions for bin dir from configuration files
-    if group_id:
+    group_name = spack.package_prefs.get_package_group(spack.spec.Spec("all"))
-        os.chown(sbang_bin_dir, os.stat(sbang_bin_dir).st_uid, group_id)
+    config_mode = spack.package_prefs.get_package_dir_permissions(spack.spec.Spec("all"))
-        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.
+    # set group on sbang_bin_dir if not already set (only if set in configuration)
-    try:
+    # TODO: after we drop python2 support, use shutil.chown to avoid gid lookups that
-        already_installed = filecmp.cmp(spack.paths.sbang_script, sbang_path)
+    # can fail for remote groups
-    except (IOError, OSError):
+    if group_name and os.stat(sbang_bin_dir).st_gid != grp.getgrnam(group_name).gr_gid:
-        already_installed = False
+        os.chown(sbang_bin_dir, os.stat(sbang_bin_dir).st_uid, grp.getgrnam(group_name).gr_gid)
 
-    if not already_installed:
+    # copy over the fresh copy of `sbang`
-        with fs.write_tmp_and_move(sbang_path) as f:
+    sbang_tmp_path = os.path.join(
-            shutil.copy(spack.paths.sbang_script, f.name)
+        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)
+    # set permissions on `sbang` (including group if set in configuration)
-    os.chmod(sbang_path, config_mode)
+    os.chmod(sbang_tmp_path, config_mode)
-    if group_id:
+    if group_name:
-        os.chown(sbang_path, os.stat(sbang_path).st_uid, group_id)
+        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):