LCOW: WORKDIR correct handling by lowenna · Pull Request #34405 · moby/moby

lowenna · 2017-08-04T18:50:57Z

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.

johnstep · 2017-08-04T19:57:06Z

builder/dockerfile/dispatchers_windows_test.go

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

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

lowenna · 2017-08-04T19:58:53Z

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

johnstep · 2017-08-04T20:09:44Z

daemon/container.go

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

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

johnstep

LGTM

tonistiigi · 2017-08-09T00:31:04Z

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.

lowenna · 2017-08-09T01:10:38Z

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.

stevvooe · 2017-08-17T20:58:23Z

builder/dockerfile/dispatchers_unix_test.go

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

stevvooe · 2017-08-17T20:58:46Z

builder/dockerfile/dispatchers_windows_test.go

"Normalize"

tonistiigi · 2017-08-17T22:14:16Z

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

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

stevvooe · 2017-08-22T00:26:27Z

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

johnstep · 2017-08-22T21:43:28Z

@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 · 2017-08-22T22:29:29Z

I took care of it in #34602.

lowenna requested review from dnephin and tonistiigi as code owners August 4, 2017 18:50

GordonTheTurtle added the status/0-triage label Aug 4, 2017

lowenna force-pushed the jjh/lcowworkdir branch 2 times, most recently from bca1b6f to e2c0405 Compare August 4, 2017 18:58

johnstep reviewed Aug 4, 2017

View reviewed changes

lowenna force-pushed the jjh/lcowworkdir branch from e2c0405 to d8b6a7f Compare August 4, 2017 19:58

lowenna force-pushed the jjh/lcowworkdir branch from d8b6a7f to 2c69572 Compare August 4, 2017 20:01

johnstep reviewed Aug 4, 2017

View reviewed changes

lowenna added status/2-code-review and removed status/0-triage labels Aug 5, 2017

johnstep approved these changes Aug 8, 2017

View reviewed changes

PatrickLang mentioned this pull request Aug 17, 2017

Epic: Linux Containers on Windows (LCOW) #33850

Closed

stevvooe reviewed Aug 17, 2017

View reviewed changes

builder/dockerfile/dispatchers_unix_test.go Outdated

Copy link

Contributor

stevvooe Aug 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

stevvooe reviewed Aug 17, 2017

View reviewed changes

builder/dockerfile/dispatchers_windows_test.go Outdated

Copy link

Contributor

stevvooe Aug 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Normalize"

LCOW: WORKDIR correct handling

9fa4490

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

lowenna force-pushed the jjh/lcowworkdir branch from 2c69572 to 9fa4490 Compare August 17, 2017 22:29

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

thaJeztah added the area/lcow Issues and PR's related to the experimental LCOW feature label Oct 30, 2018

Conversation

lowenna commented Aug 4, 2017

Uh oh!

johnstep Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

lowenna Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

lowenna commented Aug 4, 2017

Uh oh!

johnstep Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lowenna Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lowenna commented Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevvooe Aug 17, 2017

Choose a reason for hiding this comment

Uh oh!

stevvooe Aug 17, 2017

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Aug 17, 2017

Uh oh!

stevvooe commented Aug 22, 2017

Uh oh!

johnstep commented Aug 22, 2017

Uh oh!

stevvooe commented Aug 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

johnstep Aug 4, 2017 •

edited

Loading

tonistiigi commented Aug 9, 2017 •

edited

Loading

lowenna commented Aug 9, 2017 •

edited

Loading