Fix and detect headers with missing dependencies (#8893)

Summary:
It's always annoying to find a header does not include its own
dependencies and only works when included after other includes. This
change adds `make check-headers` which validates that each header can
be included at the top of a file. Some headers are excluded e.g. because
of platform or external dependencies.

rocksdb_namespace.h had to be re-worked slightly to enable checking for
failure to include it. (ROCKSDB_NAMESPACE is a valid namespace name.)

Fixes mostly involve adding and cleaning up #includes, but for
FileTraceWriter, a constructor was out-of-lined to make a forward
declaration sufficient.

This check is not currently run with `make check` but is added to
CircleCI build-linux-unity since that one is already relatively fast.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/8893

Test Plan: existing tests and resolving issues detected by new check

Reviewed By: mrambacher

Differential Revision: D30823300

Pulled By: pdillinger

fbshipit-source-id: 9fff223944994c83c105e2e6496d24845dc8e572

.circleci/config.yml

@@ -328,7 +328,7 @@ jobs:
       - run: (mkdir build && cd build && cmake -DWITH_GFLAGS=1 -DWITH_BENCHMARK=1 .. && make V=1 -j20 && ctest -j20 && make microbench) | .circleci/cat_ignore_eagain
       - post-steps
 
-  build-linux-unity:
+  build-linux-unity-and-headers:
     docker: # executor type
       - image: gcc:latest
     resource_class: large
@@ -336,6 +336,7 @@ jobs:
       - checkout # check out the code in the project directory
       - run: apt-get update -y && apt-get install -y libgflags-dev
       - run: TEST_TMPDIR=/dev/shm && make V=1 -j8 unity_test | .circleci/cat_ignore_eagain
+      - run: make V=1 -j8 -k check-headers | .circleci/cat_ignore_eagain # could be moved to a different build
       - post-steps
 
   build-linux-gcc-4_8-no_test_run:
@@ -730,9 +731,9 @@ workflows:
   build-linux-clang10-clang-analyze:
     jobs:
       - build-linux-clang10-clang-analyze
-  build-linux-unity:
+  build-linux-unity-and-headers:
     jobs:
-      - build-linux-unity
+      - build-linux-unity-and-headers
   build-windows-vs2019:
     jobs:
       - build-windows:

Makefile

@@ -516,6 +516,29 @@ ifeq ($(USE_FOLLY_DISTRIBUTED_MUTEX),1)
 	ALL_SOURCES += third-party/folly/folly/synchronization/test/DistributedMutexTest.cc
 endif
 
+# `make check-headers` to very that each header file includes its own
+# dependencies
+ifneq ($(filter check-headers, $(MAKECMDGOALS)),)
+# TODO: add/support JNI headers
+	DEV_HEADER_DIRS := $(sort include/ hdfs/ $(dir $(ALL_SOURCES)))
+# Some headers like in port/ are platform-specific
+	DEV_HEADERS := $(shell $(FIND) $(DEV_HEADER_DIRS) -type f -name '*.h' | egrep -v 'port/|plugin/|lua/|range_tree/|tools/rdb/db_wrapper.h|include/rocksdb/utilities/env_librados.h')
+else
+	DEV_HEADERS :=
+endif
+HEADER_OK_FILES = $(patsubst %.h, %.h.ok, $(DEV_HEADERS))
+
+AM_V_CCH = $(am__v_CCH_$(V))
+am__v_CCH_ = $(am__v_CCH_$(AM_DEFAULT_VERBOSITY))
+am__v_CCH_0 = @echo "  CC.h    " $<;
+am__v_CCH_1 =
+
+%.h.ok: %.h # .h.ok not actually created, so re-checked on each invocation
+# -DROCKSDB_NAMESPACE=42 ensures the namespace header is included
+	$(AM_V_CCH) echo '#include "$<"' | $(CXX) $(CXXFLAGS) -DROCKSDB_NAMESPACE=42 -x c++ -c - -o /dev/null
+
+check-headers: $(HEADER_OK_FILES)
+
 # options_settable_test doesn't pass with UBSAN as we use hack in the test
 ifdef COMPILE_WITH_UBSAN
         TESTS := $(shell echo $(TESTS) | sed 's/\boptions_settable_test\b//g')
@@ -716,7 +739,7 @@ endif  # PLATFORM_SHARED_EXT
 .PHONY: blackbox_crash_test check clean coverage crash_test ldb_tests package \
 	release tags tags0 valgrind_check whitebox_crash_test format static_lib shared_lib all \
 	dbg rocksdbjavastatic rocksdbjava gen-pc install install-static install-shared uninstall \
-	analyze tools tools_lib \
+	analyze tools tools_lib check-headers \
 	blackbox_crash_test_with_atomic_flush whitebox_crash_test_with_atomic_flush  \
 	blackbox_crash_test_with_txn whitebox_crash_test_with_txn \
 	blackbox_crash_test_with_best_efforts_recovery \
@@ -2368,7 +2391,7 @@ build_subset_tests: $(ROCKSDBTESTS_SUBSET)
 
 # Remove the rules for which dependencies should not be generated and see if any are left.
 #If so, include the dependencies; if not, do not include the dependency files
-ROCKS_DEP_RULES=$(filter-out clean format check-format check-buck-targets check-sources jclean jtest package analyze tags rocksdbjavastatic% unity.% unity_test, $(MAKECMDGOALS))
+ROCKS_DEP_RULES=$(filter-out clean format check-format check-buck-targets check-headers check-sources jclean jtest package analyze tags rocksdbjavastatic% unity.% unity_test, $(MAKECMDGOALS))
 ifneq ("$(ROCKS_DEP_RULES)", "")
 -include $(DEPFILES)
 endif

cache/cache_entry_stats.h

@@ -15,6 +15,7 @@
 #include "rocksdb/cache.h"
 #include "rocksdb/status.h"
 #include "rocksdb/system_clock.h"
+#include "test_util/sync_point.h"
 #include "util/coding_lean.h"
 
 namespace ROCKSDB_NAMESPACE {

db/compaction/compaction_iteration_stats.h

@@ -5,6 +5,8 @@
 
 #pragma once
 
+#include <cstdint>
+
 #include "rocksdb/rocksdb_namespace.h"
 
 struct CompactionIterationStats {

db/job_context.h

@@ -12,8 +12,9 @@
 #include <string>
 #include <vector>
 
-#include "db/log_writer.h"
 #include "db/column_family.h"
+#include "db/log_writer.h"
+#include "db/version_set.h"
 
 namespace ROCKSDB_NAMESPACE {
 

db/pre_release_callback.h

@@ -6,11 +6,10 @@
 #pragma once
 
 #include "rocksdb/status.h"
+#include "rocksdb/types.h"
 
 namespace ROCKSDB_NAMESPACE {
 
-class DB;
-
 class PreReleaseCallback {
  public:
   virtual ~PreReleaseCallback() {}

db/read_callback.h

@@ -5,6 +5,7 @@
 
 #pragma once
 
+#include "db/dbformat.h"
 #include "rocksdb/types.h"
 
 namespace ROCKSDB_NAMESPACE {

db/snapshot_impl.h

@@ -10,6 +10,7 @@
 #pragma once
 #include <vector>
 
+#include "db/dbformat.h"
 #include "rocksdb/db.h"
 
 namespace ROCKSDB_NAMESPACE {

db_stress_tool/db_stress_compaction_filter.h

@@ -9,6 +9,8 @@
 
 #pragma once
 
+#include "db_stress_tool/db_stress_common.h"
+#include "db_stress_tool/db_stress_shared_state.h"
 #include "rocksdb/compaction_filter.h"
 
 namespace ROCKSDB_NAMESPACE {

db_stress_tool/db_stress_listener.h

@@ -3,11 +3,14 @@
 //  COPYING file in the root directory) and Apache 2.0 License
 //  (found in the LICENSE.Apache file in the root directory).
 
+#include "file/filename.h"
 #ifdef GFLAGS
 #pragma once
 
+#include "rocksdb/db.h"
 #include "rocksdb/listener.h"
 #include "util/gflags_compat.h"
+#include "util/random.h"
 
 DECLARE_int32(compact_files_one_in);
 

db_stress_tool/db_stress_table_properties_collector.h

@@ -7,6 +7,7 @@
 
 #include "rocksdb/table.h"
 #include "util/gflags_compat.h"
+#include "util/random.h"
 
 DECLARE_int32(mark_for_compaction_one_file_in);
 

include/rocksdb/functor_wrapper.h

@@ -5,6 +5,7 @@
 
 #pragma once
 
+#include <functional>
 #include <memory>
 #include <utility>
 

include/rocksdb/rocksdb_namespace.h

@@ -5,6 +5,12 @@
 
 #pragma once
 
+// For testing purposes
+#if ROCKSDB_NAMESPACE == 42
+#undef ROCKSDB_NAMESPACE
+#endif
+
+// Normal logic
 #ifndef ROCKSDB_NAMESPACE
 #define ROCKSDB_NAMESPACE rocksdb
 #endif

include/rocksdb/thread_status.h

@@ -13,13 +13,15 @@
 
 #pragma once
 
-#include <stdint.h>
 #include <cstddef>
+#include <cstdint>
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
 
+#include "rocksdb/rocksdb_namespace.h"
+
 #if !defined(ROCKSDB_LITE) && !defined(NROCKSDB_THREAD_STATUS) && \
     defined(ROCKSDB_SUPPORT_THREAD_LOCAL)
 #define ROCKSDB_USING_THREAD_STATUS

include/rocksdb/universal_compaction.h

@@ -5,10 +5,12 @@
 
 #pragma once
 
-#include <stdint.h>
 #include <climits>
+#include <cstdint>
 #include <vector>
 
+#include "rocksdb/rocksdb_namespace.h"
+
 namespace ROCKSDB_NAMESPACE {
 
 //

include/rocksdb/utilities/ldb_cmd_execute_result.h

@@ -5,6 +5,10 @@
 //
 #pragma once
 
+#include <string>
+
+#include "rocksdb/rocksdb_namespace.h"
+
 #ifdef FAILED
 #undef FAILED
 #endif

memory/memory_usage.h

@@ -5,8 +5,11 @@
 
 #pragma once
 
+#include <cstddef>
 #include <unordered_map>
 
+#include "rocksdb/rocksdb_namespace.h"
+
 namespace ROCKSDB_NAMESPACE {
 
 // Helper methods to estimate memroy usage by std containers.

table/block_based/block_type.h

@@ -7,6 +7,8 @@
 
 #include <cstdint>
 
+#include "rocksdb/rocksdb_namespace.h"
+
 namespace ROCKSDB_NAMESPACE {
 
 // Represents the types of blocks used in the block based table format.

table/table_properties_internal.h

@@ -6,7 +6,7 @@
 #pragma once
 
 #include "rocksdb/status.h"
-#include "rocksdb/iterator.h"
+#include "table/internal_iterator.h"
 
 namespace ROCKSDB_NAMESPACE {
 

table/table_reader_caller.h

@@ -5,6 +5,8 @@
 
 #pragma once
 
+#include "rocksdb/rocksdb_namespace.h"
+
 namespace ROCKSDB_NAMESPACE {
 // A list of callers for a table reader. It is used to trace the caller that
 // accesses on a block. This is only used for block cache tracing and analysis.

util/cast_util.h

@@ -4,6 +4,9 @@
 //  (found in the LICENSE.Apache file in the root directory).
 
 #pragma once
+
+#include "rocksdb/rocksdb_namespace.h"
+
 namespace ROCKSDB_NAMESPACE {
 // The helper function to assert the move from dynamic_cast<> to
 // static_cast<> is correct. This function is to deal with legacy code.

util/channel.h

@@ -10,6 +10,8 @@
 #include <queue>
 #include <utility>
 
+#include "rocksdb/rocksdb_namespace.h"
+
 namespace ROCKSDB_NAMESPACE {
 
 template <class T>

util/crc32c_ppc.h

@@ -7,6 +7,9 @@
 
 #pragma once
 
+#include <cstddef>
+#include <cstdint>
+
 #ifdef __cplusplus
 extern "C" {
 #endif

util/defer.h

@@ -7,6 +7,8 @@
 
 #include <functional>
 
+#include "rocksdb/rocksdb_namespace.h"
+
 namespace ROCKSDB_NAMESPACE {
 
 // Defers the execution of the provided function until the Defer

util/duplicate_detector.h

@@ -5,8 +5,7 @@
 
 #pragma once
 
-#include <cinttypes>
+#include "db/db_impl/db_impl.h"
-
 #include "util/set_comparator.h"
 
 namespace ROCKSDB_NAMESPACE {

util/fastrange.h

@@ -22,6 +22,8 @@
 #include <cstdint>
 #include <type_traits>
 
+#include "rocksdb/rocksdb_namespace.h"
+
 #ifdef TEST_UINT128_COMPAT
 #undef HAVE_UINT128_EXTENSION
 #endif

util/math.h

@@ -13,6 +13,8 @@
 #include <cstdint>
 #include <type_traits>
 
+#include "rocksdb/rocksdb_namespace.h"
+
 namespace ROCKSDB_NAMESPACE {
 
 // Fast implementation of floor(log2(v)). Undefined for 0 or negative

util/set_comparator.h

@@ -5,6 +5,8 @@
 
 #pragma once
 
+#include "rocksdb/comparator.h"
+
 namespace ROCKSDB_NAMESPACE {
 // A comparator to be used in std::set
 struct SetComparator {

util/work_queue.h

@@ -21,6 +21,8 @@
 #include <mutex>
 #include <queue>
 
+#include "rocksdb/rocksdb_namespace.h"
+
 namespace ROCKSDB_NAMESPACE {
 
 /// Unbounded thread-safe work queue.

util/xxph3.h

@@ -51,6 +51,7 @@
 #endif
 #define XXPH_NAMESPACE ROCKSDB_
 #define XXPH_INLINE_ALL
+#include <cstring>
 /* END RocksDB customizations */
 
 #if defined (__cplusplus)

utilities/blob_db/blob_db_gc_stats.h

@@ -5,6 +5,10 @@
 //
 #pragma once
 
+#include <cstdint>
+
+#include "rocksdb/rocksdb_namespace.h"
+
 #ifndef ROCKSDB_LITE
 
 namespace ROCKSDB_NAMESPACE {

utilities/cassandra/cassandra_options.h

@@ -4,7 +4,10 @@
 //  (found in the LICENSE.Apache file in the root directory).
 
 #pragma once
-#include <cinttypes>
+
+#include <cstddef>
+#include <cstdint>
+#include <string>
 
 #include "rocksdb/rocksdb_namespace.h"
 

utilities/cassandra/serialize.h

@@ -10,6 +10,11 @@
 
 #pragma once
 
+#include <cstdint>
+#include <string>
+
+#include "rocksdb/rocksdb_namespace.h"
+
 namespace ROCKSDB_NAMESPACE {
 namespace cassandra {
 namespace {

utilities/trace/file_trace_reader_writer.cc

@@ -84,6 +84,10 @@ Status FileTraceReader::Read(std::string* data) {
   return s;
 }
 
+FileTraceWriter::FileTraceWriter(
+    std::unique_ptr<WritableFileWriter>&& file_writer)
+    : file_writer_(std::move(file_writer)) {}
+
 FileTraceWriter::~FileTraceWriter() { Close().PermitUncheckedError(); }
 
 Status FileTraceWriter::Close() {

utilities/trace/file_trace_reader_writer.h

@@ -34,8 +34,7 @@ class FileTraceReader : public TraceReader {
 // FileTraceWriter allows writing RocksDB traces to a file.
 class FileTraceWriter : public TraceWriter {
  public:
-  explicit FileTraceWriter(std::unique_ptr<WritableFileWriter>&& file_writer)
+  explicit FileTraceWriter(std::unique_ptr<WritableFileWriter>&& file_writer);
-      : file_writer_(std::move(file_writer)) {}
   ~FileTraceWriter();
 
   virtual Status Write(const Slice& data) override;