Internal load-namespaces tool to check for compilation errors (#80009)

* Internal load-namespaces tool to check for compilation errors

* Ignore load-namespaces in module checks

* lint load-namespaces

* Reject positional args

* Test the new utility

* Guard against nonexistent root.
Make the code more testable.

.clj-kondo/config.edn

@@ -800,7 +800,7 @@
    :name    test-namespaces}
   {:pattern "^(?!.*-test(?:\\..*)?$)(?!.*test-util)(?!^metabase\\.test.*).*$"
    :name    source-namespaces}
-  {:pattern "(?:(?:^metabase\\.cmd.*)|(?:^build.*)|(?:^metabuild-common.*)|(?:^release.*)|(?:^i18n.*)|(?:^lint-migrations-file$))|(?:^mage.*)"
+  {:pattern "(?:(?:^metabase\\.cmd.*)|(?:^build.*)|(?:^metabuild-common.*)|(?:^release.*)|(?:^i18n.*)|(?:^lint-migrations-file$))|(?:^mage.*)|(?:^load-namespaces.*)"
    :name    printable-namespaces}
   {:pattern "^metabase\\.(?:(?:lib)|(?:legacy-mbql))\\..*"
    :name    metabase-lib}

.clj-kondo/src/hooks/clojure/test.clj

@@ -153,6 +153,7 @@
          "build."
          "i18n." ; bin/i18n
          "lint-migrations-file-test"
+         "load-namespaces." ; bin/load-namespaces
          "mage."
          "main-test" ; bin/release-list
          "metabase.deps-edn-test"

.cljfmt.edn

@@ -12,6 +12,7 @@
   "enterprise/backend"
   "bin/build"
   "bin/lint-migrations-file"
+  "bin/load-namespaces"
   "bin/release-list"
   #_"mage/src"
   #_"mage/test"

.github/workflows/backend.yml

@@ -309,7 +309,7 @@ jobs:
         uses: ./.github/actions/prepare-backend
       # TODO -- we should probably also check WITHOUT ee -- do a check with only OSS namespaces on the classpath
       - name: Check namespaces
-        run: clojure -M:ee:drivers:check
+        run: clojure -M:ee:drivers:load-namespaces
 
   be-cljfmt:
     if: ${{ !inputs.skip }}

.github/workflows/build-scripts.yml

@@ -68,3 +68,7 @@ jobs:
       run: clojure -M:test
       working-directory: bin/lint-migrations-file
       timeout-minutes: 15
+    - name: Run load-namespaces tests
+      run: clojure -M:test
+      working-directory: bin/load-namespaces
+      timeout-minutes: 15

.github/workflows/cache-generator.yml

@@ -103,7 +103,7 @@ jobs:
             ":dev:ci:ee:ee-dev:test:cloverage"                  # backend-cloverage.yml
             ":dev:drivers:drivers-dev:test"                     # reset-cloud-test-dataset.yml
             ":dev:drivers:build:build-dev:build-test:ci"        # build-scripts.yml
-            ":ee:drivers:check"                                 # backend.yml (check; git dep athos/clj-check)
+            ":ee:drivers:load-namespaces"                       # backend.yml (load-namespaces)
             ":ee:doc"                                           # docs-generate-base.yml
             ":kondo"                                            # resolve-backport-conflicts.yml (replace-deps)
             ":run:ee"                                           # cross-branch-migration-test

bb.edn

@@ -96,11 +96,12 @@
                                       :description "Files or directories to fix unused requires in"}]]
    :task (task! (fix/fix-files parsed))}
 
