task icon indicating copy to clipboard operation
task copied to clipboard

.task/checksum is not created after running a task whose status checked returned non-zero

Open abrahammurciano opened this issue 3 years ago • 5 comments

  • Task version: v3.12.1 and v3.14.1
  • Operating System: Ubuntu 18

Example Taskfile showing the issue

version: '3'
tasks:
  default:
    cmds:
      - "touch ./out.txt"
      - "echo running"
    status:
      - test -f ./out.txt
    sources:
      - ./src/**

Steps to reproduce

❯ tree . -a
.
├── src
│   └── foo.txt
└── Taskfile.yaml

❯ task -vvv
task: "default" started
task: status command test -f ./out.txt exited non-zero: exit status 1
task: [default] touch ./out.txt
task: [default] echo running
running
task: "default" finished

❯ tree . -a # NOTE .task/checksum/ is missing
.
├── out.txt
├── src
│   └── foo.txt
└── Taskfile.yaml

❯ task -vvv # NOTE default task ran again erroneously because .task/checksum/ doesn't exist
task: "default" started
task: status command test -f ./out.txt exited zero
task: [default] touch ./out.txt
task: [default] echo running
running
task: "default" finished

❯ tree . -a # This time .task was created successfully
.
├── out.txt
├── src
│   └── foo.txt
├── .task
│   └── checksum
│       └── default
└── Taskfile.yaml

❯ task -vvv # This time it correctly skips it because .task/checksum exists
task: "default" started
task: status command test -f ./out.txt exited zero
task: Task "default" is up to date

I suspect the issue has to do with the fact that the first time status command exited 1. If I remove the status command or replace it with "true" the .task/checksum is created the first time.

abrahammurciano avatar Aug 16 '22 14:08 abrahammurciano

Is the problem perhaps that status and sources are two incompatible ways to do the same thing - decide whether the task needs to be run or not.

Assuming Task is supposed to accommodate both being present () , what semantics should they have? () This isn't proven; the bug might be a missing validation rule rejecting the task if both are present.

  • Run the task if either status or sources says it needs to be run
  • Run the task only if both status and sources say it needs to be run

If we choose the first (or) case, which gets evaluated first? Choosing to skip status might be a breaking change, depending on whether the shell command has any side effects, indicating we might want to evaluate that first.

theunrepentantgeek avatar Aug 19 '22 04:08 theunrepentantgeek

The current behaviour of task already answers all of those questions.

As can be seen in my second invocation above, The status check exited zero indicating up to date, but the checksum didn't exist indicating not up to date, and task concluded that the task was not up to date. Thus the task should run if either status or sources says it needs to be run.

Also, as can be seen in my examples, status commands are currently always run regardless of sources being up to date or not.

abrahammurciano avatar Aug 19 '22 08:08 abrahammurciano

The bug here is that after the first successful run, the .task/checksum folder wasn't created.

abrahammurciano avatar Aug 19 '22 08:08 abrahammurciano

I agree, task should evaluate both status and sources/generates, and run if either either don't meet expectations. I'll have to look at the code, but I suspect that whatever is doing this evaluation is simply returning as early as possible (e.g. status is happening first).

ghostsquad avatar Aug 21 '22 04:08 ghostsquad

I agree, task should evaluate both status and sources/generates, and run if either either don't meet expectations. I'll have to look at the code, but I suspect that whatever is doing this evaluation is simply returning as early as possible (e.g. status is happening first).

Can confirm that's not the case. Both are always checked and the task is run if either says it should run.

Again, I emphasize the bug here is that after the first successful run, the .task/checksum folder wasn't created.

abrahammurciano avatar Aug 21 '22 06:08 abrahammurciano

Thank you @abrahammurciano for reporting this. I've just ran into the same issue.

Any updates on that ?

This is not the expected behavior, also according to the docs, which provide an example of the combination.

It's a shame, as this means that for example, CI builds will run twice before being skipped up to date.

harelwa avatar Mar 03 '23 20:03 harelwa

I agree, task should evaluate both status and sources/generates, and run if either either don't meet expectations. I'll have to look at the code, but I suspect that whatever is doing this evaluation is simply returning as early as possible (e.g. status is happening first).

@ghostsquad perhaps this issue is worthy of a bug label

harelwa avatar Mar 03 '23 20:03 harelwa

Labels added

ghostsquad avatar Mar 03 '23 20:03 ghostsquad

Thank you.

This issue is evident in the code -

First run of task will only check status ( and return .. ) -

https://github.com/go-task/task/blob/d4ed7c3cfc2408bf0978b9d693ff226e3b9c0707/status.go#L36

Second run will get to IsUpToDate ( as status is up to date ) --

https://github.com/go-task/task/blob/d4ed7c3cfc2408bf0978b9d693ff226e3b9c0707/status.go#L51

and IsUpToDate is where the checksum files are created.

thus they are not created in the first time.

harelwa avatar Mar 03 '23 21:03 harelwa

disclaimer I'm not a go programmer.

but it should be something like this ( and this works running locally ) -

func (e *Executor) isTaskUpToDate(ctx context.Context, t *taskfile.Task) (bool, error) {
	if len(t.Status) == 0 && len(t.Sources) == 0 {
		return false, nil
	}

	isUpToDateStatus := true
	isUpToDateChecker := true

	if len(t.Status) > 0 {
		isUpToDate, err := e.isTaskUpToDateStatus(ctx, t)
		if err != nil {
			return false, err
		}
		if !isUpToDate {
			isUpToDateStatus = false
		}
	}

	if len(t.Sources) > 0 {
		checker, err := e.getStatusChecker(t)
		if err != nil {
			return false, err
		}
		isUpToDate, err := checker.IsUpToDate()
		if err != nil {
			return false, err
		}
		if !isUpToDate {
			isUpToDateChecker = false
		}
	}

	isUpToDate := isUpToDateStatus && isUpToDateChecker

	return isUpToDate, nil
}

If you find that acceptable, i'd be happy to contribute that ( pls ignore formatting, it's the formatted tabs )

harelwa avatar Mar 03 '23 21:03 harelwa

It seems that one of these functions has a side effect of writing the checksum file? I don't like that. That's likely the source of this bug (and possibly others).

ghostsquad avatar Mar 03 '23 23:03 ghostsquad