Forbid binary files in commits #792
Conversation
There was a problem hiding this comment.
+@allada +@adam-singer +@zbirenbaum +@blakehatch
Reviewable status: 0 of 4 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04 (waiting on @adam-singer, @allada, @blakehatch, and @zbirenbaum)
There was a problem hiding this comment.
Reviewable status: 1 of 4 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04 (waiting on @adam-singer, @allada, and @blakehatch)
There was a problem hiding this comment.
Reviewable status: 1 of 4 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04 (waiting on @adam-singer, @allada, and @blakehatch)
-- commits line 6 at r1:
nit: It might be a good idea to include a small statement about where to find the exclusions if you want to add one in the commit message or docs. Just something like “To add a new exclusion put the file path in the forbid-binary-files rule in pre-commit-hooks.nix” so people don’t waste time trying to hunt it down.
tools/pre-commit-hooks.nix line 54 at r1 (raw file):
types = ["text"]; }; forbid-binary-files = {
nit: Do these hooks run even if someone isn’t using nix?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 2 of 4 LGTMs obtained, and pending CI: pre-commit-checks (waiting on @allada and @blakehatch)
aaronmondal
force-pushed
the
forbid-binaries
branch
from
263f4a3 to
035f973
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 4 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04 (waiting on @allada and @blakehatch)
tools/pre-commit-hooks.nix line 54 at r1 (raw file):
Previously, zbirenbaum (Zach Birenbaum) wrote…
nit: Do these hooks run even if someone isn’t using nix?
No. They intentionally only work in the nix environment. When you enter the nix environment it invokes this script on startup:
Lines 214 to 216 in 09b32c9
This is to ensure that the hooks are reproducible between local and CI runs. This approach also "pins" the hook versions to each commit via flake.lock - when you roll back to a previous commit, the hooks in the environment will roll back as well.
allada
force-pushed
the
main
branch
2 times, most recently
from
a2a06c2 to
09defc6
Compare
This hook triggers on all non-excluded binary files. To exclude a file from this check, add it to the `excludes` section in `tools/pre-commit-hooks.nix`. Should prevent issues like TraceMachina#791 in the future.
aaronmondal
force-pushed
the
forbid-binaries
branch
from
035f973 to
e2f57e8
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 3 of 4 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Bazel Dev / ubuntu-22.04, Vercel, asan / ubuntu-22.04, integration-tests (20.04), pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04 (waiting on @blakehatch)
There was a problem hiding this comment.
Reviewable status: 3 of 3 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), macos-13, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 3 of 3 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), macos-13, windows-2022 / stable
This hook triggers on all non-excluded binary files.
Should prevent issues like #791 in the future.
This change is