Skip to content

Commit a7fb6e0

Browse files
redsun82Copilot
andcommitted
Just: let a verb fail when it could not read a justfile
A candidate that failed to parse was reported and then dropped, so a broad verb ran everything else and still exited zero: a malformed justfile removed its own work from CI while the diagnostic scrolled past. Failure now travels back through resolution and no partial run is started on top of it. Carrying the recipe rather than just its name also lets arguments be grouped by what the recipe can actually be called with. Nothing reachable today needs that, as every verb resolves to a variadic or zero-argument recipe, but the arguments are paths and a fixed-arity recipe would otherwise fail on arity alone. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent e4433b1 commit a7fb6e0

2 files changed

Lines changed: 152 additions & 45 deletions

File tree

‎misc/just/forward_command.py‎

Lines changed: 76 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -96,16 +96,20 @@ def list_value(assignments, name):
9696
return []
9797

9898

99+
def is_variadic(recipe):
100+
parameters = recipe["parameters"]
101+
return bool(parameters and parameters[-1]["kind"] in ("star", "plus"))
102+
103+
99104
def accepts(recipe, argc):
100105
"""Check whether a recipe can be called with a given number of arguments."""
101106
parameters = recipe["parameters"]
102-
variadic = parameters and parameters[-1]["kind"] in ("star", "plus")
103107
required = sum(
104108
1
105109
for parameter in parameters
106110
if parameter["default"] is None and parameter["kind"] != "star"
107111
)
108-
return required <= argc and (variadic or argc <= len(parameters))
112+
return required <= argc and (is_variadic(recipe) or argc <= len(parameters))
109113

110114

111115
def implements(dump, command, argc):
@@ -141,12 +145,14 @@ def dump_all(justfiles):
141145
with ThreadPoolExecutor(PROBE_WORKERS) as executor:
142146
dumps = list(executor.map(dump_justfile, justfiles))
143147
parsed = []
148+
failed = False
144149
for justfile, (dump, failure) in zip(justfiles, dumps):
145150
if dump is None:
151+
failed = True
146152
error(f"could not read {justfile}:\n{failure}")
147153
else:
148154
parsed.append((justfile, dump))
149-
return parsed
155+
return parsed, failed
150156

151157

152158
def git(directory, *args):
@@ -159,28 +165,26 @@ def git(directory, *args):
159165
)
160166
if result.returncode != 0:
161167
error(f"`git {' '.join(args)}` failed in {directory}:\n{result.stderr.strip()}")
162-
return []
163-
return result.stdout.splitlines()
168+
return [], True
169+
return result.stdout.splitlines(), False
164170

165171

166172
def submodules(directory):
167173
"""List the initialised submodules under a directory."""
168-
toplevel = git(directory, "rev-parse", "--show-toplevel")
174+
toplevel, failed = git(directory, "rev-parse", "--show-toplevel")
169175
if not toplevel or not (Path(toplevel[0]) / ".gitmodules").exists():
170-
return []
171-
paths = [
172-
Path(toplevel[0]) / line.split(" ", 1)[1]
173-
for line in git(
174-
toplevel[0], "config", "--file", ".gitmodules", "--get-regexp", r"\.path$"
175-
)
176-
]
176+
return [], failed
177+
lines, config_failed = git(
178+
toplevel[0], "config", "--file", ".gitmodules", "--get-regexp", r"\.path$"
179+
)
180+
paths = [Path(toplevel[0]) / line.split(" ", 1)[1] for line in lines]
177181
within = Path(directory).resolve()
178182
return [
179183
Path(directory) / os.path.relpath(path, within)
180184
for path in paths
181185
# An uninitialised submodule is an empty directory, with nothing to run.
182186
if path.is_relative_to(within) and (path / ".git").exists()
183-
]
187+
], failed or config_failed
184188

185189

