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

tools/flow: cue task not execute, when result in non-task value #2965

Closed
morlay opened this issue Mar 19, 2024 · 10 comments
Closed

tools/flow: cue task not execute, when result in non-task value #2965

morlay opened this issue Mar 19, 2024 · 10 comments
Labels
NeedsInvestigation Triage Requires triage/attention

Comments

@morlay
Copy link

morlay commented Mar 19, 2024

What version of CUE are you using (cue version)?

$ cue version
v0.8.0

Does this issue reproduce with the latest stable release?

yes

What did you do?

-- main.go --
package main

import (
	"context"
	"log"
	"os"

	"cuelang.org/go/cue"
	"cuelang.org/go/cue/cuecontext"
	"cuelang.org/go/tools/flow"
)

func main() {
	ctx := cuecontext.New()
	f, err := os.ReadFile("x.cue")
	if err != nil {
		log.Fatal(err)
	}
	v := ctx.CompileBytes(f)
	if err := v.Err(); err != nil {
		log.Fatal(err)
	}

	controller := flow.New(&flow.Config{FindHiddenTasks: true}, v, ioTaskFunc)

	if err := controller.Run(context.Background()); err != nil {
		log.Fatal(err)
	}
}

func ioTaskFunc(v cue.Value) (flow.Runner, error) {
	inputPath := cue.ParsePath("$task")
	input := v.LookupPath(inputPath)

	if !input.Exists() {
		return nil, nil
	}

	return flow.RunnerFunc(func(t *flow.Task) error {
		return nil
	}), nil
}
-- x.cue --
package main

actions: {
	pre: #Run & {}

	issue_dep: [
		// here, the result is used not in other tasks.
		// it could be prepare data for other tasks.
		"\(pre.output)",
	]
}

#Run: {
	$task: "run"
	command?: [...string]
	output: string
}

What did you expect to see?

task actions.pre execute success.

What did you see instead?

got an error before task execute.

$ go run main.go
main.go:27: actions.issue_dep.0: invalid interpolation: non-concrete value string (type string) (and 1 more errors)
@morlay morlay added NeedsInvestigation Triage Requires triage/attention labels Mar 19, 2024
@rogpeppe
Copy link
Member

I've bisected this regression to this CL: https://review.gerrithub.io/c/cue-lang/cue/+/1177330

@rogpeppe
Copy link
Member

Firstly, thanks very much for the report and the reproducer. That's really useful!

Although this is technically a regression, ISTM that it may be because the above-mentioned fix actually exposes some genuine issues with your command flow. In particular, I'm not sure if it's allowed for non-task CUE code to depend of task-computed values, as you've got here. The documentation says:

Tasks may depend on other tasks.

but it doesn't say that non-tasks may depend on tasks.

One workaround that you might be able to use here is to use a let expression rather than a field. For example, this adjustment to the code makes it work:

package main

actions: {
	pre: #Run & {}

	let issue_dep =  [
		// here, the result is used not in other tasks.
		// it could be prepare data for other tasks.
		"\(pre.output)",
	]
	post: #Run & {
		command: issue_dep
	}
}

#Run: {
	$task: "run"
	command?: [...string]
	output: string
}

If that doesn't work for you, perhaps you might be able to expand on what you're trying to do here to guide any potential fix?

@morlay
Copy link
Author

morlay commented Mar 19, 2024

@rogpeppe here is the real world usage.

https://github.com/octohelm/piper/blob/main/piper.cue#L33

We need fields to define task preset, which based on one or more seed tasks.

Example

https://github.com/octohelm/piper/blob/main/cuepkg/golang/project.cue

@rogpeppe
Copy link
Member

rogpeppe commented Mar 20, 2024

Here is a test case that I think demonstrates the problem a little better:

exec go mod tidy
exec go run .
cmp stdout want-stdout
-- want-stdout --
final value: {
	param: "some output"
	t1: {
		$task:  true
		arg:    "some output"
		output: "some output"
	}
	t2: {
		arg: "some output"
	}
}
-- go.mod --
module test

go 1.22.1

require cuelang.org/go v0.8.0

-- main.go --
package main

import (
	"context"
	"fmt"
	"log"
	"os"

	"cuelang.org/go/cue"
	"cuelang.org/go/cue/cuecontext"
	"cuelang.org/go/tools/flow"
)

func main() {
	ctx := cuecontext.New()
	f, err := os.ReadFile("x.cue")
	if err != nil {
		log.Fatal(err)
	}
	v := ctx.CompileBytes(f)
	if err := v.Err(); err != nil {
		log.Fatal(err)
	}

	controller := flow.New(nil, v, func(v cue.Value) (flow.Runner, error) {
		if !v.LookupPath(cue.MakePath(cue.Str("$task"))).Exists() {
			return nil, nil
		}
		return flow.RunnerFunc(func(t *flow.Task) error {
			v := t.Value().FillPath(cue.MakePath(cue.Str("output")), "some output")
			t.Fill(v)
			return nil
		}), nil
	})

	if err := controller.Run(context.Background()); err != nil {
		log.Fatalf("controller run: %v", err)
	}
	final := controller.Value().LookupPath(cue.MakePath(cue.Str("b")))
	fmt.Printf("final value: %v\n", final)
}

