tools/flow: task deps lost when more than one level "deep" #2517

morlay · 2023-08-02T11:17:01Z

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

$ cue version
v0.6.0-rc.1

Does this issue reproduce with the latest stable release?

Yes

What did you do?

Use task output as others' int

package test

import (
	"list"
	"tool/cli"
	"tool/exec"
)

version: #Version & {}

command: build: #Build & {
	command: flags: {
		"-c": "echo \(version.output)"
	}
	//  failed in this way too
	//  
	//	command: args: [
	//		"-c", "echo \(version.output)",
	//	]
}

command: print: cli.Print & {
	text: command.build.output
}

#Build: {
	version: string

	command: {
		name: string | *"sh"
		args: [...string]
		flags: [String=string]: string
	}

	_run: exec.Run & {
		cmd: list.FlattenN([
			command.name,
			for f, v in command.flags {
				[f, v]
			},
			command.args,
		], 1)
		stdout: string
	}

	output: _run.stdout
}

#Version: {
	_run: exec.Run & {
		cmd:    "echo v1.0.0"
		stdout: string
	}
	output: _run.stdout
}

What did you expect to see?

version -> command.build -> command.print

What did you see instead?

version not be as deps of command.build and not execute

The text was updated successfully, but these errors were encountered:

mvdan · 2023-08-02T11:21:28Z

Can you confirm whether this bug exists in v0.5.0?

morlay · 2023-08-02T11:27:23Z

Can you confirm whether this bug exists in v0.5.0?

Yes. but only test with cue cmd,

With api in my tool https://github.com/octohelm/wagon, it works well.

mvdan · 2023-08-02T11:30:48Z

If there is a regression in CUE's Go API, can you please share a reproducer with it that succeeds with v0.5.0 but fails with v0.6.0-rc.1?

morlay · 2023-08-02T11:31:10Z

@mvdan Another usage case like:

Work well with v0.6.0-rc.1, but failed with v0.5.0

package test

import (
	"tool/cli"
	"tool/exec"
)

version: #Version & {}

command: build: #Build & {
	script: "echo \(version.output)"
}

command: print: cli.Print & {
	text: command.build.output
}

#Build: {
	script: string

	_run: exec.Run & {
		cmd:    script
		stdout: string
	}

	output: _run.stdout
}

#Version: {
	_run: exec.Run & {
		cmd:    "echo v1.0.0"
		stdout: string
	}
	output: _run.stdout
}

mvdan · 2023-08-02T11:31:56Z

If a reproducer failed with v0.5.0 but works with v0.6.0-rc.1, that's a good thing, no? We mainly want to be aware of any regressions since v0.5.0, as those are something to look at before we tag the final v0.6.0.

morlay · 2023-08-02T11:40:24Z

If there is a regression in CUE's Go API, can you please share a reproducer with it that succeeds with v0.5.0 but fails with v0.6.0-rc.1?

This project could be show the case, https://github.com/octohelm/wagon

on branch main:

WAGON_GRAPH=1 make wagon.debug

You could see the pkg.version as deps of actions.go.build."linux/arm64"._dag."3"._run.build."1"._run

https://kroki.io/d2/svg/eNq8k8GKwjAQhu99ipL7ztLdZQ8efJJCiE2Ig2kmTJuiby-1FhWD1Gq9hMmQ-f6ZDL9GNlWL5Fc5o922mVCnawOWoKHIlRH51zq_TktLNWmQbJQeYnFTtonoNJTCoY_7b8X1_18pQGploRRFH1YUDnfYh2U_fcjRQ2AzPizOuZS6xFpZAzJE54ZzKT1pyfjurfSws9AZbpD8c9zfkTvli-b0tlw3ry5soticbX10jos_plhxDiph3zQ2MUp2DAAA__9n2WUA?theme=101

the replace with the patch just disable this from v0.5.0 morlay@b75bada

Once I upgrade to v0.6.0-rc.1 (on branch cuelang-0.6)

require (
   cuelang.org/go v0.6.0-rc.1                   // both remove the replace 
   github.com/octohelm/cuemod v0.7.0  // just some api bump
)

