From 1ec9a3f3edf397ce8f669510d6e74976f8b18406 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Thu, 13 Aug 2026 11:12:22 -0700 Subject: [PATCH] build(lint): fail on tracked binaries, and build the gateway binaries into bin/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? An 18MB `client` binary reached #547 and was caught only by review — nothing in CI guards against a committed binary. `go build ./service/submitqueue/gateway/client` names its output after the package directory and writes it to the *current* directory, so run from the repo root it drops a `client` there, where a broad `git add` sweeps it up. Every `service/*/server` package produces a root `server` the same way. The obvious fix — ignoring `/client` — is the wrong one. It would stop the file being committed but equally stop git mentioning it at all, so the mistake becomes invisible and the artifact quietly accumulates. It is also name-specific: it covers 8 of the 12 `main` packages and none of the other binaries anyone might add. Worse, an ignored path is never tracked, so it would hide the offender from the check below rather than complement it. ### What? `tool/linter/binaryfile` fails when any tracked file is binary. It follows the existing `licenseheader` / `messageid` / `queueshard` pattern and is wired in as `make lint-binary`, so `make lint` and the CI gate already cover it. Detection is git's own heuristic — a NUL byte in the leading 8000 bytes. The repository has no tracked binaries today, so the check starts clean and the allowlist is empty. `make build-submitqueue-gateway-client` and `make build-submitqueue-gateway-server` build with Bazel for the host platform into `bin/client` and `bin/server`, giving a sanctioned way to produce a standalone binary. `bin/` was already ignored but had no producer — the only reference to it anywhere in the Makefile, `tool/`, or `.github/` was `rm -rf bin/` in `clean`. No `.gitignore` change is needed: `bazel-bin/`, `bin/`, and `.docker-bin/` already cover every Bazel output, and the whole point of the linter is to surface a stray binary rather than hide it. ## Test Plan ✅ `bazel test //tool/linter/binaryfile:go_default_test` passes. ✅ `make lint-binary` on a clean tree reports `All 884 tracked files are text.` ✅ Reproduced #547 end to end: `go build ./service/submitqueue/gateway/client` from the repo root drops a 17,539,170-byte `client`; `git status` shows it as `?? client` rather than hiding it; after `git add client` the linter fails, naming the file and its size and pointing at Bazel. ✅ Both build targets produce Mach-O arm64 executables at `bin/client` and `bin/server`, are idempotent over Bazel's read-only output, and leave `git status` clean. ✅ `make gazelle` generates the linter's BUILD.bazel with no further drift, and `make help` lists all three new targets. --- Makefile | 23 +++- tool/linter/binaryfile/BUILD.bazel | 24 ++++ tool/linter/binaryfile/main.go | 182 ++++++++++++++++++++++++++++ tool/linter/binaryfile/main_test.go | 94 ++++++++++++++ 4 files changed, 321 insertions(+), 2 deletions(-) create mode 100644 tool/linter/binaryfile/BUILD.bazel create mode 100644 tool/linter/binaryfile/main.go create mode 100644 tool/linter/binaryfile/main_test.go diff --git a/Makefile b/Makefile index ff8fe283..b68749c2 100644 --- a/Makefile +++ b/Makefile @@ -52,7 +52,7 @@ define assert_clean fi endef -.PHONY: build build-all-linux build-runway-linux build-submitqueue-gateway-linux build-submitqueue-orchestrator-linux build-stovepipe-linux build-stovepipe-linux-debug check-gazelle check-mocks check-tidy clean clean-proto deps e2e-test fmt gazelle integration-test integration-test-submitqueue-consumer integration-test-extensions integration-test-submitqueue-gateway integration-test-submitqueue-orchestrator license-fix lint lint-fmt lint-license local-init-runway-queue-schema local-init-stovepipe-schemas local-runway-start local-runway-stop local-submitqueue-clean local-submitqueue-gateway-start local-submitqueue-gateway-stop local-init-submitqueue-schemas local-submitqueue-logs local-submitqueue-orchestrator-start local-submitqueue-orchestrator-stop local-submitqueue-ps local-submitqueue-restart local-submitqueue-start local-stop local-stovepipe-debug-start local-stovepipe-logs local-stovepipe-start local-stovepipe-stop mocks proto query-deps query-targets run-client-runway run-client-submitqueue-gateway run-client-submitqueue-orchestrator run-client-stovepipe run-queue-admin test test-no-cache tidy tidy-bazel tidy-go help +.PHONY: build build-all-linux build-runway-linux build-submitqueue-gateway-client build-submitqueue-gateway-linux build-submitqueue-gateway-server build-submitqueue-orchestrator-linux build-stovepipe-linux build-stovepipe-linux-debug check-gazelle check-mocks check-tidy clean clean-proto deps e2e-test fmt gazelle integration-test integration-test-submitqueue-consumer integration-test-extensions integration-test-submitqueue-gateway integration-test-submitqueue-orchestrator license-fix lint lint-binary lint-fmt lint-license local-init-runway-queue-schema local-init-stovepipe-schemas local-runway-start local-runway-stop local-submitqueue-clean local-submitqueue-gateway-start local-submitqueue-gateway-stop local-init-submitqueue-schemas local-submitqueue-logs local-submitqueue-orchestrator-start local-submitqueue-orchestrator-stop local-submitqueue-ps local-submitqueue-restart local-submitqueue-start local-stop local-stovepipe-debug-start local-stovepipe-logs local-stovepipe-start local-stovepipe-stop mocks proto query-deps query-targets run-client-runway run-client-submitqueue-gateway run-client-submitqueue-orchestrator run-client-stovepipe run-queue-admin test test-no-cache tidy tidy-bazel tidy-go help build: ## Build all services and examples @@ -74,6 +74,14 @@ build-runway-linux: ## Build Runway Linux binary for Docker cp -f bazel-bin/service/runway/server/runway .docker-bin/runway @echo "Runway Linux binary ready at .docker-bin/runway" +build-submitqueue-gateway-client: ## Build the gateway client CLI for the host platform into bin/client + @echo "Building gateway client..." + @$(BAZEL) build //service/submitqueue/gateway/client:gateway + @mkdir -p bin + @cp -f bazel-bin/service/submitqueue/gateway/client/gateway_/gateway bin/client 2>/dev/null || \ + cp -f bazel-bin/service/submitqueue/gateway/client/gateway bin/client + @echo "Gateway client ready at bin/client" + build-submitqueue-gateway-linux: ## Build Gateway Linux binary for Docker @echo "Building Gateway Linux binary for Docker..." @$(BAZEL) build --platforms=@rules_go//go/toolchain:linux_amd64 //service/submitqueue/gateway/server:gateway @@ -82,6 +90,14 @@ build-submitqueue-gateway-linux: ## Build Gateway Linux binary for Docker cp -f bazel-bin/service/submitqueue/gateway/server/gateway .docker-bin/gateway @echo "Gateway Linux binary ready at .docker-bin/gateway" +build-submitqueue-gateway-server: ## Build the gateway server for the host platform into bin/server + @echo "Building gateway server..." + @$(BAZEL) build //service/submitqueue/gateway/server:gateway + @mkdir -p bin + @cp -f bazel-bin/service/submitqueue/gateway/server/gateway_/gateway bin/server 2>/dev/null || \ + cp -f bazel-bin/service/submitqueue/gateway/server/gateway bin/server + @echo "Gateway server ready at bin/server" + build-submitqueue-orchestrator-linux: ## Build Orchestrator Linux binary for Docker @echo "Building Orchestrator Linux binary for Docker..." @$(BAZEL) build --platforms=@rules_go//go/toolchain:linux_amd64 //service/submitqueue/orchestrator/server:orchestrator @@ -177,9 +193,12 @@ integration-test-submitqueue-orchestrator: ## Run Orchestrator integration tests license-fix: ## Add missing license headers to source files @$(BAZEL) run //tool/linter/licenseheader -- --fix -lint: lint-fmt lint-license lint-message-id lint-queue-shard ## Run all linters +lint: lint-binary lint-fmt lint-license lint-message-id lint-queue-shard ## Run all linters @echo "All lint checks passed." +lint-binary: ## Check no binary file is tracked in the repository + @$(BAZEL) run //tool/linter/binaryfile + lint-fmt: fmt ## Check code formatting (fails if unformatted) $(call assert_clean,make fmt) @echo "All code is properly formatted." diff --git a/tool/linter/binaryfile/BUILD.bazel b/tool/linter/binaryfile/BUILD.bazel new file mode 100644 index 00000000..4c90b89a --- /dev/null +++ b/tool/linter/binaryfile/BUILD.bazel @@ -0,0 +1,24 @@ +load("@rules_go//go:def.bzl", "go_binary", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["main.go"], + importpath = "github.com/uber/submitqueue/tool/linter/binaryfile", + visibility = ["//visibility:private"], +) + +go_binary( + name = "binaryfile", + embed = [":go_default_library"], + visibility = ["//visibility:public"], +) + +go_test( + name = "go_default_test", + srcs = ["main_test.go"], + embed = [":go_default_library"], + deps = [ + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + ], +) diff --git a/tool/linter/binaryfile/main.go b/tool/linter/binaryfile/main.go new file mode 100644 index 00000000..ec5fe3ad --- /dev/null +++ b/tool/linter/binaryfile/main.go @@ -0,0 +1,182 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Command binaryfile checks that no binary file is tracked in the repository. +// +// The build is Bazel-driven and every artifact it produces lands in an ignored +// directory — bazel-bin/, bin/, or .docker-bin/ — so a tracked binary is always +// a mistake. The usual cause is an ad-hoc `go build ./service/...` run from the +// repo root: it names its executable after the package directory and writes it +// to the working directory, where a broad `git add` sweeps it up. +// +// Checking rather than ignoring is deliberate. A .gitignore entry would stop +// the file being committed but would also stop git mentioning it at all, so the +// mistake becomes invisible and the stray artifact simply accumulates. A check +// that fails names the file and says what to do instead. +// +// Detection follows git's own heuristic: a file is binary if a NUL byte appears +// in its leading bytes. +package main + +import ( + "bytes" + "flag" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" +) + +// sniffLen is how many leading bytes are examined for a NUL. It matches the +// window git uses for the same decision, which is large enough to cover any +// text header an executable format might begin with. +const sniffLen = 8000 + +// allowed lists tracked paths that are legitimately binary, relative to the +// repository root. It is empty because nothing in the repository is: the tree +// is source, schemas, and generated Go. An entry belongs here only when a +// binary genuinely has to be versioned — a test fixture that cannot be built, +// or an image a document renders — never to silence a stray build artifact. +var allowed = map[string]bool{} + +// violation is one tracked file that is binary. +type violation struct { + path string + size int64 +} + +func main() { + flag.Parse() + + root, err := findRepoRoot() + if err != nil { + fmt.Fprintf(os.Stderr, "error: %v\n", err) + os.Exit(1) + } + + files, err := trackedFiles(root) + if err != nil { + fmt.Fprintf(os.Stderr, "error: %v\n", err) + os.Exit(1) + } + + var violations []violation + var checked int + for _, path := range files { + if allowed[path] { + continue + } + info, err := os.Lstat(filepath.Join(root, path)) + // A tracked path that is missing or is a symlink has no contents of its + // own to judge; skip it rather than failing the whole run. + if err != nil || !info.Mode().IsRegular() { + continue + } + checked++ + + binary, err := isBinaryFile(filepath.Join(root, path)) + if err != nil { + fmt.Fprintf(os.Stderr, "error: %v\n", err) + os.Exit(1) + } + if binary { + violations = append(violations, violation{path: path, size: info.Size()}) + } + } + + if len(violations) > 0 { + fmt.Fprintf(os.Stderr, "%d binary file(s) are tracked:\n\n", len(violations)) + for _, v := range violations { + fmt.Fprintf(os.Stderr, " %s (%d bytes)\n", v.path, v.size) + } + fmt.Fprintf(os.Stderr, "\nThe build is Bazel-driven and writes to bazel-bin/, bin/, and\n") + fmt.Fprintf(os.Stderr, ".docker-bin/, all of which are ignored, so a tracked binary is a\n") + fmt.Fprintf(os.Stderr, "mistake. If this is a stray `go build` output, delete it and use\n") + fmt.Fprintf(os.Stderr, "Bazel instead: `make build`, or `make run-client-submitqueue-gateway`\n") + fmt.Fprintf(os.Stderr, "to run the client without producing a binary at all.\n") + os.Exit(1) + } + + fmt.Printf("All %d tracked files are text.\n", checked) +} + +// isBinaryFile reports whether the file at path is binary, reading no more than +// the leading sniffLen bytes. +func isBinaryFile(path string) (bool, error) { + file, err := os.Open(path) + if err != nil { + return false, fmt.Errorf("failed to open %s: %w", path, err) + } + defer func() { _ = file.Close() }() + + buf := make([]byte, sniffLen) + n, err := file.Read(buf) + if err != nil && n == 0 { + // A read that returns nothing, including io.EOF on an empty file, leaves + // an empty window, which isBinary correctly reports as text. + return isBinary(nil), nil + } + return isBinary(buf[:n]), nil +} + +// isBinary reports whether a leading window of a file's contents looks binary, +// which is true exactly when it contains a NUL byte. An empty window is text: +// an empty file has nothing to make it binary. +func isBinary(window []byte) bool { + return bytes.IndexByte(window, 0) >= 0 +} + +// trackedFiles returns every path git tracks, relative to root. +// +// The check is about what is committed rather than what is present, so the file +// list comes from the index; a stray artifact that is untracked is a local +// matter and not this linter's business. +func trackedFiles(root string) ([]string, error) { + cmd := exec.Command("git", "-C", root, "ls-files", "-z") + out, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("failed to list tracked files: %w", err) + } + + var files []string + for _, path := range strings.Split(string(out), "\x00") { + if path != "" { + files = append(files, path) + } + } + return files, nil +} + +func findRepoRoot() (string, error) { + // Bazel `run` executes from the runfiles tree; BUILD_WORKSPACE_DIRECTORY + // points back at the source tree. + if dir := os.Getenv("BUILD_WORKSPACE_DIRECTORY"); dir != "" { + return dir, nil + } + dir, err := os.Getwd() + if err != nil { + return "", err + } + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir, nil + } + parent := filepath.Dir(dir) + if parent == dir { + return "", fmt.Errorf("could not find repository root (no go.mod found)") + } + dir = parent + } +} diff --git a/tool/linter/binaryfile/main_test.go b/tool/linter/binaryfile/main_test.go new file mode 100644 index 00000000..1c411d9f --- /dev/null +++ b/tool/linter/binaryfile/main_test.go @@ -0,0 +1,94 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestIsBinary(t *testing.T) { + tests := []struct { + name string + window []byte + want bool + }{ + {name: "empty is text", window: nil, want: false}, + {name: "ascii source is text", window: []byte("package main\n"), want: false}, + {name: "utf-8 is text", window: []byte("// © Uber — naïve\n"), want: false}, + {name: "crlf is text", window: []byte("a\r\nb\r\n"), want: false}, + {name: "high bytes without NUL are text", window: []byte{0x80, 0xfe, 0xff}, want: false}, + {name: "leading NUL is binary", window: []byte{0x00, 'a'}, want: true}, + {name: "trailing NUL is binary", window: []byte{'a', 0x00}, want: true}, + {name: "elf header is binary", window: []byte{0x7f, 'E', 'L', 'F', 0x02, 0x00}, want: true}, + {name: "mach-o header is binary", window: []byte{0xcf, 0xfa, 0xed, 0xfe, 0x0c, 0x00}, want: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isBinary(tt.window)) + }) + } +} + +func TestIsBinaryFile(t *testing.T) { + dir := t.TempDir() + + tests := []struct { + name string + contents []byte + want bool + }{ + {name: "empty file is text", contents: []byte{}, want: false}, + {name: "go source is text", contents: []byte("package main\n\nfunc main() {}\n"), want: false}, + {name: "executable is binary", contents: append([]byte{0x7f, 'E', 'L', 'F'}, make([]byte, 512)...), want: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := filepath.Join(dir, tt.name) + require.NoError(t, os.WriteFile(path, tt.contents, 0o600)) + + got, err := isBinaryFile(path) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestIsBinaryFileOnlyReadsTheLeadingWindow(t *testing.T) { + // A NUL past the sniff window is not reached, which is what bounds the + // linter's cost on a large text file rather than a claim about the file. + path := filepath.Join(t.TempDir(), "late-nul") + contents := append(make([]byte, 0, sniffLen+2), []byte("package main\n")...) + for len(contents) < sniffLen { + contents = append(contents, 'x') + } + contents = append(contents, 0x00) + require.NoError(t, os.WriteFile(path, contents, 0o600)) + + got, err := isBinaryFile(path) + require.NoError(t, err) + assert.False(t, got) +} + +func TestIsBinaryFileMissing(t *testing.T) { + _, err := isBinaryFile(filepath.Join(t.TempDir(), "absent")) + require.Error(t, err) +}