186190
def find_justfiles(directory):
@@ -191,21 +195,21 @@ def find_justfiles(directory):
191195
worth finding.
192196
"""
193197
justfiles = set()
194-
for repository in [directory, *submodules(directory)]:
195-
justfiles.update(
196-
Path(repository) / line
197-
for line in git(
198-
repository,
199-
"ls-files",
200-
"--cached",
201-
"--others",
202-
"--exclude-standard",
203-
"--",
204-
"justfile",
205-
"*/justfile",
206-
)
198+
repositories, failed = submodules(directory)
199+
for repository in [directory, *repositories]:
200+
lines, git_failed = git(
201+
repository,
202+
"ls-files",
203+
"--cached",
204+
"--others",
205+
"--exclude-standard",
206+
"--",
207+
"justfile",
208+
"*/justfile",
207209
)
208-
return justfiles
210+
failed = failed or git_failed
211+
justfiles.update(Path(repository) / line for line in lines)
212+
return justfiles, failed
209213

210214

211215
def invocation_path(path, *, like):
@@ -230,7 +234,8 @@ def find_justfiles_above(command, arg):
230234
]
231235
found = []
232236
seen = []
233-
for justfile, dump in dump_all(candidates):
237+
parsed, failed = dump_all(candidates)
238+
for justfile, dump in parsed:
234239
# A justfile sitting exactly on the argument is called without it, as the
235240
# argument would only repeat where it already is.
236241
argc = 0 if justfile.parent.resolve() == directory else 1
@@ -248,7 +253,7 @@ def find_justfiles_above(command, arg):
248253
if recipe is not None and recipe not in seen:
249254
seen.append(recipe)
250255
found.append((justfile, recipe))
251-
return found
256+
return found, failed
252257

253258

254259
def find_justfiles_below(command, directory, covered=()):
@@ -263,10 +268,13 @@ def find_justfiles_below(command, directory, covered=()):
263268
but ask to be named rather than found.
264269
"""
265270
# The justfile at `directory` is covered by the search above it.
266-
candidates = sorted(find_justfiles(directory) - {Path(directory) / "justfile"})
271+
justfiles, failed = find_justfiles(directory)
272+
candidates = sorted(justfiles - {Path(directory) / "justfile"})
267273
matches = []
268274
opted_out = []
269-
for justfile, dump in dump_all(candidates):
275+
parsed, dump_failed = dump_all(candidates)
276+
failed = failed or dump_failed
277+
for justfile, dump in parsed:
270278
recipe = implements(dump, command, 0)
271279
if recipe is None:
272280
continue
@@ -283,7 +291,7 @@ def find_justfiles_below(command, directory, covered=()):
283291
continue
284292
contributed.setdefault(justfile.parent, []).append(recipe)
285293
found.append((justfile, recipe))
286-
return sorted(found, key=lambda match: match[0]), sorted(opted_out)
294+
return sorted(found, key=lambda match: match[0]), sorted(opted_out), failed
287295

288296

