Skip to content

Commit 567f607

Browse files
redsun82Copilot
andcommitted
Just: make forwarded command output readable
Two things made the output of a forwarded command hard to follow: the separator between commands was a fixed 56 dashes, reading as a short dash in a wide terminal rather than a break between commands; and the banner shown before each one was `just` echoing the recipe line verbatim, over 500 characters for the bazel one and carrying an unexpanded `"$@"` that matched no file in another shell. The separator is now drawn from `stty`-measured terminal width, repeating `#` rather than following it with dashes so the whole line reads as one thing, a shell comment, instead of two -- which also matters because the line is interpolated into a shell script, so a value that is not itself a shell comment would silently run. The width is exported as `JUST_CMD_RULE` so a forwarded verb inherits it instead of re-measuring in every justfile it reaches, and nothing changes without a terminal: piped output, CI logs and Windows still get the original 56 dashes. The banner is replaced by a `-> `-prefixed report of what the command actually is, built by the same file runner that decides which files matched, so it reports rather than merely intends. Buildifier's own output is filtered down to the files it actually rewrote, keeping only the tally of warnings it could not fix, since fix mode reports no other detail and always exits 0; its Starlark-file detection gains the one name this repository's list was missing (`*.star`); an exclusion is now matched against the absolute path as well as the given one, and resolved through symlinks first, both of which had let a repository excluding a nested one format it anyway; and naming a path that does not exist is now an error rather than silent, exit-0 success. Also folds in a few nearby fixes found the same way: opting out of MSYS2's path-like argument rewriting once, centrally, instead of per bazel-invoking recipe; printing the actual `error:` prefix instead of `{error}`, which was Python's interpolation syntax rather than just's; and moving each formatter's own values next to the recipe that reads them, instead of a shared header that had to be read whole to make sense of any one of them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 1a783b5 commit 567f607

7 files changed

Lines changed: 214 additions & 91 deletions

File tree

‎misc/bazel/buildifier/BUILD.bazel‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,3 @@ buildifier(
88
],
99
lint_mode = "fix",
1010
)
11-
12-
# The binary behind the target above, which formats the paths it is given rather than
13-
# always the whole workspace. `just format` goes through this so that formatting a
14-
# directory formats that directory.
15-
alias(
16-
name = "binary",
17-
actual = "@buildifier_prebuilt//:buildifier",
18-
)

‎misc/just/README.md‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,3 +149,40 @@ to be understood by all of them. That is fine when they speak the same language,
149149
a broad `just test .` reaches bazel and pytest suites alike, and a flag meant for one of
150150
them will fail on the other. It fails rather than being quietly ignored, so the answer
151151
is to aim the verb at something narrower.
152+
153+
# Command separators
154+
155+
Commands are echoed between rules that span the terminal. The formatters echo a summary
156+
rather than the line that runs: they name the formatter and what it is told to do, and
157+
leave out the wrapper that collects the files, the patterns it walks and the flags that
158+
only shape output. A leading `-> ` marks a line as that summary. `just -n` prints what
159+
actually runs, which it does whether or not the recipe is `@`-quiet.
160+
161+
Nothing is lost by this, as a banner has never been something to paste: the echoed line
162+
carried an unexpanded `"$@"`, which matches no file in another shell, so pasting one
163+
formatted nothing and exited 0.
164+
165+
What a formatter says for itself is filtered down to what happened, as several name every
166+
file they considered and most of them were left alone. One such line is kept on purpose:
167+
buildifier's count of the warnings it could not fix, which is the only notice of them,
168+
since it reports no detail in fix mode and exits 0 whether or not any remain. That tally
169+
grows with the tree and is mostly lint about docstrings. Finding it tiresome is a reason
170+
to configure what buildifier lints, never to widen the filter back over it, which would
171+
take the warnings worth having along with the rest.
172+
173+
Measuring the width means a `shell()` call, and that runs on every parse, so the result
174+
is exported as `JUST_CMD_RULE` and an inherited value is preferred to measuring again.
175+
176+
Inheritance crosses processes, which is what forwarding creates: a child per justfile
177+
reached, each of them measuring nothing. A `mod` spawns no process, so a module measures
178+
for itself, and the count is one per `mod` reached, however deeply nested, plus one for
179+
the file itself. That is cheap, and modules agree anyway since they share a terminal,
180+
but it is worth knowing before counting measurements. Presetting `JUST_CMD_RULE` skips
181+
all of them, and is also how to fix the width in CI or in a recording — it is the whole
182+
separator and ends up in a shell script, so every line of it has to be a comment, or it
183+
runs.
184+
185+
With no terminal to ask — a pipe, a log, a shell without `stty` — it falls back to a
186+
fixed 57 columns, so logs and CI output are the same width every time. That is one
187+
branch rather than a platform test: `just` runs `sh` everywhere, so Windows takes
188+
whichever arm fits rather than a path of its own.

