Fix cmd and entrypoint handling in parser #7806

erikh · 2014-08-30T11:38:46Z

This utilizes a test provided by @cnf which exposed an issue with the parser injecting "" for empty json values, which was then interpreted as a value itself by the evaluator. Removing this exposed a ton of fiddly order-dependent bits around entrypoint and cmd, which then needed to be fixed as well.

Hopefully we can address this with versioning and clarifying semantics.

erikh · 2014-08-30T11:58:49Z

There should be a button at the bottom of each PR that says "watch" or something similar. Click that.

thaJeztah · 2014-08-30T12:04:00Z

Oh, sorry for being unclear. I'm watching the whole Docker repo already, so that's not a problem, but I want to relate 'versioning' issues to my proposal, but wasn't really sure 'which way around' I should link; i.e. Link this issue from my issue, or the other way around :)

erikh · 2014-08-30T12:06:44Z

There are going to be at least several dockerfile-related tickets before 1.3 is released; I would prefer they not all be cluttered up this way. The maintainers get a lot of email already.

I appreciate the enthusiasm though and I hope to address versioning soon. :)

On Aug 30, 2014, at 5:04 AM, Sebastiaan van Stijn [email protected] wrote:

Oh, sorry for being unclear. I'm watching the whole Docker repo already, so that's not a problem, but I want to relate 'versioning' issues to my proposal, but wasn't really sure 'which way around' I should link; i.e. Link this issue from my issue, or the other way around :)

—
Reply to this email directly or view it on GitHub.

thaJeztah · 2014-08-30T12:21:09Z

Ok, I'll remove my comment. Thought it could be useful to collect/group version related issues at a central location, but don't want to spam the maintainers obviously. Meanwhile, will keep track on progress and other suggestions are of course welcome!

LK4D4 · 2014-08-30T14:01:51Z

I still don't see cli test on empty entrypoint :)

vieux · 2014-09-01T18:01:20Z

@LK4D4 👍

LK4D4 · 2014-09-01T18:34:42Z

Also, next tests fails with this PR:

TestCopyVolumeUidGid
TestCopyVolumeContent

erikh · 2014-09-01T20:11:30Z

I have tested these by hand and they do not fail, which makes me think that the tests are busted, and not the code.

Testing master now, will reply in a bit.

-Erik

On Sep 1, 2014, at 11:34 AM, Alexandr Morozov [email protected] wrote:

Also, next tests fails with this PR:

TestCopyVolumeUidGid
TestCopyVolumeContent
—
Reply to this email directly or view it on GitHub.

erikh · 2014-09-01T20:20:54Z