289297
def resolve(command, arg):
@@ -294,18 +302,18 @@ def resolve(command, arg):
294302
on. One found below gets its own directory instead, as there the argument only said
295303
where to look. Justfiles below that asked to be named are returned separately.
296304
"""
297-
above = find_justfiles_above(command, arg)
298-
resolved = [(justfile, arg, recipe["name"]) for justfile, recipe in above]
305+
above, failed = find_justfiles_above(command, arg)
306+
resolved = [(justfile, arg, recipe) for justfile, recipe in above]
299307
opted_out = []
300308
if os.path.isdir(arg):
301-
below, opted_out = find_justfiles_below(
309+
below, opted_out, below_failed = find_justfiles_below(
302310
command, arg, [recipe for _, recipe in above]
303311
)
312+
failed = failed or below_failed
304313
resolved += [
305-
(justfile, str(justfile.parent), recipe["name"])
306-
for justfile, recipe in below
314+
(justfile, str(justfile.parent), recipe) for justfile, recipe in below
307315
]
308-
return resolved, opted_out
316+
return resolved, opted_out, failed
309317

310318

311319
def report_opted_out(command, justfiles, *, ran):
@@ -340,6 +348,19 @@ def invoke_just(cwd, args):
340348
return 0
341349

342350

351+
def invocation_argument_groups(recipe, pos_args):
352+
"""Split arguments into what the recipe can be called with at once.
353+
354+
Every recipe reachable from a verb today is variadic or takes none, so this only
355+
ever yields one group. It is here because the arguments are paths and running the
356+
verb once per path is what a fixed-arity recipe would mean, where passing them
357+
together would fail on arity alone and say nothing useful about why.
358+
"""
359+
if is_variadic(recipe) or len(pos_args) <= 1:
360+
return [pos_args]
361+
return [[arg] for arg in pos_args]
362+
363+
343364
def forward(cmd, args):
344365
"""Forward a command to language-specific justfiles."""
345366
is_non_positional = re.compile(r"^(-.*|\+|[A-Z_][A-Z_0-9]*=.*)$")
@@ -348,27 +369,38 @@ def forward(cmd, args):
348369

349370
justfiles = {}
350371
opted_out = []
372+
resolution_failed = False
351373
for arg in positional_args or ["."]:
352-
resolved, skipped = resolve(cmd, arg)
374+
resolved, skipped, failed = resolve(cmd, arg)
353375
opted_out += skipped
376+
resolution_failed = resolution_failed or failed
354377
if not resolved:
378+
# A candidate that could not be read is reported below rather than here:
379+
# saying nothing matched would blame the argument for a broken justfile.
380+
if failed:
381+
continue
355382
error(f"No justfile found for {cmd} on {arg}")
356383
report_opted_out(cmd, skipped, ran=False)
357384
return 1
358385
for justfile, justfile_arg, recipe in resolved:
359386
justfiles.setdefault(justfile, (recipe, []))[1].append(justfile_arg)
360387

388+
if resolution_failed:
389+
report_opted_out(cmd, opted_out, ran=False)
390+
return 1
391+
361392
invocations = []
362393
for justfile, (recipe, pos_args) in justfiles.items():
363394
# An argument standing for the whole directory subsumes any more specific one
364395
# that ended up on the same justfile.
365396
whole_directory = str(justfile.parent)
366397
if whole_directory in pos_args:
367398
pos_args = [whole_directory]
368-
cwd, just_args = get_just_context(justfile, recipe, flags, pos_args)
369-
prefix = f"cd {cwd}; " if cwd else ""
370-
print(f"-> {prefix}just {' '.join(just_args)}")
371-
invocations.append((cwd, just_args))
399+
for group in invocation_argument_groups(recipe, pos_args):
400+
cwd, just_args = get_just_context(justfile, recipe["name"], flags, group)
401+
prefix = f"cd {cwd}; " if cwd else ""
402+
print(f"-> {prefix}just {' '.join(just_args)}")
403+
invocations.append((cwd, just_args))
372404

373405
report_opted_out(cmd, opted_out, ran=True)
374406

‎misc/just/test_forward_command.py‎

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,11 @@ def found(self, outer, inner):
290290
"dump_justfile",
291291
side_effect=lambda justfile: (dumps[Path(justfile)], None),
292292
):
293-
return forward_command.find_justfiles_above("format", str(self.argument))
293+
found, failed = forward_command.find_justfiles_above(
294+
"format", str(self.argument)
295+
)
296+
self.assertFalse(failed)
297+
return found
294298

295299
def test_a_recipe_reached_under_two_spellings_runs_once(self):
296300
found = self.found(self.implementing(), self.implementing())
@@ -308,6 +312,77 @@ def test_two_roots_sharing_a_comment_still_both_run_if_they_do_different_work(se
308312
self.assertEqual([justfile for justfile, _ in found], [self.inner, self.outer])
309313

310314

315+
class TestDiscoveryFailures(unittest.TestCase):
316+
def test_dump_all_reports_every_failure(self):
317+
justfiles = [Path("one/justfile"), Path("two/justfile"), Path("three/justfile")]
318+
dumps = [(None, "bad one"), (dump(), None), (None, "bad three")]
319+
with mock.patch.object(
320+
forward_command, "dump_justfile", side_effect=dumps
321+
), mock.patch.object(forward_command, "error") as report:
322+
parsed, failed = forward_command.dump_all(justfiles)
323+
self.assertTrue(failed)
324+
self.assertEqual(parsed, [(Path("two/justfile"), dump())])
325+
self.assertEqual(report.call_count, 2)
326+
327+
def test_git_failure_is_reported_to_callers(self):
328+
result = subprocess.CompletedProcess(
329+
["git"], 128, stdout="", stderr="fatal: not a repository"
330+
)
331+
with mock.patch.object(
332+
subprocess, "run", return_value=result
333+
), mock.patch.object(forward_command, "error"):
334+
lines, failed = forward_command.git(".", "ls-files")
335+
self.assertEqual(lines, [])
336+
self.assertTrue(failed)
337+
338+
def test_forward_fails_without_running_after_resolution_failure(self):
339+
justfile = Path("pkg/justfile")
340+
resolved = [(justfile, "pkg", recipe("test", [parameter("ARGS", "star")]))]
341+
with mock.patch.object(
342+
forward_command, "resolve", return_value=(resolved, [], True)
343+
), mock.patch.object(forward_command, "invoke_just") as invoke:
344+
self.assertEqual(forward_command.forward("test", ["pkg"]), 1)
345+
invoke.assert_not_called()
346+
347+
348+
class TestForwardInvocations(unittest.TestCase):
349+
def test_accumulates_variadic_recipe_arguments(self):
350+
justfile = Path("pkg/justfile")
351+
test_recipe = recipe("test", [parameter("ARGS", "star")])
352+
353+
def resolve(command, arg):
354+
return [(justfile, arg, test_recipe)], [], False
355+
356+
with mock.patch.object(
357+
forward_command, "resolve", side_effect=resolve
358+
), mock.patch.object(forward_command, "invoke_just", return_value=0) as invoke:
359+
self.assertEqual(forward_command.forward("test", ["pkg/a", "pkg/b"]), 0)
360+
361+
invoke.assert_called_once_with(
362+
None, ["--justfile", "pkg/justfile", "test", "pkg/a", "pkg/b"]
363+
)
364+
365+
def test_splits_non_variadic_recipe_arguments(self):
366+
justfile = Path("pkg/justfile")
367+
test_recipe = recipe("test", [parameter("ARG")])
368+
369+
def resolve(command, arg):
370+
return [(justfile, arg, test_recipe)], [], False
371+
372+
with mock.patch.object(
373+
forward_command, "resolve", side_effect=resolve
374+
), mock.patch.object(forward_command, "invoke_just", return_value=0) as invoke:
375+
self.assertEqual(forward_command.forward("test", ["pkg/a", "pkg/b"]), 0)
376+
377+
self.assertEqual(
378+
invoke.call_args_list,
379+
[
380+
mock.call(None, ["--justfile", "pkg/justfile", "test", "pkg/a"]),
381+
mock.call(None, ["--justfile", "pkg/justfile", "test", "pkg/b"]),
382+
],
383+
)
384+
385+
311386
# Resolve the same binary `forward_command` will run: it honours JUST_EXECUTABLE, so a
312387
# pinned `just` would otherwise have these fixtures checked against a different binary
313388
# than the code uses. `which` covers both spellings, returning an explicit path as given

0 commit comments

Comments
 (0)