‎misc/just/build.just‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,12 @@ _build_dist LANGUAGE: _require_semmle_code (_maybe_build_dist LANGUAGE)
1414
# something `--ui_event_filters` can let back through, and a build this long is one to
1515
# see the progress of.
1616
[no-exit-message]
17-
_maybe_build_dist LANGUAGE: (_if_in_semmle_code (f'cd "$SEMMLE_CODE"; MSYS2_ARG_CONV_EXCL="*" tools/bazel test //language-packs:intree-{{ LANGUAGE }}-as-test --test_output=errors') '# using codeql from PATH, if any')
17+
_maybe_build_dist LANGUAGE: (_if_in_semmle_code (f'cd "$SEMMLE_CODE"; tools/bazel test //language-packs:intree-{{ LANGUAGE }}-as-test --test_output=errors') '# using codeql from PATH, if any')
1818

1919
# Call bazel. Uses our official bazel wrapper if we are in an internal repository checkout
2020
[no-cd]
2121
[no-exit-message]
22-
_bazel *ARGS: (_if_in_semmle_code 'cd "$SEMMLE_CODE"; MSYS2_ARG_CONV_EXCL="*" tools/bazel' 'bazel' ARGS)
22+
_bazel *ARGS: (_if_in_semmle_code 'cd "$SEMMLE_CODE"; tools/bazel' 'bazel' ARGS)
2323

2424
# Call sembuild (requires an internal repository checkout)
2525
[no-cd]

‎misc/just/defs.just‎

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
# The first definition of a variable wins, not the last, so this order is what lets the
2+
# internal file set `SEMMLE_CODE` while the stub is the fallback when it is absent.
3+
# Swapped, the stub wins even where the real file exists and that checkout then behaves
4+
# as if it were external. Nothing here can catch that: with the file absent, as it
5+
# always is in this repository, both orders evaluate the same.
16
import? '../../../semmle-code.just' # internal repo just file, if present
27
import 'semmle-code-stub.just'
38

@@ -12,8 +17,43 @@ set allow-duplicate-variables
1217
export PATH_SEP := if os() == "windows" { ";" } else { ":" }
1318
export JUST_EXECUTABLE := just_executable()
1419

