Fix and detect headers with missing dependencies #8893
Conversation
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. Test Plan: existing tests and resolving issues detected by new check
|
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| # `make check-headers` to very that each header file includes its own | ||
| # dependencies | ||
| ifneq ($(filter check_headers, $(MAKECMDGOALS)),) | ||
| DEV_HEADER_DIRS := $(sort include/ hdfs/ $(dir $(ALL_SOURCES))) |
There was a problem hiding this comment.
Should this also include the JNI_SOURCES, or are those already in "ALL"?
There was a problem hiding this comment.
Not trivial (need more compiler flags), so making it a TODO item
Makefile
Outdated
| ifneq ($(filter check_headers, $(MAKECMDGOALS)),) | ||
| 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/|lua/|range_tree/|tools/rdb/db_wrapper.h|include/rocksdb/utilities/env_librados.h') |
There was a problem hiding this comment.
third-party and plugins should also be excluded.
Looking at this list has me thinking that some of the source is in the wrong and some of the code should be moved around...
There was a problem hiding this comment.
plugins I agree, but if third-party is already clean, it doesn't hurt to try to keep it that way.
Makefile
Outdated
| @@ -716,7 +738,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 \ | |||
There was a problem hiding this comment.
Please also add "check_headers" to the ROCKSDB_DEP_RULES exclusion list
| @@ -10,6 +10,7 @@ | |||
| #pragma once | |||
| #include <vector> | |||
|
|
|||
| #include "db/dbformat.h" | |||
There was a problem hiding this comment.
Shouldn't this in snapshot.h, not dbformat?
There was a problem hiding this comment.
kMaxSequenceNumber is in dbformat.h
There was a problem hiding this comment.
The includes in this file are overkill. It should include snapshot, types, and dbformat. No need to include db.h
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@pdillinger merged this pull request in bda8d93. |
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: facebook/rocksdb#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
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-headerswhich validates that each header canbe 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 checkbut is added toCircleCI build-linux-unity since that one is already relatively fast.
Test Plan: existing tests and resolving issues detected by new check