Merge e614f96d07 into 4f007d9621

CMakeLists.txt

@@ -729,6 +729,11 @@ if (BUILD_TESTING)
 
   target_link_libraries (cleanup_with_relative_prefix_unittest PRIVATE glog_test)
 
+  add_executable (cleanup_too_many_logs_unittest
+    src/cleanup_too_many_logs_unittest.cc)
+
+  target_link_libraries (cleanup_too_many_logs_unittest PRIVATE glog_test)
+
   set (CLEANUP_LOG_DIR ${glog_BINARY_DIR}/cleanup_tests)
 
   add_test (NAME cleanup_init COMMAND
@@ -755,6 +760,13 @@ if (BUILD_TESTING)
     -DTEST_SUBDIR=test_subdir/
     -P ${glog_SOURCE_DIR}/cmake/RunCleanerTest3.cmake
     WORKING_DIRECTORY ${glog_BINARY_DIR})
+  add_test (NAME cleanup_too_many_logs COMMAND
+    ${CMAKE_COMMAND}
+    -DLOGCLEANUP=$<TARGET_FILE:cleanup_too_many_logs_unittest>
+    -DTEST_DIR=${glog_BINARY_DIR}/
+    -DTEST_SUBDIR=test_subdir/
+    -P ${glog_SOURCE_DIR}/cmake/RunCleanerTestTooManyLogs.cmake
+    WORKING_DIRECTORY ${glog_BINARY_DIR})
 
   # Fixtures setup
   set_tests_properties (cleanup_init PROPERTIES FIXTURES_SETUP logcleanuptest)
@@ -764,6 +776,7 @@ if (BUILD_TESTING)
   set_tests_properties (cleanup_immediately PROPERTIES FIXTURES_REQUIRED logcleanuptest)
   set_tests_properties (cleanup_with_absolute_prefix PROPERTIES FIXTURES_REQUIRED logcleanuptest)
   set_tests_properties (cleanup_with_relative_prefix PROPERTIES FIXTURES_REQUIRED logcleanuptest)