20+
# MSYS2 rewrites arguments that look like paths as it hands them to a native program,
21+
# which mangles bazel's `//target` labels. Set once here rather than at each bazel call:
22+
# a call that forgets it works everywhere except Windows, which is where nobody looks.
23+
# The price of setting it once is that it covers every command a recipe runs, so a tool
24+
# that wants Windows path conversion has to ask for it back.
25+
export MSYS2_ARG_CONV_EXCL := if os() == "windows" { "*" } else { "" }
26+
1527
error := f'{{ style("error") }}error{{ NORMAL }}: '
16-
cmd_sep := "\n#--------------------------------------------------------\n"
28+
29+
# The separator line, sized to the terminal.
30+
#
31+
# `stty` rather than `tput`: it needs no terminfo, so it survives `TERM=dumb`, and reads
32+
# the terminal from stdin, so it answers when stdout is piped. The arithmetic is in the
33+
# shell because just has no integers.
34+
#
35+
# With no terminal the value is non-numeric and this falls back to a fixed 57 columns.
36+
# That is a branch, not a platform test: just runs `sh` everywhere.
37+
#
38+
# `JUST_CMD_RULE` is the whole separator and lands in a shell script, so a hand-set value
39+
# has to be a shell comment, every line of it, or it runs. Left unchecked: setting it at
40+
# all means being able to run commands already, so there is nothing to defend.
41+
#
42+
# Exported and preferred over measuring: `shell()` runs on every parse, so a forwarded
43+
# verb would otherwise re-measure in every child it spawns, and `if` is lazy in its
44+
# branches. Inheritance needs a process, so a `mod` measures for itself; presetting
45+
# `JUST_CMD_RULE` skips measuring entirely.
46+
_given_horizontal_rule := env('JUST_CMD_RULE', '')
47+
48+
_horizontal_rule := if _given_horizontal_rule == '' { shell('''
49+
w=$(stty size 2>/dev/null | cut -d" " -f2)
50+
case "$w" in '' | *[!0-9]*) w=57 ;; esac
51+
printf "%*s" $w "" | tr " " '#'
52+
''') } else { _given_horizontal_rule }
53+
54+
export JUST_CMD_RULE := _horizontal_rule
55+
56+
cmd_sep := "\n" + _horizontal_rule + "\n"
1757
export CMD_BEGIN := style("command") + cmd_sep
1858
export CMD_END := cmd_sep + NORMAL
1959
export JUST_ERROR := error
@@ -25,7 +65,7 @@ default_db_checks := ['--check-databases', '--check-diff-informed', '--fail-on-t
2565
[no-exit-message]
2666
@_require_semmle_code:
2767
{{ if SEMMLE_CODE == "" { f'''
28-
echo "{error} running this recipe requires doing so from an internal repository checkout" >&2
68+
echo "{{ error }}running this recipe requires doing so from an internal repository checkout" >&2
2969
exit 1
3070
''' } else { "" } }}
3171

‎misc/just/format.just‎

Lines changed: 53 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -1,83 +1,74 @@
11
import "build.just"
22

3-
_ql_formatter := if SEMMLE_CODE != "" { '"$SEMMLE_CODE/target/intree/codeql-nolang/codeql"' } else { "codeql" }
4-
5-
_py_formatter := if SEMMLE_CODE != "" { "uv run black" } else { "black" }
6-
7-
_cpp_formatter := if SEMMLE_CODE != "" { "uv run clang-format" } else { "clang-format" }
8-
9-
# The `buildifier` bazel target always covers the whole workspace, so the binary behind
10-
# it is used instead and given paths. bazel is asked from the root of this repository,
11-
# even when it sits inside another, and the files are bounded to that root as well: each
12-
# repository then formats its own bazel files with the buildifier version it pins, and a
13-
# verb aimed at a tree spanning both is answered once by each. This is the opposite of
14-
# building, where a target needs the enclosing workspace to resolve at all, hence
15-
# `_bazel` in build.just going the other way.
16-
_bazel_workspace := quote(parent_directory(parent_directory(source_dir())))
17-
18-
_bazel_formatter := "bazel run --noshow_progress --ui_event_filters=,+error,+fail //misc/bazel/buildifier:binary --"
19-
20-
# bazel files are named rather than suffixed, and buildifier has no exclude option of its
21-
# own, so the files skipped by the target above are skipped here too. That target formats
22-
# the whole workspace at once and so cannot take the path this recipe is given, which is
23-
# why the two run the same binary through different entry points. Their exclusions are
24-
# therefore stated twice, in two places, in two syntaxes: keep them in step, or `just
25-
# format` rewrites what pre-commit and CI deliberately leave alone.
26-
#
27-
# bazel knows `BUILD` and `WORKSPACE` by those names and everything else by the `.bazel`
28-
# extension, so a file named `BUILD.<anything>` else is a template or a generator's input
29-
# rather than a bazel file, and is none of the formatter's business to parse.
3+
# These formatters take files rather than directories, so `run_on_files.py` collects them
4+
# and passes them positionally, which is what lets paths contain spaces. Both name every
5+
# file they were given, so the lines saying nothing happened are dropped: for buildifier
6+
# that is an empty warning tally, and a non-zero one is left alone, being the only notice
7+
# of what it could not fix now that the exit code is 0 either way.
308
#
31-
# Both lists are comma-separated, so a root defining its own `_root_format` can name
32-
# several patterns in one variable. It has no need to repeat the ones here: the files
33-
# they cover belong to this repository, which formats them itself. Exclusions match the
34-
# path as walked rather than as spelled on the command line, so one naming a directory
35-
# has to cover both the path it is reached by and the path it is walked from.
9+
# The collector announces the run, rather than each recipe saying what it is about to do.
10+
# A recipe reaches that line knowing neither whether any file matched nor which of the
11+
# paths it was handed hold one, and both are worth waiting for: a path whose files are all
12+
# excluded is the ordinary case here, as a repository formatting its own bazel files is
13+
# handed a nested repository's path too. The price of letting the collector speak is that
14+
# it names the command as it is really spelled, flags that only shape output and all. It
15+
# is a report rather than something to paste in any case, the collecting being the point;
16+
# `just -n` prints what actually runs, `@` or no `@`. `_format_py` echoes its own, having
17+
# no collector to defer to and nothing to decide.
3618
#
37-
# As with the QL formatter, buildifier only names what it rewrote if it also accounts for
38-
# every file it did not, so that accounting is dropped. It counts the warnings it could
39-
# not fix there, which are left for linting to report rather than raised on every format.
40-
_bazel_names := "BUILD,WORKSPACE,*.bazel,*.bzl,*.sky"
41-
42-
_bazel_excluded := ".git/*,*/.git/*,*misc/bazel/3rdparty/*_deps/*"
19+
# `[no-cd]` keeps a relative argument meaning the caller's directory: the forwarder passes
20+
# `--justfile`, so without it the default `.` would silently become the whole repository.
4321

44-
_bazel_accounting := ': applied fixes, [0-9]+ warnings left$'
45-
46-
# `codeql query format` and `clang-format` take files rather than directories, so the
47-
# files are collected by `run_on_files.py`. Arguments are passed positionally so that
48-
# paths containing spaces survive, of which this repository has many.
49-
#
50-
# The files that were rewritten are worth reporting, but `codeql query format` only
51-
# names those once it also names every file it leaves alone, which buries them under
52-
# thousands of lines. So it is asked for all of it and the lines about files it did not
53-
# touch are dropped. Only those are dropped, so errors still come through, as does
54-
# anything unforeseen.
55-
#
56-
# `[no-cd]` is what keeps a relative argument meaning the directory the caller is in.
57-
# The forwarder reaches a recipe above its argument with `--justfile`, which otherwise
58-
# runs it from that justfile's own directory: dropping the attribute would silently turn
59-
# the default `.` into the whole repository, and the only symptom would be slowness.
22+
_ql_formatter := (if SEMMLE_CODE != "" { '"$SEMMLE_CODE/target/intree/codeql-nolang/codeql"' } else { "codeql" }) + " query format --in-place"
6023

6124
[no-cd]
6225
[no-exit-message]
6326
[positional-arguments]
64-
_format_ql +ARGS: (_maybe_build_dist "nolang")
65-
{{ cmd_sep }}{{ py }} "{{ source_dir() }}/run_on_files.py" --drop '^Formatting ' --drop '^No change for ' "*.ql,*.qll" {{ _ql_formatter }} query format --in-place -v -- "$@"{{ cmd_sep }}
27+
@_format_ql +ARGS: (_maybe_build_dist "nolang")
28+
{{ py }} "{{ source_dir() }}/run_on_files.py" --drop '^Formatting ' --drop '^No change for ' "*.ql,*.qll" {{ _ql_formatter }} -v -- "$@"
29+
30+
_py_formatter := if SEMMLE_CODE != "" { "uv run black" } else { "black" }
6631

6732
[no-cd]
6833
[no-exit-message]
6934
[positional-arguments]
70-
_format_py *ARGS=".":
71-
{{ cmd_sep }}{{ _py_formatter }} "$@"{{ cmd_sep }}
35+
@_format_py *ARGS=".":
36+
echo "$CMD_BEGIN-> "{{ quote(_py_formatter) }}" $*$CMD_END" >&2
37+
{{ _py_formatter }} "$@"
38+
39+
_cpp_formatter := (if SEMMLE_CODE != "" { "uv run clang-format" } else { "clang-format" }) + " -i"
7240

7341
[no-cd]
7442
[no-exit-message]
7543
[positional-arguments]
76-
_format_cpp *ARGS=".":
77-
{{ cmd_sep }}{{ py }} "{{ source_dir() }}/run_on_files.py" "*.h,*.cpp" {{ _cpp_formatter }} -i -- "$@"{{ cmd_sep }}
44+
@_format_cpp *ARGS=".":
45+
{{ py }} "{{ source_dir() }}/run_on_files.py" "*.h,*.cpp" {{ _cpp_formatter }} -- "$@"
46+
47+
# The `buildifier` target formats a whole workspace and so cannot take a path, so its
48+
# binary is driven directly. bazel is asked from this repository's root even when it sits
49+
# inside another, and the files are bounded to it, so each repository formats its own with
50+
# the version it pins. Building needs the enclosing workspace instead, hence `_bazel`.
51+
_bazel_workspace := quote(parent_directory(parent_directory(source_dir())))
52+
53+
_bazel_formatter := (if SEMMLE_CODE != "" { '"$SEMMLE_CODE/tools/bazel"' } else { "bazel" }) + " run @buildifier_prebuilt//:buildifier"
54+
55+
# Kept out of the formatter, unlike the other three: bazel's own flags have to sit between
56+
# the target and `--`, so no single string spans both sides of it.
57+
_bazel_args := "-mode=fix -lint=fix"
58+
59+
# Keep both in step with the `buildifier` target. The names are what it recognises walking a
60+
# workspace, a check it skips for paths handed to it, so anything extra here is rewritten
61+
# regardless. Being wider is deliberate: what it recognises differs between versions, and
62+
# each repository pins its own, so only a list covering every one of them can be shared. A
63+
# directory of one file per candidate name, formatted with `-r`, says what a version takes.
64+
# Exclusions match the path as walked and its absolute form, so one that has to hold
65+
# however the path was reached is anchored absolutely.
66+
_bazel_names := "BUILD,WORKSPACE,*.bazel,*.bzl,*.sky,*.star"
67+
68+
_bazel_excluded := ".git/*,*/.git/*,*misc/bazel/3rdparty/*_deps/*"
7869

7970
[no-cd]
8071
[no-exit-message]
8172
[positional-arguments]
82-
_format_bazel *ARGS=".":
83-
{{ cmd_sep }}export MSYS2_ARG_CONV_EXCL="*"; {{ py }} "{{ source_dir() }}/run_on_files.py" --absolute --chdir {{ _bazel_workspace }} --within {{ _bazel_workspace }} --drop '{{ _bazel_accounting }}' --exclude '{{ _bazel_excluded }}' "{{ _bazel_names }}" {{ _bazel_formatter }} -mode=fix -lint=fix -v -- "$@"{{ cmd_sep }}
73+
@_format_bazel *ARGS=".":
74+
{{ py }} "{{ source_dir() }}/run_on_files.py" --absolute --chdir {{ _bazel_workspace }} --within {{ _bazel_workspace }} --drop ': applied fixes, 0 warnings left$' --exclude '{{ _bazel_excluded }}' "{{ _bazel_names }}" {{ _bazel_formatter }} --noshow_progress --ui_event_filters=,+error,+fail -- {{ _bazel_args }} -v -- "$@"

‎misc/just/lib.just‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,5 +27,5 @@ import "format.just"
2727
[no-exit-message]
2828
[positional-arguments]
2929
@_integration_test *ARGS: _require_semmle_code
30-
echo "$CMD_BEGIN$SEMMLE_CODE/tools/pytest --codeql=build-as-test $*$CMD_END"
30+
echo "$CMD_BEGIN-> $SEMMLE_CODE/tools/pytest --codeql=build-as-test $*$CMD_END" >&2
3131
"$SEMMLE_CODE/tools/pytest" --codeql=build-as-test "$@"

0 commit comments

Comments
 (0)