Skip to content

Commit 416aad7

Browse files
committed
project: use os.rename() instead of shutil.move() for cloned manifest
As reported in zephyrproject-rtos#963 (thanks!) and seen in the source of https://github.com/python/cpython/blob/main/Lib/shutil.py, `shutil.move()` tries an (atomic) `os.rename()` first, then falls back on a copy+delete in case of an OSError raised by `os.rename()`. The source and destination being on different filesystems is one of the (too many?) conditions caught by this OSError and how `shutil.move()` claims to "optimize" moves on the same filesystem thanks to `os.rename()`. Maybe there isn't another, simple and reliable way to check whether source and destionation are on the same filesystem and perform this optimization? In this particular `project.py` use case however, we _know_ that both are on the same filesystem because we purposely located them in the same directory so we can benefit from very fast and atomic renames guaranteed by practically all filesystems in the universe. So, let's simplify `shutil.move()` down to `os.rename()` in this particular case. Quoting the same `Lib/shutil.py#move()` source: os.rename() is preferably used if the source and destination are on the same filesystem. [...] It's recommended to use os.rename() if atomic move is strictly required. But wait... if we know we are always on the same filesystem, then why does this even matter? os.rename() should always succeed inside shutil.move() and this minor cleanup should never make any visible difference, right? To answer that we need to go back to zephyrproject-rtos#963, NTFS and antivirus scanners which when combined together can break even the simplest things like renaming a directory. While this simplification is far from making west compatible with such utterly broken systems, it does remove a lot of complexity and headaches when such a failure happens and it recommends the existing --rename-delay workaround. Signed-off-by: Marc Herbert <Marc.Herbert@gmail.com>
1 parent 1152ac2 commit 416aad7

1 file changed

Lines changed: 5 additions & 7 deletions

File tree

src/west/app/project.py

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -516,18 +516,16 @@ def bootstrap(self, args) -> Path:
516516

517517
self.dbg('moving', tempdir, 'to', manifest_abspath, level=Verbosity.DBG_EXTREME)
518518

519-
# As shutil.move() is used to relocate tempdir, if manifest_abspath
520-
# is an existing directory, tmpdir will be moved _inside_ it, instead
521-
# of _to_ that path - this must be avoided. If manifest_abspath exists
522-
# but is not a directory, then semantics depend on os.rename(), so
523-
# avoid that too...
519+
# If manifest_abspath exists but is not a directory, then semantics
520+
# depend on os.rename(), so avoid that
524521
if manifest_abspath.exists():
525522
self.die(f'target directory already exists ({manifest_abspath})')
526523

527524
manifest_abspath.parent.mkdir(parents=True, exist_ok=True)
528525
try:
529-
shutil.move(os.fspath(tempdir), os.fspath(manifest_abspath))
530-
except shutil.Error as e:
526+
os.rename(os.fspath(tempdir), os.fspath(manifest_abspath))
527+
except OSError as e:
528+
self.err("try west init --rename-delay; see west init -h")
531529
self.die(e)
532530
self.small_banner('setting manifest.path to', manifest_path)
533531
self.config = Configuration(topdir=topdir)

0 commit comments

Comments
 (0)