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

LCOW: WORKDIR correct handling #34405

Merged
merged 1 commit into from Aug 18, 2017

Conversation

Projects
None yet
5 participants
@jhowardmsft
Contributor

jhowardmsft commented Aug 4, 2017

Signed-off-by: John Howard jhoward@microsoft.com

WORKDIR in LCOW mode was being treated as a Windows path and not working - /bin was being translated to C:\bin. This fixes it to use Unix semantics.

PS E:\docker\build\lcow> docker build -f workdir --no-cache .
Sending build context to Docker daemon  6.144kB
Step 1/2 : FROM busybox
 ---> efe10ee6727f
Step 2/2 : WORKDIR /bin
 ---> 29731c1d2cbd
Removing intermediate container 7425c28fb858
Successfully built 29731c1d2cbd
PS E:\docker\build\lcow> docker inspect -f "{{.ContainerConfig.Cmd}} -- {{.Config.WorkingDir}}" 297
[/bin/sh -c #(nop) WORKDIR /bin] -- /bin
PS E:\docker\build\lcow> docker run --rm 297 pwd
/bin
PS E:\docker\build\lcow> docker run --rm 297 ls -l ./sh
-rwxr-xr-x  385 root     root       1049592 Aug  4 18:44 ./sh
PS E:\docker\build\lcow>

@johnstep @dnephin PTAL

@gupta-ak - I left a TODO in there for the remote filesystem operation.

{"linux", ``, `foo`, `/foo`, ``},
{"linux", ``, `/foo`, `/foo`, ``},
{"linux", `/foo`, `bar`, `/foo/bar`, ``},
{"linux", `/foo`, `/bar`, `/bar`, ``},

This comment has been minimized.

@johnstep

johnstep Aug 4, 2017

Contributor

Since normaliseWorkdirUnix replaces Windows path separators with Unix, how about tests for that?

@johnstep

johnstep Aug 4, 2017

Contributor

Since normaliseWorkdirUnix replaces Windows path separators with Unix, how about tests for that?

This comment has been minimized.

@jhowardmsft

jhowardmsft Aug 4, 2017

Contributor

Added case {"linux", \a, b\c, /a/b/c, ``},

@jhowardmsft

jhowardmsft Aug 4, 2017

Contributor

Added case {"linux", \a, b\c, /a/b/c, ``},

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 4, 2017

Contributor

Updated for docker run --workdir to work as well.

PS E:\docker\build\lcow> docker run --rm --workdir /bin busybox ls -l sh
-rwxr-xr-x  385 root     root       1049592 Aug  4 19:56 sh
Contributor

jhowardmsft commented Aug 4, 2017

Updated for docker run --workdir to work as well.

PS E:\docker\build\lcow> docker run --rm --workdir /bin busybox ls -l sh
-rwxr-xr-x  385 root     root       1049592 Aug  4 19:56 sh
config.WorkingDir = filepath.FromSlash(config.WorkingDir) // Ensure in platform semantics
if !system.IsAbs(config.WorkingDir) {
wdInvalid := false
if runtime.GOOS == platform {

This comment has been minimized.

@johnstep

johnstep Aug 4, 2017

Contributor

Is this a sufficient LCOW check? I think so, at least for now, but maybe system.LCOWSupported is preferred.

@johnstep

johnstep Aug 4, 2017

Contributor

Is this a sufficient LCOW check? I think so, at least for now, but maybe system.LCOWSupported is preferred.

This comment has been minimized.

@jhowardmsft

jhowardmsft Aug 4, 2017

Contributor

I could be more explicit, but in reality, here it's redundant. It can only be LCOW.

@jhowardmsft

jhowardmsft Aug 4, 2017

Contributor

I could be more explicit, but in reality, here it's redundant. It can only be LCOW.

@johnstep

LGTM

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Aug 9, 2017

Member

As discussed in #33854 there is no such thing as "build platform". Only platform for current stage (and maybe default platform for image access if maintainers can agree on it). So these should be used instead for detecting if conversion is needed. Ideally I would even like if this conversion only happens on starting a container instead of the builder but we can discuss that in another issue. OCI image spec doesn't seem to define anything about C: prefix.

Member

tonistiigi commented Aug 9, 2017

As discussed in #33854 there is no such thing as "build platform". Only platform for current stage (and maybe default platform for image access if maintainers can agree on it). So these should be used instead for detecting if conversion is needed. Ideally I would even like if this conversion only happens on starting a container instead of the builder but we can discuss that in another issue. OCI image spec doesn't seem to define anything about C: prefix.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 9, 2017

Contributor

As discussed in #33854 there is no such thing as "build platform".

To be more accurate, your goal is that there shouldn't be such a thing as a "build platform". I don't necessarily disagree (entirely), but the fact is it's there presently, and the resolution of how the code base gets updated, if indeed to use FROM --platform is orthogonal to making progress on LCOW in the limited time available to hit RTM. This PR doesn't do anything contentious that isn't already in the codebase.

So these should be used instead for detecting if conversion is needed.

Yes, but after #33854 has been ratified and agreed.

Ideally I would even like if this conversion only happens on starting a container instead of the builder but we can discuss that in another issue.

Sure, that's for later and also orthogonal. But I have concerns about cache breaks if you are going to consider that. The builder has done normalization in this way since WS2016 TP4 days - a very long time. This PR doesn't change that, it just makes sure that the Windows daemon doesn't universally assume it's a full Windows path.

CI image spec doesn't seem to define anything about C: prefix.

Again, if it doesn't, that is also orthogonal to this PR and something which needs addressing in the OCI docs rather than here.

Contributor

jhowardmsft commented Aug 9, 2017

As discussed in #33854 there is no such thing as "build platform".

To be more accurate, your goal is that there shouldn't be such a thing as a "build platform". I don't necessarily disagree (entirely), but the fact is it's there presently, and the resolution of how the code base gets updated, if indeed to use FROM --platform is orthogonal to making progress on LCOW in the limited time available to hit RTM. This PR doesn't do anything contentious that isn't already in the codebase.

So these should be used instead for detecting if conversion is needed.

Yes, but after #33854 has been ratified and agreed.

Ideally I would even like if this conversion only happens on starting a container instead of the builder but we can discuss that in another issue.

Sure, that's for later and also orthogonal. But I have concerns about cache breaks if you are going to consider that. The builder has done normalization in this way since WS2016 TP4 days - a very long time. This PR doesn't change that, it just makes sure that the Windows daemon doesn't universally assume it's a full Windows path.

CI image spec doesn't seem to define anything about C: prefix.

Again, if it doesn't, that is also orthogonal to this PR and something which needs addressing in the OCI docs rather than here.

@@ -16,7 +17,7 @@ func TestNormaliseWorkdir(t *testing.T) {
}
for _, test := range testCases {
normalised, err := normaliseWorkdir(test.current, test.requested)
normalised, err := normaliseWorkdir(runtime.GOOS, test.current, test.requested)

This comment has been minimized.

@stevvooe

stevvooe Aug 17, 2017

Contributor

nit: normalize (do we standardize on us eng?)

@stevvooe

stevvooe Aug 17, 2017

Contributor

nit: normalize (do we standardize on us eng?)

@@ -5,25 +5,31 @@ package dockerfile
import "testing"
func TestNormaliseWorkdir(t *testing.T) {

This comment has been minimized.

@stevvooe

stevvooe Aug 17, 2017

Contributor

"Normalize"

@stevvooe

stevvooe Aug 17, 2017

Contributor

"Normalize"

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Aug 17, 2017

Member

As discussed offline, both build and create paths will need to be updated based on the #33854 outcome and the current definitely isn't correct. LGTM to unblock

Member

tonistiigi commented Aug 17, 2017

As discussed offline, both build and create paths will need to be updated based on the #33854 outcome and the current definitely isn't correct. LGTM to unblock

LCOW: WORKDIR correct handling
Signed-off-by: John Howard <jhoward@microsoft.com>

@johnstep johnstep merged commit 30eb4d8 into moby:master Aug 18, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 36349 has succeeded
Details
janky Jenkins build Docker-PRs 44973 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 5361 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 16405 has succeeded
Details
z Jenkins build Docker-PRs-s390x 5136 has succeeded
Details
@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 22, 2017

Contributor

@johnstep Are we going to have a PR correcting the spelling errors before merging?

Contributor

stevvooe commented Aug 22, 2017

@johnstep Are we going to have a PR correcting the spelling errors before merging?

@johnstep

This comment has been minimized.

Show comment
Hide comment
@johnstep

johnstep Aug 22, 2017

Contributor

@stevvooe I also prefer we standardize on language and locale. Is there official guidance for this somewhere? Can it be overridden on a file-by-file basis? @jhowardmsft, what is your take?

Contributor

johnstep commented Aug 22, 2017

@stevvooe I also prefer we standardize on language and locale. Is there official guidance for this somewhere? Can it be overridden on a file-by-file basis? @jhowardmsft, what is your take?

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 22, 2017

Contributor

I took care of it in #34602.

Contributor

stevvooe commented Aug 22, 2017

I took care of it in #34602.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment