build: infer platform from first node if none set #2131
Conversation
crazy-max
force-pushed
the
fix-driver-infer-platform
branch
from
e215de5 to
55be0a6
Compare
crazy-max
changed the title
| @@ -226,7 +226,7 @@ func TestSelectNodeCurrentPlatform(t *testing.T) { | |||
| require.NoError(t, err) | |||
| require.True(t, perfect) | |||
| require.Len(t, res, 1) | |||
| require.Equal(t, "bbb", res[0].Node().Builder) | |||
| require.Equal(t, "aaa", res[0].Node().Builder) | |||
There was a problem hiding this comment.
The real issue is that the platform in opt gets reset when it should not. We need to figure out a test to make sure it doesn't happen. Looks like the old build code only expect "resolveNode" to have a platform if it is an override to the existing platform but maybe this should be made safer (more explicit) somehow.
There was a problem hiding this comment.
Rebased on top of @jedevc changes in master...jedevc:buildx:fix-setting-wrong-platform-on-solve-opt to fix the solve opt case and remove the duplicated map.
There was a problem hiding this comment.
I'm not convinced we have to change this, I think we could leave the default to be the current platform - this change isn't actually needed to fix the regression.
However, no real objections, my use case for this was to ease some UX issues with docker cloud build, which I don't really have any more.
There was a problem hiding this comment.
I guess you talk about the TestSelectNodeNoPlatform and default spec case right? It seems related to #2119 and proposed new rules in #2119 (comment) should cover this in follow-up iiuc.
This was more error prone, as opposed to the approach used prior to 616fb3e. Signed-off-by: Justin Chadwell <[email protected]>
This regression was introduced in 616fb3e, with the node resolution refactor. The core issue here is just that we would unconditionally set the solve opt's platform to the default current platform, which was incorrect. We can prevent this easily by having a special case for the default case, like we had before, and then not setting the platforms field on this (which keeping the resolution behavior which was introduced). Signed-off-by: Justin Chadwell <[email protected]>
Signed-off-by: CrazyMax <[email protected]>
crazy-max
force-pushed
the
fix-driver-infer-platform
branch
from
55be0a6 to
5403231
Compare
build/driver.go
Outdated
| @@ -19,6 +19,8 @@ import ( | |||
| ) | |||
|
|
|||
| type resolvedNode struct { | |||
| SolveOpt *client.SolveOpt | |||
There was a problem hiding this comment.
I think @jedevc did not like the driverPair name, but this is the use case for why it was added. That we pass along a struct that contains both the driver (in here it is called resolver+platform, so driver for a specific node) and the request that create fixed 1-to-1 pairing.
I think even if we use embed structs, the separation of structs would be clearer this way.
There was a problem hiding this comment.
After discussing with @tonistiigi, the idea is to have a clear separation of concerns between drivers resolution and solve opts attached to the driver in build. Pushed an extra commit to move *client.SolveOpt to a local struct type in build.
crazy-max
force-pushed
the
fix-driver-infer-platform
branch
from
96fe9d7 to
857b793
Compare
crazy-max
force-pushed
the
fix-driver-infer-platform
branch
from
857b793 to
cb36d64
Compare
*client.SolveOpt in driver code is only used by build code. For a clear separation of concerns, move it to an internal struct type only accessible by BuildWithResultHandler func. Signed-off-by: CrazyMax <[email protected]>
crazy-max
force-pushed
the
fix-driver-infer-platform
branch
from
cb36d64 to
9d8ac1c
Compare
closes #2124
If user doesn't specify any platform, it should be inferred from the first node instead of using the local platform. In follow-up we should also look at the rules suggested in #2119 (comment) to take into account the preferred platform as well.