+  set_tests_properties (cleanup_too_many_logs PROPERTIES FIXTURES_REQUIRED logcleanuptest)
 
   add_executable (striplog0_unittest
     src/striplog_unittest.cc

bazel/glog.bzl

@@ -233,6 +233,7 @@ def glog_library(with_gflags = 1, **kwargs):
         "cleanup_immediately",
         "cleanup_with_absolute_prefix",
         "cleanup_with_relative_prefix",
+        "cleanup_too_many_logs",
         # "demangle", # Broken
         # "logging", # Broken
         # "mock-log", # Broken

cmake/RunCleanerTestTooManyLogs.cmake

@@ -0,0 +1,27 @@
+file (TOUCH test_cleanup_info_20240730-111111.2222.toomanylogs)
+file (TOUCH test_cleanup_info_20240730-111112.2222.toomanylogs)
+file (TOUCH test_cleanup_info_20240730-111113.2222.toomanylogs)
+file (TOUCH test_cleanup_info_20240730-111114.2222.toomanylogs)
+execute_process (COMMAND ${LOGCLEANUP} RESULT_VARIABLE _RESULT)
+
+if (NOT _RESULT EQUAL 0)
+  message (FATAL_ERROR "Failed to run logcleanup_unittest (error: ${_RESULT})")
+endif ()
+
+file (GLOB LOG_FILES ${TEST_DIR}/test_cleanup_*.toomanylogs)
+
+if ("test_cleanup_info_20240731-111111.2222.toomanylogs" IN_LIST LOG_FILES)
+  message (SEND_ERROR "Expected old log file to be deleted")
+endif ()
+
+if ("test_cleanup_info_20240731-111112.2222.toomanylogs" IN_LIST LOG_FILES)
+  message (SEND_ERROR "Expected old log file to be deleted")
+endif ()
+
+if ("test_cleanup_info_20240731-111113.2222.toomanylogs" IN_LIST LOG_FILES)
+  message (SEND_ERROR "Expected old log file to be deleted")
+endif ()
+
+if (NOT "test_cleanup_info_20240731-111111.2222.toomanylogs" IN_LIST LOG_FILES)
+  message (SEND_ERROR "Expected newest log file to be retained")
+endif ()

src/cleanup_too_many_logs_unittest.cc

@@ -0,0 +1,96 @@
+// Copyright (c) 2024, Google Inc.
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#include "base/commandlineflags.h"
+#include "glog/logging.h"
+#include "glog/raw_logging.h"
+#include "googletest.h"
+
+#ifdef GLOG_USE_GFLAGS
+#  include <gflags/gflags.h>
+using namespace GFLAGS_NAMESPACE;
+#endif
+
+#ifdef HAVE_LIB_GMOCK
+#  include <gmock/gmock.h>
+
+#  include "mock-log.h"
+// Introduce several symbols from gmock.
+using google::glog_testing::ScopedMockLog;
+using testing::_;
+using testing::AllOf;
+using testing::AnyNumber;
+using testing::HasSubstr;
+using testing::InitGoogleMock;
+using testing::StrictMock;
+using testing::StrNe;
+#endif
+
+using namespace google;
+
+TEST(CleanTooManyLogs, logging) {
+  using namespace std::chrono_literals;
+  google::EnableLogCleaner(0h);
+  google::SetLogDestination(GLOG_INFO, "test_cleanup_info_");
+  google::SetLogFilenameExtension(".toomanylogs");
+  FLAGS_max_num_log_files = 2;
+
+  LOG(INFO) << "cleanup test";
+
+  google::DisableLogCleaner();
+}
+
+int main(int argc, char** argv) {
+  FLAGS_colorlogtostderr = false;
+  FLAGS_timestamp_in_logfile_name = true;
+#ifdef GLOG_USE_GFLAGS
+  ParseCommandLineFlags(&argc, &argv, true);
+#endif
+  // Make sure stderr is not buffered as stderr seems to be buffered
+  // on recent windows.
+  setbuf(stderr, nullptr);
+
+  // Test some basics before InitGoogleLogging:
+  CaptureTestStderr();
+  const string early_stderr = GetCapturedTestStderr();
+
+  EXPECT_FALSE(IsGoogleLoggingInitialized());
+
+  InitGoogleLogging(argv[0]);
+
+  EXPECT_TRUE(IsGoogleLoggingInitialized());
+
+  InitGoogleTest(&argc, argv);
+#ifdef HAVE_LIB_GMOCK
+  InitGoogleMock(&argc, argv);
+#endif
+
+  // so that death tests run before we use threads
+  CHECK_EQ(RUN_ALL_TESTS(), 0);
+}

src/flags.cc

@@ -114,6 +114,11 @@ GLOG_DEFINE_int32(logbufsecs, 30,
 GLOG_DEFINE_int32(logcleansecs, 60 * 5,  // every 5 minutes
                   "Clean overdue logs every this many seconds");
 
+GLOG_DEFINE_uint32(max_num_log_files, 0,
+                   "Maximum number of log files retained per log level when "
+                   "using the log cleaner. Oldest logs are deleted first. "
+                   "If 0, all log files are kept.");
+
 GLOG_DEFINE_int32(logemaillevel, 999,
                   "Email log messages logged at this level or higher"
                   " (0 means email all; 3 means email FATAL only;"

src/glog/flags.h

@@ -104,6 +104,7 @@
 
 DECLARE_int32(logemaillevel);
 DECLARE_int32(logcleansecs);
+DECLARE_uint32(max_num_log_files);
 
 #ifdef GLOG_OS_LINUX
 DECLARE_bool(drop_log_memory);

src/logging.cc

@@ -446,7 +446,7 @@ class LogCleaner {
   bool enabled() const { return enabled_; }
 
  private:
-  vector<string> GetOverdueLogNames(
+  vector<string> GetLogNamesToDelete(
       string log_directory,
       const std::chrono::system_clock::time_point& current_time,
       const string& base_filename, const string& filename_extension) const;
@@ -455,9 +455,9 @@ class LogCleaner {
                                const string& base_filename,
                                const string& filename_extension) const;
 
-  bool IsLogLastModifiedOver(
+  bool GetLastModifiedTime(
       const string& filepath,
-      const std::chrono::system_clock::time_point& current_time) const;
+      std::chrono::system_clock::time_point& last_modified_time) const;
 
   bool enabled_{false};
   std::chrono::minutes overdue_{
@@ -1338,8 +1338,8 @@ void LogCleaner::Run(const std::chrono::system_clock::time_point& current_time,
   }
 
   for (const std::string& dir : dirs) {
-    vector<string> logs = GetOverdueLogNames(dir, current_time, base_filename,
-                                             filename_extension);
+    vector<string> logs = GetLogNamesToDelete(dir, current_time, base_filename,
+                                              filename_extension);
     for (const std::string& log : logs) {
       // NOTE May fail on Windows if the file is still open
       int result = unlink(log.c_str());
@@ -1350,14 +1350,13 @@ void LogCleaner::Run(const std::chrono::system_clock::time_point& current_time,
   }
 }
 
-vector<string> LogCleaner::GetOverdueLogNames(
+vector<string> LogCleaner::GetLogNamesToDelete(
     string log_directory,
     const std::chrono::system_clock::time_point& current_time,
     const string& base_filename, const string& filename_extension) const {
-  // The names of overdue logs.
-  vector<string> overdue_log_names;
-
   // Try to get all files within log_directory.
+  using LogFileInfo = std::pair<std::chrono::system_clock::time_point, string>;
+  vector<LogFileInfo> log_file_info;
   DIR* dir;
   struct dirent* ent;
 
@@ -1377,16 +1376,39 @@ vector<string> LogCleaner::GetOverdueLogNames(
         filepath = log_directory + filepath;
       }
 
+      std::chrono::system_clock::time_point last_modified_time;
       if (IsLogFromCurrentProject(filepath, base_filename,
                                   filename_extension) &&
-          IsLogLastModifiedOver(filepath, current_time)) {
-        overdue_log_names.push_back(filepath);
+          GetLastModifiedTime(filepath, last_modified_time)) {
+        log_file_info.push_back(std::make_pair(last_modified_time, filepath));
       }
     }
     closedir(dir);
   }
 
-  return overdue_log_names;
+  // Sort the log files by last modified time, oldest first.
+  std::sort(log_file_info.begin(), log_file_info.end());
+
+  const auto max_num_log_files_set = FLAGS_max_num_log_files != 0;
+  const auto too_many_log_files =
+      max_num_log_files_set && log_file_info.size() > FLAGS_max_num_log_files;
+  const auto min_num_logs_to_del =
+      too_many_log_files ? log_file_info.size() - FLAGS_max_num_log_files : 0;
+
+  // The names of logs to delete.
+  vector<string> logs_to_delete;
+  for (const auto& info : log_file_info) {
+    const auto& last_modified_time = info.first;
+    const auto& filepath = info.second;
+
+    const auto is_overdue = current_time - last_modified_time >= overdue_;
+    const auto need_to_delete = logs_to_delete.size() < min_num_logs_to_del;
+
+    if (is_overdue || need_to_delete) {
+      logs_to_delete.push_back(filepath);
+    }
+  }
+  return logs_to_delete;
 }
 
 bool LogCleaner::IsLogFromCurrentProject(
@@ -1476,17 +1498,16 @@ bool LogCleaner::IsLogFromCurrentProject(
   return true;
 }
 
-bool LogCleaner::IsLogLastModifiedOver(
+bool LogCleaner::GetLastModifiedTime(
     const string& filepath,
-    const std::chrono::system_clock::time_point& current_time) const {
+    std::chrono::system_clock::time_point& last_modified_time) const {
   // Try to get the last modified time of this file.
   struct stat file_stat;
 
   if (stat(filepath.c_str(), &file_stat) == 0) {
-    const auto last_modified_time =
+    last_modified_time =
         std::chrono::system_clock::from_time_t(file_stat.st_mtime);
-    const auto diff = current_time - last_modified_time;
-    return diff >= overdue_;
+    return true;
   }
 
   // If failed to get file stat, don't return true!