-- x.cue --
#task: {
	$task: true
	arg?: string
	param?: string
	output?: string
}

a: #task
b: #tasks & {
	param: "\(a.output)"
}
// #tasks defines a collection of tasks, all of which share
// a common parameter that could be derived from the output
// of another task.
#tasks: {
	param: string
	t1: #task & {
		arg: param
	}
	t2: {
		arg: param
	}
}

This fails, but passes when the CUE version is set to v0.7.1.

@mvdan mvdan moved this to v0.8.1 in Release v0.8 Mar 20, 2024
@mvdan
Copy link
Member

mvdan commented Mar 20, 2024

cc @nickfiggins who wrote the patch which introduced this regression - do you have any thoughts on Roger's example above, and what might be the best fix? For now we're thinking of doing a revert in v0.8.1 to un-break users like @morlay, keeping your added test case deactivated until we can figure out a better fix that keeps both test cases passing.

@rogpeppe rogpeppe changed the title tools/flow: cue task not execute, when result in non-task value. tools/flow: incomplete value not part of a task results in inappropriate error Mar 20, 2024
cueckoo pushed a commit that referenced this issue Mar 20, 2024
This reverts commit 9ef35eb
but keeps the `cmd_undefined` test case and updates
the `issue2517` test case so that it illustrates the way that
the reverted commit fails.

That commit introduced a regression that caused existing
tools/flow uses to fail. See https://cuelang.org/issue/2965.

A subsequent CL will apply a different fix for #1581.

For #2965.

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I322ac3b269bc79a541c15001a166160ad9191267
cueckoo pushed a commit that referenced this issue Mar 20, 2024
This reverts commit 9ef35eb
but keeps the `cmd_undefined` test case and adds
a new test case (very similar to the test case for #2517)
that illustrates the way that the reverted commit fails.

That commit introduced a regression that caused existing
tools/flow uses to fail. See https://cuelang.org/issue/2965.

A subsequent CL will apply a different fix for #1581.

For #2965.

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I322ac3b269bc79a541c15001a166160ad9191267
cueckoo pushed a commit that referenced this issue Mar 20, 2024
This reverts commit 9ef35eb
but keeps the `cmd_undefined` test case and adds
a new test case (very similar to the test case for #2517)
that illustrates the way that the reverted commit fails.

That commit introduced a regression that caused existing
tools/flow uses to fail. See https://cuelang.org/issue/2965.

A subsequent CL will apply a different fix for #1581.

For #2965.

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I322ac3b269bc79a541c15001a166160ad9191267
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1185281
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: CUEcueckoo <cueckoo@gmail.com>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
@nickfiggins
Copy link
Contributor

nickfiggins commented Mar 20, 2024

It seems like one way to handle this is to only return certain errors (that are considered "permanent") instead of all errors from tasks. I put up a CL here as a potential fix

@mvdan
Copy link
Member

mvdan commented Mar 21, 2024

@rogpeppe I'm surprised that your revert did not fix and close this issue, as the original reproducer should work now. Are you leaving it open with the new title to track better errors? Can you please clarify?

@rogpeppe
Copy link
Member

rogpeppe commented Mar 21, 2024

@mvdan I wasn't thinking clearly. Yes, I think this issue is now fixed - it's just the older issue (#1581) which should probably have been reopened.

@mvdan mvdan changed the title tools/flow: incomplete value not part of a task results in inappropriate error tools/flow: cue task not execute, when result in non-task value Mar 21, 2024
@mvdan
Copy link
Member

mvdan commented Mar 21, 2024

Thanks; restoring the old title for the sake of reference.

What older issue do you mean? #1571 is a discussion that's seemingly not related.

@rogpeppe
Copy link
Member

What older issue do you mean? #1571 is a discussion that's seemingly not related.

Oops, typo, I meant #1581. I've edited the comment now.

cueckoo pushed a commit that referenced this issue Apr 3, 2024
This reverts commit 9ef35eb
but keeps the `cmd_undefined` test case and adds
a new test case (very similar to the test case for #2517)
that illustrates the way that the reverted commit fails.

That commit introduced a regression that caused existing
tools/flow uses to fail. See https://cuelang.org/issue/2965.

A subsequent CL will apply a different fix for #1581.

For #2965.

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I322ac3b269bc79a541c15001a166160ad9191267
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1185281
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: CUEcueckoo <cueckoo@gmail.com>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
(cherry picked from commit ea385fd)
cueckoo pushed a commit that referenced this issue Apr 3, 2024
This reverts commit 9ef35eb
but keeps the `cmd_undefined` test case and adds
a new test case (very similar to the test case for #2517)
that illustrates the way that the reverted commit fails.

That commit introduced a regression that caused existing
tools/flow uses to fail. See https://cuelang.org/issue/2965.

A subsequent CL will apply a different fix for #1581.

For #2965.

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I322ac3b269bc79a541c15001a166160ad9191267
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1185281
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: CUEcueckoo <cueckoo@gmail.com>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
(cherry picked from commit ea385fd)
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1190978
Reviewed-by: Paul Jolly <paul@myitcv.io>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Triage Requires triage/attention
Projects
No open projects
Status: v0.8.1
Development

No branches or pull requests

4 participants