They don’t fail on master :( I will keep investigating.

On Sep 1, 2014, at 1:11 PM, Erik Hollensbe [email protected] wrote:

I have tested these by hand and they do not fail, which makes me think that the tests are busted, and not the code.

Testing master now, will reply in a bit.

-Erik

On Sep 1, 2014, at 11:34 AM, Alexandr Morozov [email protected] wrote:

Also, next tests fails with this PR:

TestCopyVolumeUidGid
TestCopyVolumeContent
—
Reply to this email directly or view it on GitHub.

erikh · 2014-09-01T20:37:21Z

Added a test for ENTRYPOINT []. All tests pass except for the two volumes ones.

The reason I’m commenting now is because the behavior might be different.

Now:

When ENTRYPOINT is blank, and CMD is also blank: set ENTRYPOINT to ‘/bin/sh -c’
When ENTRYPOINT is blank, and CMD is set, set ENTRYPOINT to an empty array.

I think this is the desired behavior, but please advise if there was a specific way it was done before.

On Sep 1, 2014, at 1:20 PM, Erik Hollensbe [email protected] wrote:

They don’t fail on master :( I will keep investigating.

On Sep 1, 2014, at 1:11 PM, Erik Hollensbe [email protected] wrote:

I have tested these by hand and they do not fail, which makes me think that the tests are busted, and not the code.

Testing master now, will reply in a bit.

-Erik

On Sep 1, 2014, at 11:34 AM, Alexandr Morozov [email protected] wrote:

Also, next tests fails with this PR:

TestCopyVolumeUidGid
TestCopyVolumeContent
—
Reply to this email directly or view it on GitHub.

vieux · 2014-09-01T20:39:01Z

You can't change it or it'll break existing Dockerfiles

erikh · 2014-09-01T20:43:04Z

What is the existing behavior? If I change that code again, other tests will break.

On Sep 1, 2014, at 1:39 PM, Victor Vieux [email protected] wrote:

You can't change it or it'll break existing Dockerfiles

—
Reply to this email directly or view it on GitHub.

vieux · 2014-09-01T20:45:20Z

When ENTRYPOINT is blank, and CMD is also blank: set ENTRYPOINT to ‘/bin/sh -c’
This seem really bad, because there is no way to start a binary without sh -c from the cli

erikh · 2014-09-01T20:51:32Z

In TestBuildWithVolumeOwnership, ENTRYPOINT and CMD are inherited and ENTRYPOINT appears to be blank. This test will fail due to varargs in the run command not being processed correctly.

TestBuildNoContext overrides CMD but not ENTRYPOINT, which is also inherited, but from no context. This will break if ENTRYPOINT is blank, which it is in this case because of the busybox image, and since it is a ‘sh -c’ type of CMD. The test will fail complaining of no ‘echo’ in $PATH, which never gets set because sh -c is never run.

These were resolved by the changes in this PR. I’m happy to integrate whatever you want, but as you can see this gets fairly hairy (and these is only a few of the tests). Just let me know what the desired behavior should be, and I’ll try to match it.

On Sep 1, 2014, at 1:45 PM, Victor Vieux [email protected] wrote:

When ENTRYPOINT is blank, and CMD is also blank: set ENTRYPOINT to ‘/bin/sh -c’
This seem really bad, because there is no way to start a binary without sh -c from the cli

—
Reply to this email directly or view it on GitHub.

erikh · 2014-09-02T18:09:01Z

Just a note on the volume tests — these pass when run independently. I’m wondering if some state is persisting.

Making notes for later, please don’t mind the noise.

On Sep 1, 2014, at 1:51 PM, Erik Hollensbe [email protected] wrote:

In TestBuildWithVolumeOwnership, ENTRYPOINT and CMD are inherited and ENTRYPOINT appears to be blank. This test will fail due to varargs in the run command not being processed correctly.

TestBuildNoContext overrides CMD but not ENTRYPOINT, which is also inherited, but from no context. This will break if ENTRYPOINT is blank, which it is in this case because of the busybox image, and since it is a ‘sh -c’ type of CMD. The test will fail complaining of no ‘echo’ in $PATH, which never gets set because sh -c is never run.

These were resolved by the changes in this PR. I’m happy to integrate whatever you want, but as you can see this gets fairly hairy (and these is only a few of the tests). Just let me know what the desired behavior should be, and I’ll try to match it.

On Sep 1, 2014, at 1:45 PM, Victor Vieux [email protected] wrote:

When ENTRYPOINT is blank, and CMD is also blank: set ENTRYPOINT to ‘/bin/sh -c’
This seem really bad, because there is no way to start a binary without sh -c from the cli

—
Reply to this email directly or view it on GitHub.

tianon · 2014-09-02T19:04:32Z

+1; "sh -c" will definitely break stuff

When the command is finally run, we do a LookupPath on the first item in
the array, which honors $PATH.

erikh · 2014-09-02T19:14:41Z

I’ll review whether PATH is being honored, but I didn’t touch anything in runconfig, so I can’t see why it would change.

On Sep 2, 2014, at 12:04 PM, Tianon Gravi [email protected] wrote:

+1; "sh -c" will definitely break stuff

When the command is finally run, we do a LookupPath on the first item in
the array, which honors $PATH.
—
Reply to this email directly or view it on GitHub.

erikh · 2014-09-03T12:35:57Z

Ok, this should be ready for merge now; all tests pass. Please review @LK4D4 @tiborvass

LK4D4 · 2014-09-03T13:15:37Z

integration-cli/docker_cli_build_test.go

I prefer this as separate test as edge case.

done, PTAL
On Sep 3, 2014, at 6:15 AM, Alexandr Morozov [email protected] wrote:

In integration-cli/docker_cli_build_test.go:

@@ -766,6 +766,25 @@ func TestBuildEntrypoint(t *testing.T) {
if res != expected {
t.Fatalf("Entrypoint %s, expected %s", res, expected)
}
+

deleteImages(name)

expected = "[]"
I prefer this as separate test as edge case.

—
Reply to this email directly or view it on GitHub.

erikh · 2014-09-04T20:28:24Z

ping @LK4D4 @tianon PTAL, all tests are passing now.

tiborvass · 2014-09-04T20:58:18Z

@erikh you did not remove the test you extracted, from the original test.

erikh · 2014-09-04T21:35:54Z

lol whoops, fixed now

On Sep 4, 2014, at 1:58 PM, Tibor Vass [email protected] wrote:

@erikh you did not remove the test you extracted, from the original test.

—
Reply to this email directly or view it on GitHub.

brauner · 2014-09-04T22:58:49Z

Sorry, but may I chime in here. I have question about this. I'm setting up some Docker containers that run certain programming environments. Hence, I run the containers with the program, say Python, as default entrypoint because I want to be able to call the program with options. But most of the time I just run it without any additional options. That's why I specify in the Dockerfiles:

CMD [" "]
ENTRYPOINT ["/usr/bin/python"]

This allows me to run the programm either with or without options. Should I do this differently now? If so I would change this pretty soon should Docker break this behaviour at some point.

tiborvass · 2014-09-04T23:08:09Z

@lord-garbage

Right now, you don't need the empty CMD trick, if you're not doing FROM image where image already has a CMD set.
In Docker 1.3, you won't need the empty CMD trick at all.
I note that you did CMD [" "] with a space between the double quotes. Don't you have the same behavior without the space? @erikh can we make sure we handle the case with the space? I guess we expect (cmd " ").

erikh · 2014-09-04T23:43:16Z

I can write a test

brauner · 2014-09-05T06:51:20Z

My bad, I don't actually have CMD [" "]. Instead I have CMD []. And I have the FROM part set.

tiborvass · 2014-09-05T13:43:53Z

LGTM

crosbymichael · 2014-09-10T23:38:57Z

builder/dispatchers.go

Shouldn't this check happen earlier?

no. unless I'm misunderstanding you, this is propagated from the parser, so there will only be one argument if the CMD was not json encoded.

Well you are consuming the value in args = append([]string{b.image}, args...) then later checking if it is == "" before erroring out.

ohhh. ok. I'll fix it rihgt away.

Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <[email protected]> (github: erikh)

autoConfig seeding of entrypoint. Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <[email protected]> (github: erikh)

… function Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <[email protected]> (github: erikh)

dispatcher Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <[email protected]> (github: erikh)

erikh · 2014-09-11T13:02:51Z

@crosbymichael last commit fixes your concerns.

crosbymichael · 2014-09-11T17:42:03Z

LGTM

crosbymichael · 2014-09-11T17:42:20Z

@erikh is there an issues that this fixes?

Fix cmd and entrypoint handling in parser

LK4D4 reviewed Sep 3, 2014
View reviewed changes

crosbymichael reviewed Sep 10, 2014
View reviewed changes

Erik Hollensbe added 2 commits September 11, 2014 06:00

builder/parser/dumper: Handle newlines appropriately.

234f0e4

Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <[email protected]> (github: erikh)

builder/parser: fix handling of empty strings as the only argument.

6b322ad

Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <[email protected]> (github: erikh)

Erik Hollensbe added 6 commits September 11, 2014 06:00

builder: Fix a small bug that could improperly cache.

3a6d122

Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <[email protected]> (github: erikh)

builder: Fix entrypoint and cmd semantics in evaluator. Test c/o @cnf

ac10799

Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <[email protected]> (github: erikh)

evaluator: ensure entrypoint stays blank if set blank while CMD is set.

78a847a

Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <[email protected]> (github: erikh)

Do not populate path to prevent runconfig merge conflict; remove

92f885d

autoConfig seeding of entrypoint. Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <[email protected]> (github: erikh)

move entrypoint integration-cli test for blank entrypoint to separate…

576416c

… function Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <[email protected]> (github: erikh)

builder: Fix an error check being done at the wrong spot in run

91bed43

dispatcher Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <[email protected]> (github: erikh)

crosbymichael added a commit that referenced this pull request Sep 11, 2014

Merge pull request #7806 from erikh/fix_cmd_handling_in_parser

08f1a91

Fix cmd and entrypoint handling in parser

crosbymichael merged commit 08f1a91 into moby:master Sep 11, 2014

erikh mentioned this pull request Sep 11, 2014

unsetting CMD as the last instruction in a docker file causes it to be set to [""] #7798

Closed

bjaglin mentioned this pull request Oct 12, 2014

[1.3.0-rc] Regression for ONBUILD {CMD|ENTRYPOINT|RUN} - array syntax parsed but dropped #8520

Closed

This was referenced Dec 3, 2020

vendor: BuildKit 950603da215ae03b843f3f66fbe86c4876a6f5a1 #41745

Merged

Revert "Ensure ENTRYPOINT command has at least one argument" moby/buildkit#1874

Merged

Fix cmd and entrypoint handling in parser #7806

Fix cmd and entrypoint handling in parser #7806

Uh oh!

Conversation

erikh commented Aug 30, 2014

Uh oh!

erikh commented Aug 30, 2014

Uh oh!

thaJeztah commented Aug 30, 2014

Uh oh!

erikh commented Aug 30, 2014

Uh oh!

thaJeztah commented Aug 30, 2014

Uh oh!

LK4D4 commented Aug 30, 2014

Uh oh!

vieux commented Sep 1, 2014

Uh oh!

LK4D4 commented Sep 1, 2014

Uh oh!

erikh commented Sep 1, 2014

Uh oh!

erikh commented Sep 1, 2014

Uh oh!

erikh commented Sep 1, 2014

Uh oh!

vieux commented Sep 1, 2014

Uh oh!

erikh commented Sep 1, 2014

Uh oh!

vieux commented Sep 1, 2014

Uh oh!

erikh commented Sep 1, 2014

Uh oh!

erikh commented Sep 2, 2014

Uh oh!

tianon commented Sep 2, 2014

Uh oh!

erikh commented Sep 2, 2014

Uh oh!

erikh commented Sep 3, 2014

Uh oh!

LK4D4 Sep 3, 2014

Choose a reason for hiding this comment

Uh oh!

erikh Sep 3, 2014

Choose a reason for hiding this comment

Uh oh!

erikh commented Sep 4, 2014

Uh oh!

tiborvass commented Sep 4, 2014

Uh oh!

erikh commented Sep 4, 2014

Uh oh!

brauner commented Sep 4, 2014

Uh oh!

tiborvass commented Sep 4, 2014

Uh oh!

erikh commented Sep 4, 2014

Uh oh!

brauner commented Sep 5, 2014

Uh oh!

tiborvass commented Sep 5, 2014

Uh oh!

crosbymichael Sep 10, 2014

Choose a reason for hiding this comment

Uh oh!

erikh Sep 10, 2014

Choose a reason for hiding this comment

Uh oh!

crosbymichael Sep 11, 2014

Choose a reason for hiding this comment

Uh oh!

erikh Sep 11, 2014

Choose a reason for hiding this comment

Uh oh!

erikh commented Sep 11, 2014

Uh oh!

crosbymichael commented Sep 11, 2014

Uh oh!