Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature request: idempotentcy guards #2308

Closed
samrees opened this issue Aug 9, 2024 · 11 comments
Closed

feature request: idempotentcy guards #2308

samrees opened this issue Aug 9, 2024 · 11 comments

Comments

@samrees
Copy link

samrees commented Aug 9, 2024

one of the handiest things in executing a dependency graph of tasks would be explicit idempotency guards.

I'm pretty sure rake had them, but Chef might have the best explanation: https://docs.chef.io/resource_common/#guards

basically they're declarations of only_if or not_if and then some chunk of executable code. If the guard passes, task runs.

Very often my tasks are dominated by code that checks if they're still valid. Things like: ensuring that keys/tokens are valid and not expired before doing expensive operations to swap them out.

With guard functionality, it would be an explicit place where we could technically re-implement Makes "if this list of input files has not changed and we have an output file on disk, dont re-compile." Which would make Just finally the complete better implementation needed to put Make to rest, RIP.

@casey
Copy link
Owner

casey commented Aug 9, 2024

This would definitely be useful, although I wonder what the syntax and implementation would look like.

One slightly weird idea which wouldn't require any new syntax would be to allow returning a specific exit code to indicate that further recipe lines should not run:

foo:
  test -f a.out && exit 200
  cc main.c

Here, returning 200 would indicate that the rest of the recipe should not run, but the recipe should still be considered to have run successfully.

This is nice because the implementation is extremely simple and there's no new syntax. However, if a recipe line ever returns 200 as a bona-fide exit code, just would report success, when in fact the recipe has failed.

@casey
Copy link
Owner

casey commented Aug 9, 2024

One possibility would be to put this behind a setting:

set skip-status-code := "200"

foo:
  test -f a.out && exit 200
  cc main.c

So the user is opting into this behavior, and will make sure none of their commands return 200 in the case of a bona-fide error. However, this would still be error prone, since in reality, users have no idea what status codes their commands return for errors.

@samrees
Copy link
Author

samrees commented Aug 10, 2024

Syntax wise I think its just a different way of combining dependent recipes. I think the error code is a step backwards of what I'm currently doing. Let me share a snippet of a mostly real example of how I wire up/layer to do aws centric tasks just using the dependency tree.

_ensure-aws acct:
  @[[ $([[ -e ~/.aws/credentials ]] && date -ju -f "%Y-%m-%dT%H:%M:%SZ" $(awk '/\[/{prefix=$0; next} $1{print prefix $0}' ~/.aws/credentials | grep "\[$acct\]aws_expiration" | cut -d = -f2) +%s 2> /dev/null || echo 0) -gt $(($(date -j '+%s')+600)) ]] || clisso get $acct

_ensure-all-aws: (_ensure-aws "hub") (_ensure-aws "vendor1") (_ensure-aws "vendor2")

_pub_ip tag: (_ensure-aws "hub")
  @AWS_PROFILE=hub AWS_REGION=$CLUSTER_REGION aws ec2 describe-instances --filters "Name=tag:Name,Values=$tag" "Name=instance-state-name,Values=running" --query "Reservations[*].Instances[*].PublicIpAddress" --output text | head -1
  
ssh-foo command="":
  ssh -J user@$(just _pub_ip "bastion") ec2-user@$(just _priv_ip "foo") "{{command}}"
  
redo-foo-thing:
  @just ssh-foo "{ big-long-command; }"

I can keep building up the recipe tree making these mostly reusable recipes. The main thing is the user doesn't know the difference between dependent recipes and recipes I'm actually just using as guards. Its a clutter that can make that top of my Justfiles very long, and without some kind of visualization or syntax a reader has a hard time piecing it back together. Also I dont think I can dynamically adjust arguments to dependent tasks in this form, whereas a guard I'd expect it to get access to the recipes arguments and be able to adjust downstream recipes from there.

Readability and dynamicism make me want the separate syntax of chef's guards. I'll also mention like Just recipes, chef's guards allowed multiple interpreters: https://docs.chef.io/resource_common/#guard-interpreters I'll also link for reference elixir's guards as they're a different, but similarly syntactically delightful form of dependent execution.
https://hexdocs.pm/elixir/main/patterns-and-guards.html#why-guards

@casey
Copy link
Owner

casey commented Aug 10, 2024

