From 7945bd383a4c9aa6ec713fb1a84b68a49a59a148 Mon Sep 17 00:00:00 2001 From: Dave Rolle Date: Mon, 11 May 2026 13:40:39 +0200 Subject: [PATCH] feat: emit buildifier-canonical BUILD.bazel files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrite `emit-bazel*` to produce output identical to `buildifier --type=build`. A round-trip test pipes generated content through buildifier and asserts zero diff. Generated `BUILD.bazel` files previously used non-standard formatting: all kwargs on one line, no attribute sorting, no trailing commas, an `#autogenerated` comment header. IDEs configured with buildifier would reformat the entire file on open, creating noisy diffs (banksy#15022 comment). The first re-run of `gen_srcs` after this lands will produce a one-time diff on every generated BUILD.bazel. Emission changes (gen_build.clj): - attr-priority + sortable-list-attrs match buildifier's NamePriority + IsSortableListArg subsets - label-sort-key uses phase ordering (: < // < @) - emit-bazel-kwargs returns sorted "k = v" entry strings; multi-element list values render multi-line via emit-vector - IPersistentList: inline when positional-only or single-kwarg-no- positional, multi-line otherwise (4-space arg indent, trailing comma) - IPersistentList: load() args after the bzl path are sorted alphabetic - IPersistentVector: standalone inline with ", " separator - IPersistentMap: dict literals use `{"k": "v"}` (no space before `:`) - emit-bazel* String: `//path/pkg:pkg` shortens to `//path/pkg` - build-file-header replaces the "#autogenerated" comment with a Starlark triple-quoted docstring Generated content layout (gen-dir, gen-deps-build): - docstring header at top - load() before package() - distinct over dedupe in __clj_lib deps construction - trailing newline at end of file Tests (gen_build_test.clj): - Vector inline (empty, single, multi-element) - Inline function calls (positional-only, single-kwarg) - Multi-kwarg multi-line with sorted attrs - Attribute priority ordering - Label sort phase ordering; unsortable attrs (aot) preserved - Buildifier round-trip — fails explicitly when buildifier absent CI / build: - MODULE.bazel: `buildifier_prebuilt` 8.5.1.2 as a dev_dependency - test BUILD: gen-build-test declares buildifier as a `data` dep and exports its runfiles path via the `BUILDIFIER` env var; the test resolves it through test-utils/runfiles-env — no PATH lookup - .circleci/config.yml: no install step needed Addresses: TOOL-518 --- MODULE.bazel | 52 +++++--- MODULE.bazel.lock | 5 +- src/rules_clojure/gen_build.clj | 183 +++++++++++++++++++------- test/rules_clojure/BUILD | 2 + test/rules_clojure/gen_build_test.clj | 95 ++++++++++++- 5 files changed, 266 insertions(+), 71 deletions(-) diff --git a/MODULE.bazel b/MODULE.bazel index 5e34e3d..3a2b945 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -1,10 +1,14 @@ -module(name = "rules_clojure", - version = "0.5") +module( + name = "rules_clojure", + version = "0.5", +) bazel_dep(name = "rules_java", version = "8.16.1") bazel_dep(name = "rules_jvm_external", version = "6.8") -maven = use_extension("@rules_jvm_external//:extensions.bzl", "maven") +bazel_dep(name = "buildifier_prebuilt", version = "8.5.1.2", dev_dependency = True) + +maven = use_extension("@rules_jvm_external//:extensions.bzl", "maven") maven.install( name = "maven_deps", artifacts = [ @@ -13,50 +17,56 @@ maven.install( "org.clojure:core.specs.alpha:0.4.74", "org.clojure:data.json:2.4.0", "org.clojure:tools.deps:0.28.1578", - "org.clojure:tools.namespace:1.1.0" + "org.clojure:tools.namespace:1.1.0", ], + fail_if_repin_required = True, repositories = [ "https://repo1.maven.org/maven2", - "https://repo.clojars.org/" + "https://repo.clojars.org/", ], - fail_if_repin_required = True ) - maven.amend_artifact( name = "maven_deps", coordinates = "org.clojure:clojure", - exclusions = ["org.clojure:spec.alpha", - "org.clojure:core.specs.alpha"] + exclusions = [ + "org.clojure:spec.alpha", + "org.clojure:core.specs.alpha", + ], ) - maven.amend_artifact( name = "maven_deps", coordinates = "org.clojure:spec.alpha", - exclusions = ["org.clojure:clojure", - "org.clojure:core.specs.alpha"] + exclusions = [ + "org.clojure:clojure", + "org.clojure:core.specs.alpha", + ], ) - maven.amend_artifact( name = "maven_deps", coordinates = "org.clojure:core.specs.alpha", - exclusions = ["org.clojure:clojure", - "org.clojure:spec.alpha"] + exclusions = [ + "org.clojure:clojure", + "org.clojure:spec.alpha", + ], ) # used for testing maven.install( name = "clojure_old", artifacts = [ - "org.clojure:clojure:1.8.0",], + "org.clojure:clojure:1.8.0", + ], fail_if_repin_required = True, repositories = [ "https://repo1.maven.org/maven2", - "https://repo.clojars.org/" - ]) - + "https://repo.clojars.org/", + ], +) use_repo(maven, "maven_deps") use_repo(maven, "clojure_old") bazel_dep(name = "example-simple", version = "0.0.0", dev_dependency = True) -local_path_override(module_name = "example-simple", - path = "examples/simple") +local_path_override( + module_name = "example-simple", + path = "examples/simple", +) diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index 3c142bf..c2623f2 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -46,6 +46,8 @@ "https://bcr.bazel.build/modules/bazel_skylib/1.7.1/MODULE.bazel": "3120d80c5861aa616222ec015332e5f8d3171e062e3e804a2a0253e1be26e59b", "https://bcr.bazel.build/modules/bazel_skylib/1.8.1/MODULE.bazel": "88ade7293becda963e0e3ea33e7d54d3425127e0a326e0d17da085a5f1f03ff6", "https://bcr.bazel.build/modules/bazel_skylib/1.8.1/source.json": "7ebaefba0b03efe59cac88ed5bbc67bcf59a3eff33af937345ede2a38b2d368a", + "https://bcr.bazel.build/modules/buildifier_prebuilt/8.5.1.2/MODULE.bazel": "9a6e0a2e87d1e3da679e157da5192ea351d5739ca1ff51831c2b736d5b6034de", + "https://bcr.bazel.build/modules/buildifier_prebuilt/8.5.1.2/source.json": "33e11b3bf11e39cb762480a7e6ea1d24d044636135cdd8b8e74b07ebcd3b8d8b", "https://bcr.bazel.build/modules/buildozer/7.1.2/MODULE.bazel": "2e8dd40ede9c454042645fd8d8d0cd1527966aa5c919de86661e62953cd73d84", "https://bcr.bazel.build/modules/buildozer/7.1.2/source.json": "c9028a501d2db85793a6996205c8de120944f50a0d570438fcae0457a5f9d1f8", "https://bcr.bazel.build/modules/google_benchmark/1.8.2/MODULE.bazel": "a70cf1bba851000ba93b58ae2f6d76490a9feb74192e57ab8e8ff13c34ec50cb", @@ -163,7 +165,8 @@ "https://bcr.bazel.build/modules/rules_python/1.4.1/source.json": "8ec8c90c70ccacc4de8ca1b97f599e756fb59173e898ee08b733006650057c07", "https://bcr.bazel.build/modules/rules_shell/0.2.0/MODULE.bazel": "fda8a652ab3c7d8fee214de05e7a9916d8b28082234e8d2c0094505c5268ed3c", "https://bcr.bazel.build/modules/rules_shell/0.3.0/MODULE.bazel": "de4402cd12f4cc8fda2354fce179fdb068c0b9ca1ec2d2b17b3e21b24c1a937b", - "https://bcr.bazel.build/modules/rules_shell/0.3.0/source.json": "c55ed591aa5009401ddf80ded9762ac32c358d2517ee7820be981e2de9756cf3", + "https://bcr.bazel.build/modules/rules_shell/0.6.1/MODULE.bazel": "72e76b0eea4e81611ef5452aa82b3da34caca0c8b7b5c0c9584338aa93bae26b", + "https://bcr.bazel.build/modules/rules_shell/0.6.1/source.json": "20ec05cd5e592055e214b2da8ccb283c7f2a421ea0dc2acbf1aa792e11c03d0c", "https://bcr.bazel.build/modules/rules_swift/1.16.0/MODULE.bazel": "4a09f199545a60d09895e8281362b1ff3bb08bbde69c6fc87aff5b92fcc916ca", "https://bcr.bazel.build/modules/rules_swift/2.1.1/MODULE.bazel": "494900a80f944fc7aa61500c2073d9729dff0b764f0e89b824eb746959bc1046", "https://bcr.bazel.build/modules/rules_swift/2.1.1/source.json": "40fc69dfaac64deddbb75bd99cdac55f4427d9ca0afbe408576a65428427a186", diff --git a/src/rules_clojure/gen_build.clj b/src/rules_clojure/gen_build.clj index d56cc30..3925a45 100644 --- a/src/rules_clojure/gen_build.clj +++ b/src/rules_clojure/gen_build.clj @@ -67,6 +67,64 @@ val (throw (ex-info "value does not conform" (s/explain-data spec val))))) +(def ^:private attr-priority + "Attribute sort priorities based on a subset of buildifier's NamePriority table. + Lower numbers sort first. Unlisted attributes default to 0." + {:name -99 + :src -92 + :srcs -90 + :outs -88 + :hdrs -87 + :size -95 + :timeout -94 + :testonly -93 + :exports 2 + :runtime_deps 3 + :deps 4 + :implementation 5 + :implements 6 + :alwayslink 7}) + +(def ^:private sortable-list-attrs + "List attributes that buildifier sorts alphabetically. + Superset of attrs we generate, from buildifier's IsSortableListArg." + #{:data :default_visibility :deps :exports :hdrs :implementation_deps + :outs :private_deps :resources :runtime_deps :srcs :tags :tests :tools}) + +(defn- build-file-header + "Starlark docstring header for generated BUILD files." + [generator-description] + (str "\"\"\"\nDo not edit this file manually!\n" + "It is automatically generated by " generator-description ".\n\"\"\"\n\n")) + +(defn- sort-kwargs-entries + "Sort kwargs entries by buildifier priority: name first, then by priority + bucket, then alphabetically within bucket." + [entries] + (sort-by (fn [[k _]] [(get attr-priority k 0) (name k)]) entries)) + +(defn- label-sort-key + "Sort key matching buildifier's byStringExpr: phase by prefix (: < // < @), + then by the label with colons replaced by dots (equivalent to buildifier's + segment-by-segment comparison since segments contain no dots)." + [s] + (let [phase (cond + (str/starts-with? s ":") 1 + (str/starts-with? s "//") 2 + (str/starts-with? s "@") 3 + :else 0)] + [phase (str/replace s ":" ".")])) + +(defn- emit-vector + "Emit a vector of pre-stringified items. Inline when count <= 1, otherwise + multi-line with 8-space element indent + 4-space closing bracket." + [items] + (if (<= (count items) 1) + (str "[" (str/join ", " items) "]") + (str "[\n " + (str/join ",\n " items) + ",\n ]"))) + (defn emit-bazel-dispatch [x] (class x)) @@ -92,8 +150,18 @@ (defmethod emit-bazel* :default [x] (assert false (print-str "don't know how to emit" (class x)))) +(defn- shorten-label + "Buildifier rule: collapse `//path/to/pkg:pkg` to `//path/to/pkg` when the + target name equals the directory basename. Leaves all other strings + unchanged." + [s] + (let [m (re-matches #"(.*//.+)/([^/:]+):([^/:]+)" s)] + (if (and m (= (nth m 2) (nth m 3))) + (str (nth m 1) "/" (nth m 2)) + s))) + (defmethod emit-bazel* String [x] - (pr-str x)) + (pr-str (shorten-label x))) (defmethod emit-bazel* Keyword [x] (name x)) @@ -106,36 +174,59 @@ true "True" false "False")) -(defn emit-bazel-kwargs [kwargs] +(defn emit-bazel-kwargs + "Return a seq of \"k = v\" entry strings, sorted by buildifier attribute priority. + Sortable list-typed values (per `sortable-list-attrs`) are sorted by `label-sort-key`. + Vector values render inline (<=1 element) or multi-line via `emit-vector`." + [kwargs] {:pre [(map? kwargs)] - :post [(string? %)]} + :post [(sequential? %)]} (->> (:x kwargs) + sort-kwargs-entries (map (fn [[k v]] - (print-str (emit-bazel* k) "=" (emit-bazel* v)))) - (interpose ",\n\t") - (apply str))) + (let [v-str (cond + (and (vector? v) (contains? sortable-list-attrs k)) + (emit-vector (->> v (sort-by label-sort-key) (mapv emit-bazel*))) + (vector? v) + (emit-vector (mapv emit-bazel* v)) + :else (emit-bazel* v))] + (str (emit-bazel* k) " = " v-str)))))) (defmethod emit-bazel* KeywordArgs [x] - (emit-bazel-kwargs x)) - -(defmethod emit-bazel* IPersistentList [[name & args]] - ;; function call - (let [args (when (seq args) - (mapv emit-bazel* args))] - (str name "(" (apply str (interpose ", " args)) ")"))) + (str/join ", " (emit-bazel-kwargs x))) + +(defmethod emit-bazel* IPersistentList [[fn-name & args]] + ;; Buildifier-canonical function call: + ;; - positional-only OR single-kwarg-no-positional: single-line. + ;; - mixed or >1 kwarg: multi-line with 4-space arg indent and trailing comma. + ;; - load() args after the bzl-path are sorted alphabetically (buildifier rule). + (let [kwarg-args (filter kwargs? args) + positional (remove kwargs? args) + positional-strs (cond->> (mapv emit-bazel* positional) + (= 'load fn-name) (#(into [(first %)] (sort (rest %))))) + kwarg-entries (vec (mapcat emit-bazel-kwargs kwarg-args)) + kwarg-count (count kwarg-entries) + positional-count (count positional-strs)] + (if (or (zero? kwarg-count) + (and (zero? positional-count) (= 1 kwarg-count))) + (str fn-name "(" (str/join ", " (concat positional-strs kwarg-entries)) ")") + (str fn-name "(\n " + (str/join ",\n " (concat positional-strs kwarg-entries)) + ",\n)")))) (defmethod emit-bazel* IPersistentVector [x] - (str "[" (->> x - (map emit-bazel*) - (interpose ",") - (apply str)) "]")) + ;; Standalone vector (non-kwarg context): inline. Kwarg-context formatting + ;; with sorting and multi-line is handled in emit-bazel-kwargs. + (str "[" (->> x (map emit-bazel*) (str/join ", ")) "]")) (defmethod emit-bazel* IPersistentMap [x] - (str "{" (->> x - (map (fn [[k v]] - (str (emit-bazel* k) " : " (emit-bazel* v)))) - (interpose ",") - (apply str)) "}")) + ;; Buildifier-canonical dict literal: `{"k": "v"}` (no space before colon). + (str "{" + (->> x + (map (fn [[k v]] + (str (emit-bazel* k) ": " (emit-bazel* v)))) + (str/join ", ")) + "}")) (s/fdef emit-bazel :args (s/cat :x ::bazel) :ret string?) (defn emit-bazel @@ -726,32 +817,30 @@ (mapcat (fn [[_base paths]] (ns-rules args paths))))) - content (str "#autogenerated, do not edit\n" - (emit-bazel (list 'package (kwargs {:default_visibility ["//visibility:public"]}))) - "\n" + content (str (build-file-header "the `//:gen_srcs` target from `rules_clojure`") (emit-bazel (apply list 'load "@rules_clojure//:rules.bzl" (cond-> ["clojure_library" "clojure_test"] has-binary? (conj "clojure_binary")))) + "\n\n" + (emit-bazel (list 'package (kwargs {:default_visibility ["//visibility:public"]}))) "\n" - "\n" - (str/join "\n\n" rules) - "\n" - "\n" + (when (seq rules) + (str "\n" (str/join "\n\n" rules) "\n")) (when (or (seq paths) (seq clj-subdirs)) - (str - (emit-bazel (list 'clojure_library (kwargs {:name "__clj_lib" - :deps (vec - (concat - (dedupe (map (fn [p] (str ":" (fs/basename p))) paths)) - (map (fn [p] - (str "//" (fs/path-relative-to deps-edn-dir p) ":__clj_lib")) clj-subdirs)))}))) - "\n" - "\n" - (emit-bazel (list 'filegroup (kwargs (merge - {:name "__clj_files" - :srcs (mapv fs/filename paths) - :data (mapv (fn [p] - (str "//" (fs/path-relative-to deps-edn-dir p) ":__clj_files")) clj-subdirs)})))))))] + (str "\n" + (emit-bazel (list 'clojure_library (kwargs {:name "__clj_lib" + :deps (vec + (concat + (distinct (map (fn [p] (str ":" (fs/basename p))) paths)) + (map (fn [p] + (str "//" (fs/path-relative-to deps-edn-dir p) ":__clj_lib")) clj-subdirs)))}))) + "\n\n" + (emit-bazel (list 'filegroup (kwargs (merge + {:name "__clj_files" + :srcs (mapv fs/filename paths) + :data (mapv (fn [p] + (str "//" (fs/path-relative-to deps-edn-dir p) ":__clj_files")) clj-subdirs)})))) + "\n")))] (let [build-file (-> dir (fs/->path "BUILD.bazel") fs/path->file) changed? (not= content (when (.exists build-file) (slurp build-file :encoding "UTF-8")))] (when changed? @@ -837,9 +926,10 @@ "generates the BUILD file for @deps//: with a single target containing all deps.edn-resolved dependencies" [{:keys [repository-dir deps-build-dir dep-ns->label jar->lib lib->jar lib->deps deps-repo-tag deps-bazel] :as args}] (spit (-> (fs/->path deps-build-dir "BUILD.bazel") fs/path->file) - (str/join "\n\n" (concat - [(emit-bazel (list 'package (kwargs {:default_visibility ["//visibility:public"]}))) - (emit-bazel (list 'load "@rules_clojure//:rules.bzl" "clojure_library"))] + (str (build-file-header "`rules_clojure`") + (str/join "\n\n" (concat + [(emit-bazel (list 'load "@rules_clojure//:rules.bzl" "clojure_library")) + (emit-bazel (list 'package (kwargs {:default_visibility ["//visibility:public"]})))] (->> jar->lib (sort-by (fn [[k v]] (library->label v))) (mapcat (fn [[jarpath lib]] @@ -894,6 +984,7 @@ {:name "__all" :runtime_deps (->> jar->lib (mapv (comp library->label val)))})))])) + "\n") :encoding "UTF-8")) diff --git a/test/rules_clojure/BUILD b/test/rules_clojure/BUILD index 48c06da..2102cf6 100644 --- a/test/rules_clojure/BUILD +++ b/test/rules_clojure/BUILD @@ -67,6 +67,8 @@ clojure_test(name="persistent-classloader-test", clojure_test(name="gen-build-test", deps=[":test-deps"], + data=["@buildifier_prebuilt//:buildifier"], + env={"BUILDIFIER": "$(rlocationpath @buildifier_prebuilt//:buildifier)"}, test_ns = "rules-clojure.gen-build-test") clojure_test(name="compile-test", diff --git a/test/rules_clojure/gen_build_test.clj b/test/rules_clojure/gen_build_test.clj index 5c9f646..703ff6c 100644 --- a/test/rules_clojure/gen_build_test.clj +++ b/test/rules_clojure/gen_build_test.clj @@ -1,9 +1,11 @@ (ns rules-clojure.gen-build-test - (:require [clojure.string :as str] + (:require [clojure.java.io :as io] + [clojure.java.shell :as shell] + [clojure.string :as str] [clojure.test :refer [deftest is testing]] - [clojure.java.io :as io] + [rules-clojure.fs :as fs] [rules-clojure.gen-build :as gb] - [rules-clojure.fs :as fs])) + [rules-clojure.test-utils :as test-utils])) (defn- make-temp-dir "Create a temp directory with a src/ subdirectory (matching basis :paths)." @@ -171,3 +173,90 @@ (is (not (contains? attrs :tags)) "absent :tags should not appear in attrs") (finally (fs/rm-rf (.toPath dir))))))) + + +;; ---- formatting tests (emit-bazel) ---- + +(deftest test-emit-bazel-vector-inline + (testing "empty vector" + (is (= "[]" (gb/emit-bazel [])))) + (testing "single element" + (is (= "[\"a\"]" (gb/emit-bazel ["a"])))) + (testing "single element stays inline regardless of length" + (is (= "[\"long-element-name\"]" (gb/emit-bazel ["long-element-name"])))) + (testing "multi-element standalone vector renders inline with comma+space separator" + (is (= "[\"a\", \"b\"]" (gb/emit-bazel ["a" "b"]))))) + +(deftest test-emit-bazel-function-call-inline + (testing "positional args only" + (is (= "load(\"@rules_clojure//:rules.bzl\", \"clojure_library\")" + (gb/emit-bazel (list 'load "@rules_clojure//:rules.bzl" "clojure_library"))))) + (testing "single kwarg stays inline" + (is (= "package(default_visibility = [\"//visibility:public\"])" + (gb/emit-bazel (list 'package (gb/kwargs {:default_visibility ["//visibility:public"]}))))))) + +(deftest test-emit-bazel-function-call-multiline + (testing "multiple kwargs go multiline with sorted attrs" + (let [result (gb/emit-bazel + (list 'clojure_library + (gb/kwargs {:name "build" + :deps ["//resources:data_readers" + "@deps//:org_clojure_clojure"] + :srcs ["build.clj"] + :aot ["build"]})))] + (is (= (str "clojure_library(\n" + " name = \"build\",\n" + " srcs = [\"build.clj\"],\n" + " aot = [\"build\"],\n" + " deps = [\n" + " \"//resources:data_readers\",\n" + " \"@deps//:org_clojure_clojure\",\n" + " ],\n" + ")") + result))))) + +(deftest test-attr-sorting + (testing "name first, then by priority, then alphabetical" + (let [result (gb/emit-bazel + (list 'clojure_library + (gb/kwargs {:deps ["a"] :name "x" :aot ["b"] :srcs ["c"] :runtime_deps ["d"]})))] + (is (str/starts-with? result "clojure_library(\n name = \"x\",\n srcs = [\"c\"],")) + (is (str/includes? result " aot = [\"b\"],")) + (is (str/includes? result " runtime_deps = [\"d\"],\n deps = [\"a\"],"))))) + +(deftest test-list-sorting + (testing "deps are sorted by buildifier phase order (: < // < @)" + (let [result (gb/emit-bazel + (list 'clojure_library + (gb/kwargs {:name "x" + :deps ["@deps//:zzz" ":aaa" "//pkg:lib"]})))] + (is (str/includes? result " \":aaa\",\n \"//pkg:lib\",\n \"@deps//:zzz\",")))) + (testing "aot is NOT sorted (not in sortable set)" + (let [result (gb/emit-bazel + (list 'clojure_library + (gb/kwargs {:name "x" + :aot ["z-ns" "a-ns"]})))] + (is (str/includes? result " \"z-ns\",\n \"a-ns\","))))) + +(deftest test-buildifier-round-trip + (testing "generated output is buildifier-stable" + (let [buildifier-path (first (test-utils/runfiles-env "BUILDIFIER"))] + (is (some? buildifier-path) "BUILDIFIER env var must be set (see :env on the gen-build-test target)") + (when buildifier-path + (let [content (str "\"\"\"\nTest file.\n\"\"\"\n\n" + (gb/emit-bazel (list 'load "@rules_clojure//:rules.bzl" "clojure_library")) + "\n\n" + (gb/emit-bazel (list 'package (gb/kwargs {:default_visibility ["//visibility:public"]}))) + "\n\n" + (gb/emit-bazel + (list 'clojure_library + (gb/kwargs {:name "test" + :deps ["@deps//:zzz" ":aaa"] + :srcs ["z.clj" "a.clj"] + :aot ["z-ns" "a-ns"] + :runtime_deps ["@deps//:bbb"]}))) + "\n") + result (shell/sh buildifier-path "--type=build" :in content)] + (is (zero? (:exit result)) (str "buildifier failed: " (:err result))) + (is (= content (:out result)) + (str "buildifier produced diff. Expected:\n" content "\nGot:\n" (:out result))))))))