-  check
+  load-namespaces
-  {:doc "Checks whether we can compile all source files (and finds circular dependencies)"
+  {:doc "Checks that every shipped namespace loads without :dev dependencies (and finds circular dependencies)"
-   :examples [["./bin/mage check"]]
+   :examples [["./bin/mage load-namespaces"]]
-   :requires [[mage.check]]
+   :requires [[mage.load-namespaces]]
-   :task (task! (mage.check/check (:arguments parsed)))}
+   :arg-schema [:tuple]
+   :task (task! (mage.load-namespaces/load-namespaces))}
 
   codespell
   {:doc "Check for misspellings in the source code."

bin/load-namespaces/deps.edn

@@ -0,0 +1,7 @@
+{:paths ["src"]
+
+ :aliases
+ {:test {:extra-paths ["test" "test-resources/protocol-reload" "test-resources/broken"]
+         :extra-deps  {com.cognitect/test-runner {:git/url "https://github.com/cognitect-labs/test-runner.git"
+                                                  :sha     "209b64504cb3bd3b99ecfec7937b358a879f55c1"}}
+         :main-opts   ["-m" "cognitect.test-runner"]}}}

bin/load-namespaces/src/load_namespaces/core.clj

@@ -0,0 +1,76 @@
+(ns load-namespaces.core
+  "Loads every namespace under the given source paths, with reflection warnings on, so that CI catches
+   namespaces that only fail against a shipped classpath -- one without the `:dev` dependencies. Circular
+   dependencies and compilation errors surface here too, since both make a namespace fail to load."
+  (:require
+   [clojure.java.io :as io]
+   [clojure.string :as str])
+  (:import
+   (java.io File PrintWriter Writer)))
+
+(set! *warn-on-reflection* true)
+
+(defn- loadable-paths
+  "Extension-stripped, `root`-relative paths of every JVM-loadable source file under `root`, which is the
+   form [[clojure.core/load]] wants. `root` must also be a classpath root, or `load` cannot resolve them.
+   `.cljs` is skipped: it cannot be loaded on the JVM."
+  [root]
+  (let [^File root-file (io/file root)]
+    ;; a nonexistent root scans as an empty directory, so a rename would silently drop a whole source tree
+    (when-not (.isDirectory root-file)
+      (throw (ex-info (str "Not a source directory: " root)
+                      {:root root})))
+    (let [strip (inc (count (.getPath root-file)))]
+      (sort
+       (for [^File file (file-seq root-file)
+             :when      (.isFile file)
+             :let       [path (.getPath file)]
+             :when      (or (str/ends-with? path ".clj")
+                            (str/ends-with? path ".cljc"))]
+         (subs path strip (str/last-index-of path ".")))))))
+
+(defn- path->lib
+  "The namespace a source file at `path` is expected to declare."
+  [path]
+  (symbol (-> path (str/replace "/" ".") (str/replace "_" "-"))))
+
+(defn- load-path
+  "Loads `path`, returning the Throwable it threw, or nil if it loaded or was already loaded."
+  [path]
+  (let [lib (path->lib path)]
+    ;; Namespaces this one already required must not be loaded a second time: reloading a namespace
+    ;; redefines its protocols, and every implementation compiled against the previous definition breaks.
+    (when-not (contains? (loaded-libs) lib)
+      (binding [*out* *err*]
+        (println "Loading namespace" lib))
+      (try
+        (binding [*warn-on-reflection* true]
+          ;; leading slash: without it `load` resolves the path against the *calling* namespace's directory
+          (load (str "/" path)))
+        nil
+        (catch Throwable e
+          (binding [*out* *err*]
+            (println "Failed to load" lib))
+          (.printStackTrace e (PrintWriter. ^Writer *err* true))
+          e)))))
+
+(defn- load-namespaces
+  "Loads every namespace under `source-paths`, returning how many of them failed."
+  [source-paths]
+  (->> source-paths
+       (mapcat loadable-paths)
+       (map load-path)
+       (filter some?)
+       count))
+
+(defn -main
+  "Loads every namespace under `source-paths`, or under `src` if none are given, and exits non-zero if any
+   of them failed."
+  [& source-paths]
+  (let [failures (load-namespaces (or (seq source-paths) ["src"]))]
+    (shutdown-agents)
+    (when (pos? failures)
+      (binding [*out* *err*]
+        (println failures "namespace(s) failed to load"))
+      ;; exiting with `failures` would wrap past 255 and could be read as success
+      (System/exit 1))))

bin/load-namespaces/test-resources/broken/broken_fixtures/boom.clj

@@ -0,0 +1,3 @@
+(ns broken-fixtures.boom)
+
+(throw (ex-info "intentional load failure" {}))

bin/load-namespaces/test-resources/discovery/discovery_fixtures/browser_only.cljs

@@ -0,0 +1 @@
+(ns discovery-fixtures.browser-only)

bin/load-namespaces/test-resources/discovery/discovery_fixtures/nested/two_faced.cljc

@@ -0,0 +1 @@
+(ns discovery-fixtures.nested.two-faced)

bin/load-namespaces/test-resources/discovery/discovery_fixtures/plain.clj

@@ -0,0 +1 @@
+(ns discovery-fixtures.plain)

bin/load-namespaces/test-resources/protocol-reload/reload_fixtures/consumer.clj

@@ -0,0 +1,9 @@
+(ns reload-fixtures.consumer
+  (:require
+   [reload-fixtures.proto :as proto]))
+
+(defrecord Friend []
+  proto/Greeter
+  (greet [_this] "hi"))
+
+(def friend (->Friend))

bin/load-namespaces/test-resources/protocol-reload/reload_fixtures/proto.clj

@@ -0,0 +1,4 @@
+(ns reload-fixtures.proto)
+
+(defprotocol Greeter
+  (greet [this]))

bin/load-namespaces/test/load_namespaces/core_test.clj

@@ -0,0 +1,49 @@
+(ns load-namespaces.core-test
+  (:require
+   [clojure.string :as str]
+   [clojure.test :refer [deftest is testing]]
+   [load-namespaces.core :as core])
+  (:import
+   (java.io StringWriter)))
+
+(set! *warn-on-reflection* true)
+
+(defn- greet-the-friend []
+  ((requiring-resolve 'reload-fixtures.proto/greet)
+   @(requiring-resolve 'reload-fixtures.consumer/friend)))
+
+(deftest loadable-paths-test
+  (testing "finds .clj and .cljc, skips .cljs, and strips the root and the extension"
+    (is (= ["discovery_fixtures/nested/two_faced" "discovery_fixtures/plain"]
+           (#'core/loadable-paths "test-resources/discovery")))))
+
+(deftest loadable-paths-rejects-non-directories-test
+  (is (thrown-with-msg? clojure.lang.ExceptionInfo #"Not a source directory"
+                        (#'core/loadable-paths "test-resources/no-such-root")))
+  (is (thrown-with-msg? clojure.lang.ExceptionInfo #"Not a source directory"
+                        (#'core/loadable-paths "deps.edn"))))
+
+(deftest path->lib-test
+  (is (= 'discovery-fixtures.nested.two-faced
+         (#'core/path->lib "discovery_fixtures/nested/two_faced"))))
+
+(deftest load-failures-are-counted-and-reported-test
+  (let [err      (StringWriter.)
+        failures (binding [*err* err] (#'core/load-namespaces ["test-resources/broken"]))
+        output   (str err)]
+    (is (= 1 failures))
+    (is (str/includes? output "Failed to load broken-fixtures.boom"))
+    (is (str/includes? output "intentional load failure"))))
+
+(deftest already-loaded-namespaces-are-not-reloaded-test
+  (let [err      (StringWriter.)
+        failures (binding [*err* err] (#'core/load-namespaces ["test-resources/protocol-reload"]))
+        output   (str err)]
+    (is (zero? failures))
+    (testing "the consumer pulled the protocol in, so the scan left it alone"
+      (is (str/includes? output "Loading namespace reload-fixtures.consumer"))
+      (is (not (str/includes? output "Loading namespace reload-fixtures.proto")))
+      (is (= "hi" (greet-the-friend)))))
+  (testing "reloading it redefines the protocol, breaking implementations compiled against the old one"
+    (load "/reload_fixtures/proto")
+    (is (thrown? IllegalArgumentException (greet-the-friend)))))

deps.edn

@@ -566,6 +566,8 @@
     "bin/build/test"
     "bin/lint-migrations-file/src"
     "bin/lint-migrations-file/test"
+    "bin/load-namespaces/src"
+    "bin/load-namespaces/test"
     "bin/release-list/src"
     "bin/release-list/test"
     "modules/drivers/athena/src"
@@ -599,32 +601,31 @@
     "modules/drivers/vertica/src"
     "modules/drivers/vertica/test"]}
 
-  ;; clojure -M:ee:drivers:check
+  ;; clojure -M:ee:drivers:load-namespaces
   ;;
-  ;; checks that all the namespaces we actually ship can be compiled, without any dependencies that we don't
+  ;; checks that all the namespaces we actually ship can be loaded, without any dependencies that we don't
   ;; ship (such as `:dev` dependencies). See #27009 for more context.
-  :check
+  :load-namespaces
-  {:extra-deps {athos/clj-check {:git/url "https://github.com/athos/clj-check.git"
+  {:extra-paths ["bin/load-namespaces/src"]
-                                 :sha     "d997df866b2a04b7ce7b17533093ee0a2e2cb729"}}
+   :main-opts   ["-m" "load-namespaces.core"
-   :main-opts  ["-m" "clj-check.check"
+                 "src"
-                "src"
+                 "enterprise/backend/src"
-                "enterprise/backend/src"
+                 "modules/drivers/athena/src"
-                "modules/drivers/athena/src"
+                 "modules/drivers/bigquery-cloud-sdk/src"
-                "modules/drivers/bigquery-cloud-sdk/src"
+                 "modules/drivers/clickhouse/src"
-                "modules/drivers/clickhouse/src"
+                 "modules/drivers/databricks/src"
-                "modules/drivers/databricks/src"
+                 "modules/drivers/druid-jdbc/src"
-                "modules/drivers/druid-jdbc/src"
+                 "modules/drivers/hive-like/src"
-                "modules/drivers/hive-like/src"
+                 "modules/drivers/mongo/src"
-                "modules/drivers/mongo/src"
+                 "modules/drivers/oracle/src"
-                "modules/drivers/oracle/src"
+                 "modules/drivers/presto-jdbc/src"
-                "modules/drivers/presto-jdbc/src"
+                 "modules/drivers/redshift/src"
-                "modules/drivers/redshift/src"
+                 "modules/drivers/snowflake/src"
-                "modules/drivers/snowflake/src"
+                 "modules/drivers/sparksql/src"
-                "modules/drivers/sparksql/src"
+                 "modules/drivers/sqlserver/src"
-                "modules/drivers/sqlserver/src"
+                 "modules/drivers/starburst/src"
-                "modules/drivers/starburst/src"
+                 "modules/drivers/vertica/src"]
-                "modules/drivers/vertica/src"]
+   :jvm-opts    ["-Dclojure.main.report=stderr"]}
-   :jvm-opts   ["-Dclojure.main.report=stderr"]}
 
   ;; clojure -X:dev:ee:ee-dev:drivers:drivers-dev:test:eastwood
   :eastwood

mage/src/mage/check.clj

@@ -1,9 +0,0 @@
-(ns mage.check
-  (:require
-   [mage.shell :as shell]))
-
-(set! *warn-on-reflection* true)
-
-(defn check [_cli-args]
-  (let [{:keys [exit], :or {exit -1}} (shell/sh* "clojure" "-M:ee:drivers:check")]
-    (System/exit exit)))

mage/src/mage/load_namespaces.clj

@@ -0,0 +1,12 @@
+(ns mage.load-namespaces
+  (:require
+   [mage.shell :as shell]
+   [mage.util :as u]))
+
+(set! *warn-on-reflection* true)
+
+(defn load-namespaces
+  "Runs the `:load-namespaces` alias, exiting with whatever it exited with."
+  []
+  (let [{:keys [exit], :or {exit -1}} (shell/sh* "clojure" "-M:ee:drivers:load-namespaces")]
+    (u/exit exit)))