dockerfile: set correct default path and shell based on OS #1747
Conversation
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
| @@ -227,7 +227,7 @@ func (gwCtr *gatewayContainer) Start(ctx context.Context, req client.StartReques | |||
| if procInfo.Meta.Cwd == "" { | |||
| procInfo.Meta.Cwd = "/" | |||
| } | |||
| procInfo.Meta.Env = addDefaultEnvvar(procInfo.Meta.Env, "PATH", utilsystem.DefaultPathEnv) | |||
| procInfo.Meta.Env = addDefaultEnvvar(procInfo.Meta.Env, "PATH", utilsystem.DefaultPathEnvUnix) // support windows? | |||
There was a problem hiding this comment.
@coryb Discovered that there is no way to check platform here. Do you think it may become a problem in the future and should we extend this somehow?
There was a problem hiding this comment.
I think we can plumb the platform information through NewContainer if we need to. It seems like something that would not be to hard to add on later?
There was a problem hiding this comment.
@coryb I think we should do it. It can be optional so doesn't complicate the client side. Another case where it may come up is if in the future there is a fully distributed mode and BuildKit needs to chose the correct node for the container (eg. windows/linux or arch based). so basically should add both of these fields https://github.com/moby/buildkit/blob/master/solver/pb/ops.proto#L21-L22
There was a problem hiding this comment.
Sure, I can file an issue and take that on after this PR gets merged. Sound good?
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
tonistiigi
force-pushed
the
default-shell-path
branch
from
5d077cd
to
cec6dae
Compare
| @@ -153,7 +153,13 @@ func (e *ExecOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, [] | |||
| } | |||
| if c.Caps != nil { | |||
| if err := c.Caps.Supports(pb.CapExecMetaSetsDefaultPath); err != nil { | |||
| env = env.SetDefault("PATH", system.DefaultPathEnv) | |||
| os := "linux" | |||
There was a problem hiding this comment.
Shouldn't this be runtime.GOOS?
There was a problem hiding this comment.
No, this is client side and does not represent the exec os. Usually this should be set and unix is better default than windows. Note also that this is only executed on missing cap that was added a long time ago so very theoretical to ever reach these lines.
fixes #1560