Made semantics of `os::rmdir` consistent between POSIX and Windows. This commit will fix 2 known bugs in the Windows implementation of `os::rmdir`, as chronicled in MESOS-5942, namely:
1. Calling `os::rmdir` with a file argument (rather than a directory) and the `recursive` parameter set to `true` will fail on Windows, but succeed on POSIX. The POSIX semantics of `os::rmdir` are a union of `rm -r` and `::rmdir`, the behavior of which depends on the arguments the caller passes in. If the formal parameter `recursive` is set to `true`, then the semantics are meant to be`rm -r`; if `false`, the semantics are meant to be `::rmdir`. The implications of this are somewhat subtle: `::rmdir` will error out if you try to delete (e.g.) regular files, while `rm -r` will happily delete them. On Windows, we currently always have `::rmdir`-style semantics, in that we if you pass a path that points at a file to `os::rmdir`, it will not delete that file. This commit will reverse this, and move make the Windows implementation semantically identical to the POSIX implementation (at least in this regard). 2. Recursively deleting nested directories fails on windows when `removeRoot` is set to `false`. Currently if you set the `removeRoot` parameter to `false`, the Windows implementation of `os::rmdir` will fail to delete a directory inside a directory. The reason is that we are propagating the `removeRoot` flag to the recursive calls to `os::rmdir`. The implication of this is that the recursive call will *not* delete the nested directory (since `removeRoot` is `false`). This commit will fix this by setting `removeRoot` to `true` in recursive calls to `os::rmdir`. Review: https://reviews.apache.org/r/50673/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/719f7c0f Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/719f7c0f Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/719f7c0f Branch: refs/heads/master Commit: 719f7c0f335834322088e7921255a1352f4f2b41 Parents: 2fd78e6 Author: Alex Clemmer <clemmer.alexan...@gmail.com> Authored: Thu Oct 13 16:25:58 2016 -0700 Committer: Joseph Wu <josep...@apache.org> Committed: Fri Oct 14 16:46:54 2016 -0700 ---------------------------------------------------------------------- .../stout/include/stout/os/windows/rmdir.hpp | 21 ++++++- 3rdparty/stout/tests/os/rmdir_tests.cpp | 59 ++++++++++++++++++++ 2 files changed, 77 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/719f7c0f/3rdparty/stout/include/stout/os/windows/rmdir.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/stout/include/stout/os/windows/rmdir.hpp b/3rdparty/stout/include/stout/os/windows/rmdir.hpp index b74bf71..4437484 100644 --- a/3rdparty/stout/include/stout/os/windows/rmdir.hpp +++ b/3rdparty/stout/include/stout/os/windows/rmdir.hpp @@ -21,6 +21,7 @@ #include <stout/windows.hpp> #include <stout/os/realpath.hpp> +#include <stout/os/rm.hpp> #include <stout/os/stat.hpp> #include <stout/windows/error.hpp> @@ -29,11 +30,25 @@ namespace os { namespace internal { -// Recursive version of `RemoveDirectory`. NOTE: unlike `rmdir`, this requires -// Windows-formatted paths, and therefore should be in the `internal` namespace. +// Recursive version of `RemoveDirectory`. Two things are notable about this +// implementation: +// +// 1. Unlike `rmdir`, this requires Windows-formatted paths, and therefore +// should be in the `internal` namespace. +// 2. To match the semantics of the POSIX implementation, this function +// implements the semantics of `rm -r`, rather than `rmdir`. In particular, +// if `path` points at a file, this function will delete it, while a call to +// `rmdir` will not. inline Try<Nothing> recursive_remove_directory( const std::string& path, bool removeRoot, bool continueOnError) { + // NOTE: Special case required to match the semantics of POSIX. See comment + // above. As below, this also handles symlinks correctly, i.e., given a path + // to a symlink, we delete the symlink rather than the target. + if (os::stat::isfile(path)) { + return os::rm(path); + } + // Appending a slash here if the path doesn't already have one simplifies // path join logic later, because (unlike Unix) Windows doesn't like double // slashes in paths. @@ -79,7 +94,7 @@ inline Try<Nothing> recursive_remove_directory( // Delete current path, whether it's a directory, file, or symlink. if (is_directory) { Try<Nothing> removed = recursive_remove_directory( - current_absolute_path, removeRoot, continueOnError); + current_absolute_path, true, continueOnError); if (removed.isError()) { if (continueOnError) { http://git-wip-us.apache.org/repos/asf/mesos/blob/719f7c0f/3rdparty/stout/tests/os/rmdir_tests.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/stout/tests/os/rmdir_tests.cpp b/3rdparty/stout/tests/os/rmdir_tests.cpp index ffe234b..e19a508 100644 --- a/3rdparty/stout/tests/os/rmdir_tests.cpp +++ b/3rdparty/stout/tests/os/rmdir_tests.cpp @@ -94,6 +94,65 @@ TEST_F(RmdirTest, TrivialRemoveEmptyDirectoryRelativePath) } +// Tests behavior of `rmdir` when path points at a file instead of a directory. +TEST_F(RmdirTest, RemoveFile) +{ + const string tmpdir = os::getcwd(); + hashset<string> expectedRootListing = hashset<string>::EMPTY; + hashset<string> expectedSubListing = hashset<string>::EMPTY; + + // Directory is initially empty. + EXPECT_EQ(expectedRootListing, listfiles(tmpdir)); + + // Successfully make directory using absolute path, and then `touch` a file + // in that folder. + const string newDirectoryName = "newDirectory"; + const string newDirectoryAbsolutePath = path::join(tmpdir, newDirectoryName); + const string newFileName = "newFile"; + const string newFileAbsolutePath = path::join( + newDirectoryAbsolutePath, + newFileName); + + expectedRootListing.insert(newDirectoryName); + expectedSubListing.insert(newFileName); + + EXPECT_SOME(os::mkdir(newDirectoryAbsolutePath)); + EXPECT_SOME(os::touch(newFileAbsolutePath)); + EXPECT_EQ(expectedRootListing, listfiles(tmpdir)); + EXPECT_EQ(expectedSubListing, listfiles(newDirectoryAbsolutePath)); + + // Successful recursive remove with `removeRoot` set to `true` (using the + // semantics of `rm -r`). + EXPECT_SOME(os::rmdir(newFileAbsolutePath)); + EXPECT_TRUE(os::exists(newDirectoryAbsolutePath)); + ASSERT_EQ(hashset<string>::EMPTY, listfiles(newDirectoryAbsolutePath)); + + // Add file to directory again. + EXPECT_SOME(os::touch(newFileAbsolutePath)); + + // Successful recursive remove with `removeRoot` set to `false` (using the + // semantics of `rm -r`). + EXPECT_SOME(os::rmdir(newFileAbsolutePath, true, false)); + EXPECT_TRUE(os::exists(newDirectoryAbsolutePath)); + ASSERT_EQ(hashset<string>::EMPTY, listfiles(newDirectoryAbsolutePath)); + + // Add file to directory again. + EXPECT_SOME(os::touch(newFileAbsolutePath)); + + // Error on non-recursive remove with `removeRoot` set to `true` (using the + // semantics of `rmdir`). + EXPECT_ERROR(os::rmdir(newFileAbsolutePath, false, true)); + EXPECT_TRUE(os::exists(newDirectoryAbsolutePath)); + EXPECT_TRUE(os::exists(newFileAbsolutePath)); + + // Error on non-recursive remove with `removeRoot` set to `false` (using the + // semantics of `rmdir`). + EXPECT_ERROR(os::rmdir(newFileAbsolutePath, false, false)); + EXPECT_TRUE(os::exists(newDirectoryAbsolutePath)); + EXPECT_TRUE(os::exists(newFileAbsolutePath)); +} + + TEST_F(RmdirTest, RemoveRecursiveByDefault) { const string tmpdir = os::getcwd();
