From 0afed66ccf88235aeab0f0a51b68a5151bd311e9 Mon Sep 17 00:00:00 2001
From: Uli Schlachter <psychon@znc.in>
Date: Mon, 6 Feb 2017 18:35:20 +0100
Subject: [PATCH] Add a utility checking uses of require()s

Currently, "everything can require everything". It's an unstructured
mess which sometimes causes problems.

This commit adds a tool that enforces a white-list of require() uses. It
uses depgraph to scan the source code and then each use of require()
that is found is checked. If any violations are found, the tool returns
a failure.

This tool is wired up to a new target "make check-requires" which is
included in "make check". Thus, Travis will run this.

Reference: https://github.com/awesomeWM/awesome/issues/1400
Signed-off-by: Uli Schlachter <psychon@znc.in>
---
 .travis.yml                                |   2 +
 CMakeLists.txt                             |   7 ++
 build-utils/check_for_invalid_requires.lua | 119 +++++++++++++++++++++
 3 files changed, 128 insertions(+)
 create mode 100755 build-utils/check_for_invalid_requires.lua

diff --git a/.travis.yml b/.travis.yml
index f853c3cd2..e0442ed8a 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -88,6 +88,8 @@ install:
   - travis_retry sudo luarocks install busted
   # Install luacheck for "make check-qa".
   - if [ "$DO_CHECKQA" = 1 ]; then travis_retry sudo luarocks install luacheck; fi
+  # Install depgraph for "make check-qa".
+  - if [ "$DO_CHECKQA" = 1 ]; then travis_retry sudo luarocks install depgraph; fi
 
   # Install ldoc for building docs.
   - |
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 5fd05c9d8..f963539a1 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -381,6 +381,13 @@ add_custom_target(check-integration
     COMMENT "Running integration tests"
     USES_TERMINAL
     VERBATIM)
+add_custom_target(check-requires
+    lua "${CMAKE_SOURCE_DIR}/build-utils/check_for_invalid_requires.lua"
+    WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
+    COMMENT "Checking use of require()"
+    USES_TERMINAL
+    VERBATIM)
+list(APPEND CHECK_QA_TARGETS check-requires)
 a_find_program(BUSTED_EXECUTABLE busted FALSE)
 if(BUSTED_EXECUTABLE)
     # Keep the arguments in sync with the version below!
diff --git a/build-utils/check_for_invalid_requires.lua b/build-utils/check_for_invalid_requires.lua
new file mode 100755
index 000000000..a5ec5b19b
--- /dev/null
+++ b/build-utils/check_for_invalid_requires.lua
@@ -0,0 +1,119 @@
+#!/usr/bin/env lua
+
+-- This uses the "depgraph" package to parse all source code and look for calls
+-- to require(). For all these uses, we make sure that it is allowed by the
+-- white-list below. This is done to mitigate the risk of cyclic dependencies
+-- between modules and to (hopefully) enforce a bit of sane code structure.
+
+local have_depgraph, depgraph = pcall(require, "depgraph")
+
+if not have_depgraph then
+    print("depgraph not found")
+    print(depgraph)
+    print("(this error is non-fatal; we just skip its use)")
+    os.exit(0)
+end
+
+local allowed_deps = {
+    gears = {
+        lgi = true,
+    },
+    beautiful = {
+        gears = true,
+        lgi = true,
+    },
+    wibox = {
+        beautiful = true,
+        gears = true,
+        lgi = true,
+    },
+    awful = {
+        beautiful = true,
+        gears = true,
+        lgi = true,
+        wibox = true,
+    },
+    naughty = {
+        awful = true,
+        beautiful = true,
+        gears = true,
+        lgi = true,
+        wibox = true,
+    },
+    menubar = {
+        awful = true,
+        beautiful = true,
+        gears = true,
+        lgi = true,
+        wibox = true,
+    },
+    -- TODO: Get rid of these
+    ["beautiful.xresources"] = { ["awful.util"] = true },
+    ["gears.object"]         = { ["awful.util"] = true },
+    ["wibox.container"]      = { ["awful.util"] = true },
+    ["wibox.layout"]         = { ["awful.util"] = true },
+    ["wibox.widget"]         = { ["awful.util"] = true },
+    ["gears.surface"]        = { ["wibox.hierarchy"] = true },
+}
+
+-- Turn "foo.bar.baz" into "foo.bar". Returns nil if there is nothing more to
+-- remove.
+local function get_supermodule(module)
+    return string.match(module, "(.+)%.%a+")
+end
+
+-- Check if "module" (or one of its parents) is allowed to depend on
+-- "dependency" (or one of its parents).
+local function is_allowed(module, dependency)
+    assert(module ~= nil)
+    -- If both have a common parent, the dependency is allowed
+    do
+        local last_mod, mod = module, module
+        while mod do
+            last_mod, mod = mod, get_supermodule(mod)
+        end
+        local last_dep, dep = dependency, dependency
+        while dep do
+            last_dep, dep = dep, get_supermodule(dep)
+        end
+        if last_mod == last_dep then
+            return true
+        end
+    end
+    -- Check the allowed_deps table
+    while module ~= nil do
+        local allowed = allowed_deps[module]
+        if allowed then
+            local dep = dependency
+            while dep ~= nil do
+                if allowed[dep] then
+                    return true
+                end
+                dep = get_supermodule(dep)
+            end
+        end
+        module = get_supermodule(module)
+    end
+    return false
+end
+
+-- Generate the dependency graph
+local graph = assert(depgraph.make_graph({"lib"}, {}, "lib", nil, nil))
+
+-- Check if all require's are allowed by the above table
+local had_violation = false
+for _, module in ipairs(graph.modules) do
+    for _, dep in ipairs(module.deps) do
+        if not is_allowed(module.name, dep.name) then
+            had_violation = true
+            print(string.format("Dependency from %s onto %s is not allowed",
+                    module.name, dep.name))
+        end
+    end
+end
+
+if had_violation then
+    os.exit(1)
+end
+
+-- vim: filetype=lua:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80