Fix race condition in FileSystemPackageRepository directory creation, modernize usage of os.makedirs (#1913)

* Fix race condition in `FileSystemPackageRepository` directory creation

Signed-off-by: Nathan Rusch <[email protected]>

* Replace `safe_makedirs` helper with `os.makedirs(..., exist_ok=True)`

Signed-off-by: Nathan Rusch <[email protected]>

* Update remaining uses of `os.makedirs` to include `exist_ok=True`

Signed-off-by: Nathan Rusch <[email protected]>

---------

Signed-off-by: Nathan Rusch <[email protected]>
Co-authored-by: Jean-Christophe Morin <[email protected]>

Author: nrusch

src/rez/bind/_utils.py

@@ -26,8 +26,7 @@ def log(msg):
 
 def make_dirs(*dirs):
     path = os.path.join(*dirs)
-    if not os.path.exists(path):
-        os.makedirs(path)
+    os.makedirs(path, exist_ok=True)
     return path
 
 

src/rez/package_cache.py

@@ -24,8 +24,7 @@
 from rez.exceptions import PackageCacheError
 from rez.vendor.lockfile import LockFile, NotLocked
 from rez.vendor.progress.spinner import PixelSpinner
-from rez.utils.filesystem import safe_listdir, safe_makedirs, safe_remove, \
-    forceful_rmtree
+from rez.utils.filesystem import forceful_rmtree, safe_listdir, safe_remove
 from rez.utils.colorize import ColorizedStreamHandler
 from rez.utils.logging_ import print_warning
 from rez.packages import get_variant
@@ -100,9 +99,9 @@ def __init__(self, path):
         self.path = path
 
         # make dirs for internal use
-        safe_makedirs(self._log_dir)
-        safe_makedirs(self._pending_dir)
-        safe_makedirs(self._remove_dir)
+        os.makedirs(self._log_dir, exist_ok=True)
+        os.makedirs(self._pending_dir, exist_ok=True)
+        os.makedirs(self._remove_dir, exist_ok=True)
 
     def get_cached_root(self, variant):
         """Get location of variant payload copy.
@@ -172,7 +171,6 @@ def add_variant(self, variant, force=False, wait_for_copying=False, logger=None)
             - int: One of VARIANT_FOUND, VARIANT_CREATED, VARIANT_COPYING, VARIANT_COPY_STALLED
         """
         from rez.utils.base26 import get_next_base26
-        from rez.utils.filesystem import safe_makedirs
 
         # do some sanity checking on variant to cache
         package = variant.parent
@@ -263,7 +261,7 @@ def add_variant(self, variant, force=False, wait_for_copying=False, logger=None)
 
         # 1.
         path = self._get_hash_path(variant)
-        safe_makedirs(path)
+        os.makedirs(path, exist_ok=True)
 
         # construct data to store to json file
         data = {

src/rez/package_copy.py

@@ -17,7 +17,7 @@
 from rez.utils.sourcecode import IncludeModuleManager
 from rez.utils.logging_ import print_info, print_warning
 from rez.utils.filesystem import replacing_symlink, replacing_copy, \
-    safe_makedirs, additive_copytree, make_path_writable, get_existing_path
+    additive_copytree, make_path_writable, get_existing_path
 
 
 def copy_package(package, dest_repository, variants=None, shallow=False,
@@ -287,7 +287,7 @@ def _copy_variant_payload(src_variant, dest_pkg_repo, shallow=False,
 
     # copy the variant payload
     with ctxt:
-        safe_makedirs(variant_install_path)
+        os.makedirs(variant_install_path, exist_ok=True)
 
         # determine files not to copy
         skip_files = []
@@ -362,7 +362,7 @@ def _copy_variant_payload(src_variant, dest_pkg_repo, shallow=False,
                 src_package.config.variant_shortlinks_dirname
             )
 
-            safe_makedirs(base_shortlinks_path)
+            os.makedirs(base_shortlinks_path, exist_ok=True)
 
             # shortlink
             rel_variant_path = os.path.relpath(
@@ -425,5 +425,5 @@ def _copy_package_include_modules(src_package, dest_pkg_repo, overrides=None):
         ctxt = with_noop()
 
     with ctxt:
-        safe_makedirs(dest_include_modules_path)
+        os.makedirs(dest_include_modules_path, exist_ok=True)
         additive_copytree(src_include_modules_path, dest_include_modules_path)

src/rez/package_maker.py

@@ -220,15 +220,13 @@ def make_package(name, path, make_base=None, make_root=None, skip_existing=True,
 
             base = variant_.base
             if make_base and base:
-                if not os.path.exists(base):
-                    os.makedirs(base)
+                os.makedirs(base, exist_ok=True)
                 os.chdir(base)
                 make_base(variant_, base)
 
             root = variant_.root
             if make_root and root:
-                if not os.path.exists(root):
-                    os.makedirs(root)
+                os.makedirs(root, exist_ok=True)
                 os.chdir(root)
                 make_root(variant_, root)
 

src/rez/pip.py

@@ -386,8 +386,7 @@ def make_root(variant, path):
                 src = os.path.join(targetpath, rel_src)
                 dest = os.path.join(path, rel_dest)
 
-                if not os.path.exists(os.path.dirname(dest)):
-                    os.makedirs(os.path.dirname(dest))
+                os.makedirs(os.path.dirname(dest), exist_ok=True)
 
                 shutil.copyfile(src, dest)
 

src/rez/suite.py

@@ -441,7 +441,7 @@ def save(self, path, verbose=False):
                 raise SuiteError("Cannot save, path exists: %r" % path)
 
         contexts_path = os.path.join(path, "contexts")
-        os.makedirs(contexts_path)
+        os.makedirs(contexts_path, exist_ok=True)
 
         # write suite data
         data = self.to_dict()
@@ -460,7 +460,7 @@ def save(self, path, verbose=False):
 
         # create alias wrappers
         tools_path = os.path.join(path, "bin")
-        os.makedirs(tools_path)
+        os.makedirs(tools_path, exist_ok=True)
         if verbose:
             print("creating alias wrappers in %r..." % tools_path)
 

src/rez/tests/test_packages.py

@@ -347,8 +347,7 @@ def test_variant_iteration(self):
     def test_variant_install(self):
         """test variant installation."""
         repo_path = os.path.join(self.root, "packages")
-        if not os.path.exists(repo_path):
-            os.makedirs(repo_path)
+        os.makedirs(repo_path, exist_ok=True)
 
         def _data(obj):
             d = obj.validated_data()

src/rez/utils/filesystem.py

@@ -173,19 +173,6 @@ def safe_listdir(path):
         raise
 
 
-def safe_makedirs(path):
-    """Safe makedirs.
-
-    Works in a multithreaded scenario.
-    """
-    if not os.path.exists(path):
-        try:
-            os.makedirs(path)
-        except OSError:
-            if not os.path.exists(path):
-                raise
-
-
 def safe_remove(path):
     """Safely remove the given file or directory.
 
@@ -303,8 +290,7 @@ def replace_file_or_dir(dest, source):
 def additive_copytree(src, dst, symlinks=False, ignore=None):
     """Version of `copytree` that merges into an existing directory.
     """
-    if not os.path.exists(dst):
-        os.makedirs(dst)
+    os.makedirs(dst, exist_ok=True)
 
     for item in os.listdir(src):
         s = os.path.join(src, item)
@@ -437,8 +423,7 @@ def copy(srcname, dstname):
     else:
         copy = shutil.copy2
 
-    if not os.path.isdir(dst):
-        os.makedirs(dst)
+    os.makedirs(dst, exist_ok=True)
 
     errors = []
     for name in names:

src/rez/utils/py_dist.py

@@ -16,8 +16,7 @@
 
 def _mkdirs(*dirs):
     path = os.path.join(*dirs)
-    if not os.path.exists(path):
-        os.makedirs(path)
+    os.makedirs(path, exist_ok=True)
     return path
 
 

src/rezplugins/build_process/local.py

@@ -14,8 +14,8 @@
 from rez.utils.logging_ import print_warning
 from rez.utils.base26 import create_unique_base26_symlink
 from rez.utils.colorize import Printer, warning
-from rez.utils.filesystem import safe_makedirs, copy_or_replace, \
-    make_path_writable, get_existing_path, forceful_rmtree
+from rez.utils.filesystem import copy_or_replace, get_existing_path, \
+    forceful_rmtree, make_path_writable
 from rez.utils.sourcecode import IncludeModuleManager
 from rez.utils.filesystem import TempDirs
 from rez.package_test import PackageTestRunner, PackageTestResults
@@ -148,7 +148,7 @@ def _build_variant_base(self, variant, build_type, install_path=None,
         if clean and os.path.exists(variant_build_path):
             self._rmtree(variant_build_path)
 
-        safe_makedirs(variant_build_path)
+        os.makedirs(variant_build_path, exist_ok=True)
 
         # find last dir of installation path that exists, and possibly make it
         # writable during variant installation
@@ -167,8 +167,7 @@ def _build_variant_base(self, variant, build_type, install_path=None,
                 pkg_repo = package_repository_manager.get_repository(install_path)
                 pkg_repo.pre_variant_install(variant.resource)
 
-                if not os.path.exists(variant_install_path):
-                    safe_makedirs(variant_install_path)
+                os.makedirs(variant_install_path, exist_ok=True)
 
                 # if hashed variants are enabled, create the variant shortlink
                 if variant.parent.hashed_variants:
@@ -179,7 +178,7 @@ def _build_variant_base(self, variant, build_type, install_path=None,
                             variant.parent.config.variant_shortlinks_dirname
                         )
 
-                        safe_makedirs(base_shortlinks_path)
+                        os.makedirs(base_shortlinks_path, exist_ok=True)
 
                         # create the shortlink
                         rel_variant_path = os.path.relpath(
@@ -293,7 +292,7 @@ def _install_include_modules(self, install_path):
         base_path = self.get_package_install_path(install_path)
 
         path = os.path.join(base_path, IncludeModuleManager.include_modules_subpath)
-        safe_makedirs(path)
+        os.makedirs(path, exist_ok=True)
 
         definition_python_path = self.package.config.package_definition_python_path