Add (& fix) some simple source code checks (#8821)

Summary:
* Don't hardcode namespace rocksdb (use ROCKSDB_NAMESPACE)
* Don't #include <rocksdb/...> (use double quotes)
* Support putting NOCOMMIT (any case) in source code that should not be
committed/pushed in current state.

These will be run with `make check` and in GitHub actions

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

Test Plan: existing tests, manually try out new checks

Reviewed By: zhichao-cao

Differential Revision: D30791726

Pulled By: pdillinger

fbshipit-source-id: 399c883f312be24d9e55c58951d4013e18429d92

.github/workflows/sanity_check.yml

@@ -39,3 +39,6 @@ jobs:
 
     - name: Compare buckify output
       run: make check-buck-targets
+
+    - name: Simple source code checks
+      run: make check-sources

Makefile

@@ -943,6 +943,7 @@ endif
 ifndef SKIP_FORMAT_BUCK_CHECKS
 	$(MAKE) check-format
 	$(MAKE) check-buck-targets
+	$(MAKE) check-sources
 endif
 
 # TODO add ldb_tests
@@ -1220,6 +1221,9 @@ check-format:
 check-buck-targets:
 	buckifier/check_buck_targets.sh
 
+check-sources:
+	build_tools/check-sources.sh
+
 package:
 	bash build_tools/make_package.sh $(SHARED_MAJOR).$(SHARED_MINOR)
 
@@ -1897,7 +1901,7 @@ clipping_iterator_test: $(OBJ_DIR)/db/compaction/clipping_iterator_test.o $(TEST
 
 ribbon_bench: $(OBJ_DIR)/microbench/ribbon_bench.o $(LIBRARY)
 	$(AM_LINK)
-	
+
 cache_reservation_manager_test: $(OBJ_DIR)/cache/cache_reservation_manager_test.o $(TEST_LIBRARY) $(LIBRARY)
 	$(AM_LINK)
 #-------------------------------------------------
@@ -2364,7 +2368,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 jclean jtest package analyze tags rocksdbjavastatic% unity.% unity_test, $(MAKECMDGOALS))
+ROCKS_DEP_RULES=$(filter-out clean format check-format check-buck-targets check-sources jclean jtest package analyze tags rocksdbjavastatic% unity.% unity_test, $(MAKECMDGOALS))
 ifneq ("$(ROCKS_DEP_RULES)", "")
 -include $(DEPFILES)
 endif

build_tools/check-sources.sh

@@ -0,0 +1,28 @@
+#!/usr/bin/env bash
+# Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
+#
+# Check for some simple mistakes that should prevent commit or push
+
+BAD=""
+
+git grep 'namespace rocksdb' -- '*.[ch]*'
+if [ "$?" != "1" ]; then
+  echo "^^^^^ Do not hardcode namespace rocksdb. Use ROCKSDB_NAMESPACE"
+  BAD=1
+fi
+
+git grep -i 'nocommit' -- ':!build_tools/check-sources.sh'
+if [ "$?" != "1" ]; then
+  echo "^^^^^ Code was not intended to be committed"
+  BAD=1
+fi
+
+git grep '<rocksdb/' -- ':!build_tools/check-sources.sh'
+if [ "$?" != "1" ]; then
+  echo '^^^^^ Use double-quotes as in #include "rocksdb/something.h"'
+  BAD=1
+fi
+
+if [ "$BAD" ]; then
+  exit 1
+fi

cache/cache_bench.cc

@@ -13,7 +13,7 @@ int main() {
   return 1;
 }
 #else
-#include <rocksdb/cache_bench_tool.h>
+#include "rocksdb/cache_bench_tool.h"
 int main(int argc, char** argv) {
   return ROCKSDB_NAMESPACE::cache_bench_tool(argc, argv);
 }

db_stress_tool/db_stress.cc

@@ -15,7 +15,7 @@ int main() {
   return 1;
 }
 #else
-#include <rocksdb/db_stress_tool.h>
+#include "rocksdb/db_stress_tool.h"
 
 int main(int argc, char** argv) {
   return ROCKSDB_NAMESPACE::db_stress_tool(argc, argv);

examples/compaction_filter_example.cc

@@ -3,10 +3,10 @@
 //  COPYING file in the root directory) and Apache 2.0 License
 //  (found in the LICENSE.Apache file in the root directory).
 
-#include <rocksdb/compaction_filter.h>
+#include "rocksdb/compaction_filter.h"
-#include <rocksdb/db.h>
+#include "rocksdb/db.h"
-#include <rocksdb/merge_operator.h>
+#include "rocksdb/merge_operator.h"
-#include <rocksdb/options.h>
+#include "rocksdb/options.h"
 
 class MyMerge : public ROCKSDB_NAMESPACE::MergeOperator {
  public:

include/rocksdb/memtablerep.h

@@ -35,7 +35,6 @@
 
 #pragma once
 
-#include <rocksdb/slice.h>
 #include <stdint.h>
 #include <stdlib.h>
 
@@ -43,6 +42,8 @@
 #include <stdexcept>
 #include <unordered_set>
 
+#include "rocksdb/slice.h"
+
 namespace ROCKSDB_NAMESPACE {
 
 class Arena;

include/rocksdb/system_clock.h

@@ -7,12 +7,13 @@
 // found in the LICENSE file. See the AUTHORS file for names of contributors.
 
 #pragma once
-#include <rocksdb/rocksdb_namespace.h>
-#include <rocksdb/status.h>
 #include <stdint.h>
 
 #include <memory>
 
+#include "rocksdb/rocksdb_namespace.h"
+#include "rocksdb/status.h"
+
 #ifdef _WIN32
 // Windows API macro interference
 #undef GetCurrentTime

java/rocksjni/event_listener_jnicallback.cc

@@ -10,7 +10,7 @@
 
 #include "rocksjni/portal.h"
 
-namespace rocksdb {
+namespace ROCKSDB_NAMESPACE {
 EventListenerJniCallback::EventListenerJniCallback(
     JNIEnv* env, jobject jevent_listener,
     const std::set<EnabledEventCallback>& enabled_event_callbacks)
@@ -499,4 +499,4 @@ void EventListenerJniCallback::OnFileOperation(const jmethodID& mid,
 
   CleanupCallbackInvocation(env, attached_thread, {&jop_info});
 }
-}  // namespace rocksdb
+}  // namespace ROCKSDB_NAMESPACE

java/rocksjni/event_listener_jnicallback.h

@@ -17,7 +17,7 @@
 #include "rocksdb/listener.h"
 #include "rocksjni/jnicallback.h"
 
-namespace rocksdb {
+namespace ROCKSDB_NAMESPACE {
 
 enum EnabledEventCallback {
   ON_FLUSH_COMPLETED = 0x0,
@@ -117,6 +117,6 @@ class EventListenerJniCallback : public JniCallback, public EventListener {
   jmethodID m_on_error_recovery_completed_mid;
 };
 
-}  // namespace rocksdb
+}  // namespace ROCKSDB_NAMESPACE
 
 #endif  // JAVA_ROCKSJNI_EVENT_LISTENER_JNICALLBACK_H_

memory/memkind_kmem_allocator.cc

@@ -8,7 +8,7 @@
 
 #include "memkind_kmem_allocator.h"
 
-namespace rocksdb {
+namespace ROCKSDB_NAMESPACE {
 
 void* MemkindKmemAllocator::Allocate(size_t size) {
   void* p = memkind_malloc(MEMKIND_DAX_KMEM, size);
@@ -29,5 +29,5 @@ size_t MemkindKmemAllocator::UsableSize(void* p,
 }
 #endif  // ROCKSDB_MALLOC_USABLE_SIZE
 
-}  // namespace rocksdb
+}  // namespace ROCKSDB_NAMESPACE
 #endif  // MEMKIND

memory/memkind_kmem_allocator.h

@@ -11,7 +11,7 @@
 #include <memkind.h>
 #include "rocksdb/memory_allocator.h"
 
-namespace rocksdb {
+namespace ROCKSDB_NAMESPACE {
 
 class MemkindKmemAllocator : public MemoryAllocator {
  public:
@@ -23,5 +23,5 @@ class MemkindKmemAllocator : public MemoryAllocator {
 #endif
 };
 
-}  // namespace rocksdb
+}  // namespace ROCKSDB_NAMESPACE
 #endif  // MEMKIND

memory/memkind_kmem_allocator_test.cc

@@ -14,7 +14,7 @@
 #include "table/block_based/block_based_table_factory.h"
 #include "test_util/testharness.h"
 
-namespace rocksdb {
+namespace ROCKSDB_NAMESPACE {
 TEST(MemkindKmemAllocatorTest, Allocate) {
   MemkindKmemAllocator allocator;
   void* p;
@@ -84,7 +84,7 @@ TEST(MemkindKmemAllocatorTest, DatabaseBlockCache) {
   ASSERT_OK(s);
   ASSERT_OK(DestroyDB(dbname, options));
 }
-}  // namespace rocksdb
+}  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {
   ::testing::InitGoogleTest(&argc, argv);

table/block_based/index_builder.cc

@@ -20,9 +20,8 @@
 #include "table/block_based/partitioned_filter_block.h"
 #include "table/format.h"
 
-// Without anonymous namespace here, we fail the warning -Wmissing-prototypes
 namespace ROCKSDB_NAMESPACE {
-// using namespace rocksdb;
+
 // Create a index builder based on its type.
 IndexBuilder* IndexBuilder::CreateIndexBuilder(
     BlockBasedTableOptions::IndexType index_type,

test_util/testharness.h

@@ -14,7 +14,7 @@
 #else
 #include <gtest/gtest.h>
 #endif
-#include <rocksdb/utilities/regex.h>
+#include "rocksdb/utilities/regex.h"
 
 // A "skipped" test has a specific meaning in Facebook infrastructure: the
 // test is in good shape and should be run, but something about the

tools/db_bench.cc

@@ -14,7 +14,7 @@ int main() {
   return 1;
 }
 #else
-#include <rocksdb/db_bench_tool.h>
+#include "rocksdb/db_bench_tool.h"
 int main(int argc, char** argv) {
   return ROCKSDB_NAMESPACE::db_bench_tool(argc, argv);
 }