the pkg.version task is missing

https://kroki.io/d2/svg/eNq8kkELwiAUx-99iuG9F6vo0KEv0kJsigluT54T6tvH5kaNeViDdpGnvP_vp_KkIVU2ButzRkY_mg0T3daDRuAaK5TASQkZa5ZtL9l3yz0YK4GbSmgF3AVr48pGII-BSjUJJ_hzYtFZMGvq8NwJqk7HgoFvlPPX_Aa8RPcag9IJLoWGguVt2WXmifrYvi0p1OBIDY15f5ayJ_7or74lmJ8udBhIa7x-VdlnIpaP4xT1DgAA__-tCSWl?theme=101

Other deps missing cause by #2516

morlay · 2023-08-02T13:34:31Z

Simplier testcase issue2517.txtar

-- in.cue --
package p

import (
	"tool/cli"
	"tool/exec"
)

root: version: exec.Run & {
	cmd:    "echo v1.0.0"
	stdout: string
}

root: build: {
	args: [
	    "echo \(root.version.stdout)"
	]

	run: exec.Run & {
		cmd:    args
		stdout: string
	}
}

root: print: cli.Print & {
	text: root.build.run.stdout
}
-- out/run/errors --
-- out/run/t0 --
graph TD
  t0("root.version [Ready]")
  t1("root.build.run [Waiting]")
  t1-->t0
  t2("root.print [Waiting]")
  t2-->t1

-- out/run/t1 --
graph TD
  t0("root.version [Terminated]")
  t1("root.build.run [Ready]")
  t1-->t0
  t2("root.print [Waiting]")
  t2-->t1

-- out/run/t1/value --
{
	$id: "tool/exec.Run"
	cmd: "echo v1.0.0"
	env: {}
	stdout:  "foo"
	stderr:  null
	stdin:   null
	success: bool
}
-- out/run/t1/stats --
Leaks:  0
Freed:  58
Reused: 50
Allocs: 8
Retain: 0

Unifications: 27
Conjuncts:    78
Disjuncts:    58
-- out/run/t2 --
graph TD
  t0("root.version [Terminated]")
  t1("root.build.run [Terminated]")
  t1-->t0
  t2("root.print [Ready]")
  t2-->t1

-- out/run/t2/value --
{
	$id: "tool/exec.Run"
	cmd: "echo foo"
	env: {}
	stdout:  "foo"
	stderr:  null
	stdin:   null
	success: bool
}
-- out/run/t2/stats --
Leaks:  0
Freed:  58
Reused: 58
Allocs: 0
Retain: 0

Unifications: 27
Conjuncts:    84
Disjuncts:    58
-- out/run/t3 --
graph TD
  t0("root.version [Terminated]")
  t1("root.build.run [Terminated]")
  t1-->t0
  t2("root.print [Terminated]")
  t2-->t1

-- out/run/t3/value --
{
	$id:    "tool/cli.Print"
	stdout: "foo"
	text:   "foo"
}
-- out/run/t3/stats --
Leaks:  0
Freed:  59
Reused: 59
Allocs: 0
Retain: 0

Unifications: 28
Conjuncts:    88
Disjuncts:    59
-- out/run/stats/totals --
Leaks:  0
Freed:  175
Reused: 167
Allocs: 8
Retain: 0

Unifications: 82
Conjuncts:    250
Disjuncts:    175

@mvdan it could pass when &dep.Config{ Dynamic: true, Descend: true } but issue2490.txtar failed.

myitcv · 2023-08-07T14:39:19Z

@morlay thanks for the report. It was a little hard to pin down exactly what you did to trigger the problem but I think I have now down so in the following.

What did you do?

go mod tidy
go run .
cmp stdout stdout.golden

-- go.mod --
module mod.com

go 1.20

require cuelang.org/go v0.6.0-rc.1
-- main.go --
package main

import (
	"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(&flow.Config{
		FindHiddenTasks: true,
	}, v, ioTaskFunc)
	for _, t := range controller.Tasks() {
		fmt.Printf("task at path: %v\n", t.Path())
		for _, d := range t.Dependencies() {
			fmt.Printf(" --> %v\n", d.Path())
		}
	}
}

func ioTaskFunc(v cue.Value) (flow.Runner, error) {
	inputPath := cue.ParsePath("$id")
	input := v.LookupPath(inputPath)
	if !input.Exists() {
		return nil, nil
	}

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

command: build: {
	_prepare: {
		$id: "test"
	}

	_env: {
		INPUT: _prepare.stdout
	}

	_run: {
		$id: "test"
		env: _env
	}
}
-- stdout.golden --
task at path: command.build._prepare
task at path: command.build._run
 --> command.build._prepare

What did you expect to see?

A passing test. This passes with v0.5.0

What did you see instead?

$ ts repro.txtar
> go mod tidy
> go run .
[stdout]
task at path: command.build._prepare
task at path: command.build._run

> cmp stdout stdout.golden
--- stdout
+++ stdout.golden
@@ -1,2 +1,3 @@
 task at path: command.build._prepare
 task at path: command.build._run
+ --> command.build._prepare

FAIL: /tmp/testscript2121119944/repro.txtar/script.txtar:3: stdout and stdout.golden differ

i.e. the dependency from _run --> _prepare (through _env) does not exist with v0.6.0-rc.1.

This bisects to https://cuelang.org/cl/556460

Also noting that the docs for dep.Config.Descend says:

  // Descend enables recursively descending into fields. This option is
  // implied by Dynamic.
  Descend bool

But a cursory glance at the code does not show the value of Dynamic being referenced with respect to Descend.

mvdan · 2023-08-09T09:06:45Z

@morlay I've just merged a fix for this issue, and we're fairly sure that your other tools/flow issue had the same underlying cause so it should be fixed as well. Could you please try the latest master version in your Go code and confirm whether or not the fixes work for you?

morlay · 2023-08-09T11:09:51Z

@mvdan flow.New hangs with "v0.6.0" 😭

https://github.com/octohelm/wagon/blob/main/pkg/engine/plan/runner.go#L220-L226

f := flow.New(
    &flow.Config{
	    FindHiddenTasks: true,
    },
    cueValue,
    noOpRunner,
)

mvdan · 2023-08-09T11:14:56Z

Ah, that's unfortunate. Could you please file a new issue following the bug template? Ideally with a self-contained reproducer, and a git bisect so we can tell what was the last CUE version to work correctly.

morlay · 2023-08-09T11:23:21Z

@myitcv I will try

log in markTaskDependencies of flow.Controller

Seems cause the repeated dep.Dependency

actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/arm64"._pull.output
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/arm64"._pull.config
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/amd64"._pull.output
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/amd64"._pull.config
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/amd64"._pull
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/amd64"._pull.output
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/amd64"._pull.config
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/amd64"._pull
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/arm64"._pull.output
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/arm64"._pull.config
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/arm64"._pull
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/arm64"._pull.output
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/arm64"._pull.config
actions.go.ship."push/linux/amd64"._push <= actions.go.ship._push.x._pull."linux/arm64"._pull
actions.go.ship."push/linux/amd64"._push <= pkg.version.output
actions.go.ship."push/linux/amd64"._push <= pkg.version.output
actions.go.ship."push/linux/amd64"._push <= pkg.version.output
actions.go.ship."push/linux/amd64"._push <= pkg.version.output
actions.go.ship."push/linux/amd64"._push <= pkg.version.output
actions.go.ship."push/linux/amd64"._push <= pkg.version.output
actions.go.ship."push/linux/amd64"._push <= pkg.version.output

morlay · 2023-08-09T11:29:16Z

when I added some map to avoid the loop hangs.

visited := map[dep.Dependency]bool{}

dep.Visit(cfg, c.opCtx, n, func(d dep.Dependency) error {
    if visited[d] {
	    return nil
    }
    visited[d] = true
    
    //  ...
})

will got error

Error: cyclic task dependency:
        task actions.go.ship."push/linux/amd64"._push refers to
        task actions.go.build."linux/amd64"._dag."3"._run.build."1"._run refers to
        task actions.go.build."linux/arm64"._dag."3"._run.build."1"._run refers to
        task actions.go.ship."push/linux/amd64"._push:

mvdan · 2023-08-09T11:31:26Z

Please don't try to fix it, these bugs tend to be hard to investigate and fix. The priority is a way for us to reproduce the bug - see my previous comment.

morlay · 2023-08-10T06:33:14Z

@mvdan not try to fix it. just debug to locate where cause the issue.

My cuepkg contains code like, which may not correctly since v0.5.0 (https://github.com/cue-lang/cue/releases/tag/v0.5.0)

ship?: #Ship

if ship != _|_ {
   ship: "auths":  auths 
   ship: "mirror": mirror
}

Once I upgrade this, everything works well.

myitcv · 2023-08-10T06:41:50Z

@mvdan not try to fix it. just debug to locate where cause the issue.

My cuepkg contains code like, which may not correctly since v0.5.0 (https://github.com/cue-lang/cue/releases/tag/v0.5.0)
ship?: #Ship

if ship != _|_ {
   ship: "auths":  auths 
   ship: "mirror": mirror
}
Once I upgrade this, everything works well.

@morlay what is the if comprehension looking to check here?

morlay · 2023-08-10T07:38:19Z

@morlay what is the if comprehension looking to check here?

ship contains optional tasks.

if some ship fields defined,

actions: go: #Project & {
   ship: {
      dest: "image_name"
   }
}

The ship tasks will be enabled (then merge auths and mirror into ship parameters)
Otherwise, ship tasks will be disabled.

Here an example in my release project: "webapp" and "go" both base the #Project taskset.

"go" tasks need ship, but webapp don't

$ wagon do  

Allowed action:
go
go archive  [--output=<dir>]
go build
go build "darwin/amd64"  [--output=<path_to_oci_tar>]
go build "darwin/arm64"  [--output=<path_to_oci_tar>]
go build "linux/amd64"   [--output=<path_to_oci_tar>]
go build "linux/arm64"   [--output=<path_to_oci_tar>]
go ship
go ship "push/linux/amd64"  Push <arch> suffix image
go ship "push/linux/arm64"  Push <arch> suffix image
go ship "push/x"            Merge pushed arch suffix images into mutli-arch image
go ship push amd64          Push <arch> suffix image
go ship push arm64          Push <arch> suffix image
go ship push x              Merge pushed arch suffix images into mutli-arch image
go ship pushx               Push all images as multi-arch images
go source                   [--output=<dir>]
webapp build                [--output=<dir>]
webapp build image          [--output=<path_to_oci_tar>]
webapp source               [--output=<dir>]

If I want keep this feature, I could to write like this instead.

ship?: #Ship & { 
   "auths":  auths
   "mirror": mirror
}

myitcv · 2023-08-10T07:42:08Z

If I want keep this feature, I could to write like this instead.
ship?: #Ship & { 
   "auths":  auths
   "mirror": mirror
}

Yes, exactly what I was going to suggest. #943 will thankfully make this use of _|_ unnecessary, so much clearer to the reader what the intent was.

morlay · 2023-08-10T07:51:35Z

Yes, exactly what I was going to suggest. #943 will thankfully make this use of _|_ unnecessary, so much clearer to the reader what the intent was.

Yes. it is much clear.
So I‘m doing refactor my codes of old usages now.

morlay · 2023-08-10T08:46:07Z

@myitcv deps broking with the new way 😄 #2535

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 <[email protected]> Change-Id: I322ac3b269bc79a541c15001a166160ad9191267

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 <[email protected]> Change-Id: I322ac3b269bc79a541c15001a166160ad9191267 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1185281 Reviewed-by: Daniel Martí <[email protected]> TryBot-Result: CUEcueckoo <[email protected]> Unity-Result: CUE porcuepine <[email protected]>

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 <[email protected]> Change-Id: I322ac3b269bc79a541c15001a166160ad9191267 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1185281 Reviewed-by: Daniel Martí <[email protected]> TryBot-Result: CUEcueckoo <[email protected]> Unity-Result: CUE porcuepine <[email protected]> (cherry picked from commit ea385fd)

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 <[email protected]> Change-Id: I322ac3b269bc79a541c15001a166160ad9191267 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1185281 Reviewed-by: Daniel Martí <[email protected]> TryBot-Result: CUEcueckoo <[email protected]> Unity-Result: CUE porcuepine <[email protected]> (cherry picked from commit ea385fd) Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1190978 Reviewed-by: Paul Jolly <[email protected]> TryBot-Result: CUEcueckoo <[email protected]>

morlay added NeedsInvestigation Triage Requires triage/attention labels Aug 2, 2023

morlay mentioned this issue Aug 2, 2023

"cycle error" regression with let fields between 0.4.3 and 0.5.0 #2355

Closed

myitcv mentioned this issue Aug 7, 2023

tool/flow tasks deps lost when struct value pass in task field #2516

Closed

myitcv changed the title ~~v0.6.0-rc.1 [tool/flow] task deps lost when used slice or map task parameters~~ tools/flow: task deps lost when more than one level "deep" Aug 7, 2023

mvdan added this to the v0.6.0 required fields milestone Aug 7, 2023

mvdan removed the Triage Requires triage/attention label Aug 7, 2023

cueckoo closed this as completed in 97d7109 Aug 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools/flow: task deps lost when more than one level "deep" #2517

tools/flow: task deps lost when more than one level "deep" #2517

morlay commented Aug 2, 2023 •

edited

Loading

mvdan commented Aug 2, 2023

morlay commented Aug 2, 2023

mvdan commented Aug 2, 2023

morlay commented Aug 2, 2023

mvdan commented Aug 2, 2023

morlay commented Aug 2, 2023 •

edited

Loading

morlay commented Aug 2, 2023 •

edited

Loading

myitcv commented Aug 7, 2023

mvdan commented Aug 9, 2023

morlay commented Aug 9, 2023

mvdan commented Aug 9, 2023

morlay commented Aug 9, 2023 •

edited

Loading

morlay commented Aug 9, 2023

mvdan commented Aug 9, 2023

morlay commented Aug 10, 2023 •

edited

Loading

myitcv commented Aug 10, 2023

morlay commented Aug 10, 2023

myitcv commented Aug 10, 2023

morlay commented Aug 10, 2023

morlay commented Aug 10, 2023

tools/flow: task deps lost when more than one level "deep" #2517

tools/flow: task deps lost when more than one level "deep" #2517

Comments

morlay commented Aug 2, 2023 • edited Loading

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

Does this issue reproduce with the latest stable release?

What did you do?

What did you expect to see?

What did you see instead?

mvdan commented Aug 2, 2023

morlay commented Aug 2, 2023

mvdan commented Aug 2, 2023

morlay commented Aug 2, 2023

mvdan commented Aug 2, 2023

morlay commented Aug 2, 2023 • edited Loading

morlay commented Aug 2, 2023 • edited Loading

myitcv commented Aug 7, 2023

What did you do?

What did you expect to see?

What did you see instead?

mvdan commented Aug 9, 2023

morlay commented Aug 9, 2023

mvdan commented Aug 9, 2023

morlay commented Aug 9, 2023 • edited Loading

morlay commented Aug 9, 2023

mvdan commented Aug 9, 2023

morlay commented Aug 10, 2023 • edited Loading

myitcv commented Aug 10, 2023

morlay commented Aug 10, 2023

myitcv commented Aug 10, 2023

morlay commented Aug 10, 2023

morlay commented Aug 10, 2023

morlay commented Aug 2, 2023 •

edited

Loading

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

morlay commented Aug 2, 2023 •

edited

Loading

morlay commented Aug 2, 2023 •

edited

Loading

morlay commented Aug 9, 2023 •

edited

Loading

morlay commented Aug 10, 2023 •

edited

Loading