From bda8d93ba95f34aa91ae8578fe7d3454f3324f6a Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Fri, 10 Sep 2021 09:59:05 -0700 Subject: [PATCH] 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 | 7 ++--- Makefile | 27 +++++++++++++++++-- cache/cache_entry_stats.h | 1 + db/compaction/compaction_iteration_stats.h | 2 ++ db/job_context.h | 3 ++- db/pre_release_callback.h | 3 +-- db/read_callback.h | 1 + db/snapshot_impl.h | 1 + db_stress_tool/db_stress_compaction_filter.h | 2 ++ db_stress_tool/db_stress_listener.h | 3 +++ .../db_stress_table_properties_collector.h | 1 + include/rocksdb/functor_wrapper.h | 1 + include/rocksdb/rocksdb_namespace.h | 6 +++++ include/rocksdb/thread_status.h | 4 ++- include/rocksdb/universal_compaction.h | 4 ++- .../utilities/ldb_cmd_execute_result.h | 4 +++ memory/memory_usage.h | 3 +++ table/block_based/block_type.h | 2 ++ table/table_properties_internal.h | 2 +- table/table_reader_caller.h | 2 ++ util/cast_util.h | 3 +++ util/channel.h | 2 ++ util/crc32c_ppc.h | 3 +++ util/defer.h | 2 ++ util/duplicate_detector.h | 3 +-- util/fastrange.h | 2 ++ util/math.h | 2 ++ util/set_comparator.h | 2 ++ util/work_queue.h | 2 ++ util/xxph3.h | 1 + utilities/blob_db/blob_db_gc_stats.h | 4 +++ utilities/cassandra/cassandra_options.h | 5 +++- utilities/cassandra/serialize.h | 5 ++++ utilities/trace/file_trace_reader_writer.cc | 4 +++ utilities/trace/file_trace_reader_writer.h | 3 +-- 35 files changed, 106 insertions(+), 16 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 779a8ca07..b39532496 100644 --- a/.circleci/config.yml +++ b/.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: diff --git a/Makefile b/Makefile index b37e43e54..274864378 100644 --- a/Makefile +++ b/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 diff --git a/cache/cache_entry_stats.h b/cache/cache_entry_stats.h index eb028a087..cc452053d 100644 --- a/cache/cache_entry_stats.h +++ b/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 { diff --git a/db/compaction/compaction_iteration_stats.h b/db/compaction/compaction_iteration_stats.h index 894aea1d9..910c4469a 100644 --- a/db/compaction/compaction_iteration_stats.h +++ b/db/compaction/compaction_iteration_stats.h @@ -5,6 +5,8 @@ #pragma once +#include + #include "rocksdb/rocksdb_namespace.h" struct CompactionIterationStats { diff --git a/db/job_context.h b/db/job_context.h index 4b0a8edfc..8e191e31c 100644 --- a/db/job_context.h +++ b/db/job_context.h @@ -12,8 +12,9 @@ #include #include -#include "db/log_writer.h" #include "db/column_family.h" +#include "db/log_writer.h" +#include "db/version_set.h" namespace ROCKSDB_NAMESPACE { diff --git a/db/pre_release_callback.h b/db/pre_release_callback.h index b74be9537..6b9039487 100644 --- a/db/pre_release_callback.h +++ b/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() {} diff --git a/db/read_callback.h b/db/read_callback.h index 8989c755a..c042352db 100644 --- a/db/read_callback.h +++ b/db/read_callback.h @@ -5,6 +5,7 @@ #pragma once +#include "db/dbformat.h" #include "rocksdb/types.h" namespace ROCKSDB_NAMESPACE { diff --git a/db/snapshot_impl.h b/db/snapshot_impl.h index bfa44e3f5..9961bdd58 100644 --- a/db/snapshot_impl.h +++ b/db/snapshot_impl.h @@ -10,6 +10,7 @@ #pragma once #include +#include "db/dbformat.h" #include "rocksdb/db.h" namespace ROCKSDB_NAMESPACE { diff --git a/db_stress_tool/db_stress_compaction_filter.h b/db_stress_tool/db_stress_compaction_filter.h index 60f8ac71d..279c4452a 100644 --- a/db_stress_tool/db_stress_compaction_filter.h +++ b/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 { diff --git a/db_stress_tool/db_stress_listener.h b/db_stress_tool/db_stress_listener.h index 6985a7742..7fbab57e7 100644 --- a/db_stress_tool/db_stress_listener.h +++ b/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); diff --git a/db_stress_tool/db_stress_table_properties_collector.h b/db_stress_tool/db_stress_table_properties_collector.h index 536d8a5f3..d1758cbb4 100644 --- a/db_stress_tool/db_stress_table_properties_collector.h +++ b/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); diff --git a/include/rocksdb/functor_wrapper.h b/include/rocksdb/functor_wrapper.h index c5f7414b1..17b021bf7 100644 --- a/include/rocksdb/functor_wrapper.h +++ b/include/rocksdb/functor_wrapper.h @@ -5,6 +5,7 @@ #pragma once +#include #include #include diff --git a/include/rocksdb/rocksdb_namespace.h b/include/rocksdb/rocksdb_namespace.h index e9f8620d0..a339ec2aa 100644 --- a/include/rocksdb/rocksdb_namespace.h +++ b/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 diff --git a/include/rocksdb/thread_status.h b/include/rocksdb/thread_status.h index 6b2f5c885..bb2de6c7b 100644 --- a/include/rocksdb/thread_status.h +++ b/include/rocksdb/thread_status.h @@ -13,13 +13,15 @@ #pragma once -#include #include +#include #include #include #include #include +#include "rocksdb/rocksdb_namespace.h" + #if !defined(ROCKSDB_LITE) && !defined(NROCKSDB_THREAD_STATUS) && \ defined(ROCKSDB_SUPPORT_THREAD_LOCAL) #define ROCKSDB_USING_THREAD_STATUS diff --git a/include/rocksdb/universal_compaction.h b/include/rocksdb/universal_compaction.h index f4df5c000..9a09d8f4e 100644 --- a/include/rocksdb/universal_compaction.h +++ b/include/rocksdb/universal_compaction.h @@ -5,10 +5,12 @@ #pragma once -#include #include +#include #include +#include "rocksdb/rocksdb_namespace.h" + namespace ROCKSDB_NAMESPACE { // diff --git a/include/rocksdb/utilities/ldb_cmd_execute_result.h b/include/rocksdb/utilities/ldb_cmd_execute_result.h index c837b47f7..57bac3346 100644 --- a/include/rocksdb/utilities/ldb_cmd_execute_result.h +++ b/include/rocksdb/utilities/ldb_cmd_execute_result.h @@ -5,6 +5,10 @@ // #pragma once +#include + +#include "rocksdb/rocksdb_namespace.h" + #ifdef FAILED #undef FAILED #endif diff --git a/memory/memory_usage.h b/memory/memory_usage.h index 9061d45eb..7f1de8c97 100644 --- a/memory/memory_usage.h +++ b/memory/memory_usage.h @@ -5,8 +5,11 @@ #pragma once +#include #include +#include "rocksdb/rocksdb_namespace.h" + namespace ROCKSDB_NAMESPACE { // Helper methods to estimate memroy usage by std containers. diff --git a/table/block_based/block_type.h b/table/block_based/block_type.h index b2a913746..0ca899f20 100644 --- a/table/block_based/block_type.h +++ b/table/block_based/block_type.h @@ -7,6 +7,8 @@ #include +#include "rocksdb/rocksdb_namespace.h" + namespace ROCKSDB_NAMESPACE { // Represents the types of blocks used in the block based table format. diff --git a/table/table_properties_internal.h b/table/table_properties_internal.h index a7a92e3e1..171192434 100644 --- a/table/table_properties_internal.h +++ b/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 { diff --git a/table/table_reader_caller.h b/table/table_reader_caller.h index 7a57b5e98..10ec08130 100644 --- a/table/table_reader_caller.h +++ b/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. diff --git a/util/cast_util.h b/util/cast_util.h index d84516fba..c7f515eeb 100644 --- a/util/cast_util.h +++ b/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. diff --git a/util/channel.h b/util/channel.h index 705fa2d28..19b956297 100644 --- a/util/channel.h +++ b/util/channel.h @@ -10,6 +10,8 @@ #include #include +#include "rocksdb/rocksdb_namespace.h" + namespace ROCKSDB_NAMESPACE { template diff --git a/util/crc32c_ppc.h b/util/crc32c_ppc.h index 15ed6fae5..f0b0b66d5 100644 --- a/util/crc32c_ppc.h +++ b/util/crc32c_ppc.h @@ -7,6 +7,9 @@ #pragma once +#include +#include + #ifdef __cplusplus extern "C" { #endif diff --git a/util/defer.h b/util/defer.h index a78193173..33c0243e6 100644 --- a/util/defer.h +++ b/util/defer.h @@ -7,6 +7,8 @@ #include +#include "rocksdb/rocksdb_namespace.h" + namespace ROCKSDB_NAMESPACE { // Defers the execution of the provided function until the Defer diff --git a/util/duplicate_detector.h b/util/duplicate_detector.h index 72920ca3c..ce1f351db 100644 --- a/util/duplicate_detector.h +++ b/util/duplicate_detector.h @@ -5,8 +5,7 @@ #pragma once -#include - +#include "db/db_impl/db_impl.h" #include "util/set_comparator.h" namespace ROCKSDB_NAMESPACE { diff --git a/util/fastrange.h b/util/fastrange.h index 871fa830e..a70a980f6 100644 --- a/util/fastrange.h +++ b/util/fastrange.h @@ -22,6 +22,8 @@ #include #include +#include "rocksdb/rocksdb_namespace.h" + #ifdef TEST_UINT128_COMPAT #undef HAVE_UINT128_EXTENSION #endif diff --git a/util/math.h b/util/math.h index 795efb018..dad539c41 100644 --- a/util/math.h +++ b/util/math.h @@ -13,6 +13,8 @@ #include #include +#include "rocksdb/rocksdb_namespace.h" + namespace ROCKSDB_NAMESPACE { // Fast implementation of floor(log2(v)). Undefined for 0 or negative diff --git a/util/set_comparator.h b/util/set_comparator.h index 9b5cfc1dc..e0e64436a 100644 --- a/util/set_comparator.h +++ b/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 { diff --git a/util/work_queue.h b/util/work_queue.h index f120ca77c..94ece85d9 100644 --- a/util/work_queue.h +++ b/util/work_queue.h @@ -21,6 +21,8 @@ #include #include +#include "rocksdb/rocksdb_namespace.h" + namespace ROCKSDB_NAMESPACE { /// Unbounded thread-safe work queue. diff --git a/util/xxph3.h b/util/xxph3.h index c7a9c799d..12405abe0 100644 --- a/util/xxph3.h +++ b/util/xxph3.h @@ -51,6 +51,7 @@ #endif #define XXPH_NAMESPACE ROCKSDB_ #define XXPH_INLINE_ALL +#include /* END RocksDB customizations */ #if defined (__cplusplus) diff --git a/utilities/blob_db/blob_db_gc_stats.h b/utilities/blob_db/blob_db_gc_stats.h index 1e6e4a25d..fea6b0032 100644 --- a/utilities/blob_db/blob_db_gc_stats.h +++ b/utilities/blob_db/blob_db_gc_stats.h @@ -5,6 +5,10 @@ // #pragma once +#include + +#include "rocksdb/rocksdb_namespace.h" + #ifndef ROCKSDB_LITE namespace ROCKSDB_NAMESPACE { diff --git a/utilities/cassandra/cassandra_options.h b/utilities/cassandra/cassandra_options.h index 8cf756a98..efa73a308 100644 --- a/utilities/cassandra/cassandra_options.h +++ b/utilities/cassandra/cassandra_options.h @@ -4,7 +4,10 @@ // (found in the LICENSE.Apache file in the root directory). #pragma once -#include + +#include +#include +#include #include "rocksdb/rocksdb_namespace.h" diff --git a/utilities/cassandra/serialize.h b/utilities/cassandra/serialize.h index cd980ade0..8f50a02dd 100644 --- a/utilities/cassandra/serialize.h +++ b/utilities/cassandra/serialize.h @@ -10,6 +10,11 @@ #pragma once +#include +#include + +#include "rocksdb/rocksdb_namespace.h" + namespace ROCKSDB_NAMESPACE { namespace cassandra { namespace { diff --git a/utilities/trace/file_trace_reader_writer.cc b/utilities/trace/file_trace_reader_writer.cc index dc58ded21..f2ca74144 100644 --- a/utilities/trace/file_trace_reader_writer.cc +++ b/utilities/trace/file_trace_reader_writer.cc @@ -84,6 +84,10 @@ Status FileTraceReader::Read(std::string* data) { return s; } +FileTraceWriter::FileTraceWriter( + std::unique_ptr&& file_writer) + : file_writer_(std::move(file_writer)) {} + FileTraceWriter::~FileTraceWriter() { Close().PermitUncheckedError(); } Status FileTraceWriter::Close() { diff --git a/utilities/trace/file_trace_reader_writer.h b/utilities/trace/file_trace_reader_writer.h index 909317fe4..65d483108 100644 --- a/utilities/trace/file_trace_reader_writer.h +++ b/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&& file_writer) - : file_writer_(std::move(file_writer)) {} + explicit FileTraceWriter(std::unique_ptr&& file_writer); ~FileTraceWriter(); virtual Status Write(const Slice& data) override;