From a58b058a39aa34f33affac27433e2bc708b32f23 Mon Sep 17 00:00:00 2001 From: Mischa Jonker Date: Mon, 13 Sep 2021 13:30:38 +0200 Subject: Fix ninja -t clean for directories on Windows remove() deletes both files and directories. On Windows we have to select the correct function (DeleteFile will yield Permission Denied when used on a directory) This fixes the behavior of ninja -t clean in some cases https://github.com/ninja-build/ninja/issues/828 --- src/disk_interface.cc | 31 ++++++++++++++++++++++++------- src/disk_interface_test.cc | 8 ++++++++ 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/src/disk_interface.cc b/src/disk_interface.cc index a9497cb..a37c570 100644 --- a/src/disk_interface.cc +++ b/src/disk_interface.cc @@ -279,14 +279,31 @@ int RealDiskInterface::RemoveFile(const string& path) { // Skip error checking. If this fails, accept whatever happens below. SetFileAttributes(path.c_str(), attributes & ~FILE_ATTRIBUTE_READONLY); } - if (!DeleteFile(path.c_str())) { - DWORD win_err = GetLastError(); - if (win_err == ERROR_FILE_NOT_FOUND || win_err == ERROR_PATH_NOT_FOUND) { - return 1; + if (attributes & FILE_ATTRIBUTE_DIRECTORY) { + // remove() deletes both files and directories. On Windows we have to + // select the correct function (DeleteFile will yield Permission Denied when + // used on a directory) + // This fixes the behavior of ninja -t clean in some cases + // https://github.com/ninja-build/ninja/issues/828 + if (!RemoveDirectory(path.c_str())) { + DWORD win_err = GetLastError(); + if (win_err == ERROR_FILE_NOT_FOUND || win_err == ERROR_PATH_NOT_FOUND) { + return 1; + } + // Report remove(), not RemoveDirectory(), for cross-platform consistency. + Error("remove(%s): %s", path.c_str(), GetLastErrorString().c_str()); + return -1; + } + } else { + if (!DeleteFile(path.c_str())) { + DWORD win_err = GetLastError(); + if (win_err == ERROR_FILE_NOT_FOUND || win_err == ERROR_PATH_NOT_FOUND) { + return 1; + } + // Report as remove(), not DeleteFile(), for cross-platform consistency. + Error("remove(%s): %s", path.c_str(), GetLastErrorString().c_str()); + return -1; } - // Report as remove(), not DeleteFile(), for cross-platform consistency. - Error("remove(%s): %s", path.c_str(), GetLastErrorString().c_str()); - return -1; } #else if (remove(path.c_str()) < 0) { diff --git a/src/disk_interface_test.cc b/src/disk_interface_test.cc index b424243..339aea1 100644 --- a/src/disk_interface_test.cc +++ b/src/disk_interface_test.cc @@ -219,6 +219,14 @@ TEST_F(DiskInterfaceTest, RemoveFile) { #endif } +TEST_F(DiskInterfaceTest, RemoveDirectory) { + const char* kDirectoryName = "directory-to-remove"; + EXPECT_TRUE(disk_.MakeDir(kDirectoryName)); + EXPECT_EQ(0, disk_.RemoveFile(kDirectoryName)); + EXPECT_EQ(1, disk_.RemoveFile(kDirectoryName)); + EXPECT_EQ(1, disk_.RemoveFile("does not exist")); +} + struct StatTest : public StateTestWithBuiltinRules, public DiskInterface { StatTest() : scan_(&state_, NULL, NULL, this, NULL) {} -- cgit v0.12