Those examples are pretty complicated. Can you provide a simpler example, perhaps that just tests if a file exists or not, and the syntax you would propose for guards?

@samrees
Copy link
Author

samrees commented Aug 13, 2024

Alright. so this might be the kind of issue that i just close, because I went through all the syntax again that already exists and I can see theres very few elegant ways of doing it.

_file_exists file:
  @[[ -f {{file}} ]]

test: build
not_if: (_file_exists "test_output.txt")
  ./test

I would imagine there are symmetrical not_if and only_if additions, and this would presume they have to be on a single line.

@casey
Copy link
Owner

casey commented Aug 13, 2024

This syntax:

test: build
not_if: (_file_exists "test_output.txt")
  ./test

Is ambiguous, since it is currently already a valid justfile, with recipes test and not_if with dependency _file_exists.

@samrees
Copy link
Author

samrees commented Aug 13, 2024

I appreciate your time looking at this.

@samrees samrees closed this as completed Aug 13, 2024
@casey
Copy link
Owner

casey commented Aug 13, 2024

I actually think this should be left open. I'm not sure what the right syntax or implementation would be, but I think it's useful.

@casey casey reopened this Aug 13, 2024
@laniakea64
Copy link
Contributor

laniakea64 commented Aug 14, 2024

Not sure I'm completely understanding this feature request, but might one possibility be something like this? -

_file_exists file:
  @[[ -f {{file}} ]]

test_only_if: ?(_file_exists "test_output.txt") build
  ./test

test_not_if: ?!(_file_exists "test_output.txt") build
  ./test

just would evaluate dependencies in order, same as it does now. When it reaches a dependency that's prefixed with ?, that dependency recipe is treated as a condition check. If the condition check dependency recipe failed: don't run any more of the requested recipe's dependencies, don't run the requested recipe, but continue as if the requested recipe had succeeded.

Similarly, not_if (requiring that the condition check recipe DOES fail) could be a ?! prefix.

Advantage: flexibility. It automatically allows more than one condition check. And these condition checks can be inserted anywhere in the dependencies list, so some dependencies could run prior to a condition check if desired. Or the condition check could be in the list of subsequent dependencies, if it's gating whether something should run after the main recipe.

Disadvantage: this type of condition wouldn't be able to alter queued recipes' parameters on the fly -

dynamically adjust arguments to dependent tasks in this form, whereas a guard I'd expect it to get access to the recipes arguments and be able to adjust downstream recipes from there.

Although that might have workarounds (e.g. the condition recipe could write something to a file for queued recipes to read).

Potential catch: With the way just seems to read justfiles, it might require extra attention to ensure confusing forms like the following are invalid:

# the ? is a prefix operator, it does NOT apply to "pre"
# it applies to the "file_exists" dependency
only_if: pre? (_file_exists "test_output.txt") build
  ./test

# not obvious at a glance whether `?` goes with "pre" or "check"
# or is some operator with a left-hand side and a right-hand side
# if writing clearly, these dependencies should be written as:
# pre ?check build
test_ambiguous1: pre ? check build
  ./test
test_ambiguous2: pre?check build
  ./test

Or maybe the fact this concern is even coming up indicates this syntax is a terrible idea in just - if so, sorry for the noise.

@casey
Copy link
Owner

casey commented Aug 14, 2024

Yah, having that kind of subtle relationship between what a dependency does and what the parent recipe does is probably tricky, especially as it relates to ordering. Also, I think guards can probably always be written like so:

guard:
  if [ ! -f foo ]; then \
    # do stuff \
  fi

I think the main motivation of guards would be to avoid putting the whole recipe inside of a conditional. So I actually think that syntax like this would be nice:

guard:
  ?test ! -f foo
  # do stuff

Where a ? prefix stops running a linewise recipe early if a line fails, but does not fail the recipe.

But, you can always switch to a shebang recipe to allow early, non-failing return:

guard:
  #!/usr/bin/env bash
  test ! -f foo && exit 0
  # do stuff

Which is nearly as good, so maybe there really isn't much of a hole to fill.

@casey casey closed this as completed Aug 14, 2024
@laniakea64
Copy link
Contributor

I actually think that syntax like this would be nice:

guard:
  ?test ! -f foo
  # do stuff

Where a ? prefix stops running a linewise recipe early if a line fails, but does not fail the recipe.

➡️ #2544

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants