Build scripts can distinguish between solve nodes for different targets. #3531
Conversation
Clients need to pass the target when querying requirements, platforms, etc. Use "" for the default target (i.e. the name assigned to 'main').
…mat. Previously, build scripts with multiple targets only had their default target's requirements transformed.
mitchell-as
force-pushed
the
mitchell/dx-2960
branch
from
d76629d
to
aed5512
Compare
|
|
||
| if f := a.Value.FuncCall; target == "" && f != nil && isSolveFuncName(f.Name) { | ||
| // This is coming from a complex build expression with no straightforward way to determine | ||
| // a default target. Fall back on a top-level solve node. |
There was a problem hiding this comment.
pkg/buildscript/queries.go
Outdated
| // Otherwise, the "src" key contains a reference to the solve node. Look over the build expression | ||
| // again for that referenced node. | ||
| for _, arg := range node.FuncCall.Arguments { | ||
| if arg.Assignment == nil { | ||
| continue | ||
| } | ||
| a := arg.Assignment | ||
| if a.Key == srcKey && a.Value.Ident != nil { | ||
| node, err := b.getSolveNode(*a.Value.Ident) | ||
| if err != nil { | ||
| return nil, errs.Wrap(err, "Could not get solve node from target") | ||
| } | ||
| return node, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Why can't this solve node be identified by the getTargetNode above? Is it because the target value wasn't found? Can you please add a more detailed comment.
There was a problem hiding this comment.
Consider this build script snippet:
runtime = state_tool_artifacts_v1(src = sources)
sources = solve(...)
main = runtime
getTargetNode() would return sources (main -> runtime -> src -> sources). Since sources is not a solve node (it's a reference), we have to look again for the sources node. The second time, getTargetNode("sources") will give us the solve node we need.
I've added a brief comment in the code.
| seen := make(map[string]bool) | ||
| for _, assignment := range raw.Assignments { | ||
| if _, exists := seen[assignment.Key]; exists { | ||
| return nil, locale.NewInputError(locale.Tl("err_buildscript_duplicate_keys", "Build script has duplicate '{{.V0}}' assignments", assignment.Key)) | ||
| } | ||
| seen[assignment.Key] = true | ||
| } |
There was a problem hiding this comment.
Does this still allow multiple solve nodes for different targets? The ACs in the story don't stipulate that it has to be multiple solve nodes for a single target that raise an error. Maybe the story needs to be updated?
There was a problem hiding this comment.
Yes, you can have more than one solve node in a buildscript. There's only an issue if you have a buildscript whose target has two different solve nodes. For example:
runtime = state_tool_artifacts_v1(src = sources)
sources = solve(...)
sources = solve(... something different...)
another_solve = solve(...)
This error would identify sources has having duplicate assignments. another_solve is totally fine, as it's a different target or whatever.
pkg/buildscript/queries.go
Outdated
| return name == solveFuncName || name == solveLegacyFuncName | ||
| } | ||
|
|
||
| func (b *BuildScript) getTargetNode(target string) (*Value, error) { |
There was a problem hiding this comment.
nit: Any taste for a variadic argument here? Would allow us to forgo the passing of "" and be ready for multiple targets when the time comes.
There was a problem hiding this comment.
Yes! I was looking for an excuse to make this an optional argument, and multiple target support justifies it!
mitchell-as
force-pushed
the
mitchell/dx-2960
branch
from
85a0823
to
3306dce
Compare
mitchell-as
force-pushed
the
mitchell/dx-2960
branch
from
3306dce
to
bdf9fd5
Compare
pkg/buildscript/queries.go
Outdated
| return nil, errNodeNotFound | ||
| } | ||
|
|
||
| func (b *BuildScript) getSolveNode(target ...string) (*Value, error) { |
There was a problem hiding this comment.
nit: This should probably be targets
Revert "Merge branch version/0-47-0-RC1 to adopt changes from PR #3531"
Clients need to pass the target when querying requirements, platforms, etc. Use "" for the default target (i.e. the name assigned to 'main').