Can't run commands with arguments any more after removing `--cmd` option from entry point

[jpkotta]

Example: docker run -it --rm -v $PWD:/workdir --workdir /workdir crops/poky:ubuntu-16.04 ls /

I would expect this command to list files in the container's root, but it lists files in $PWD. This is because it's ignoring everything after the first command line argument; it's only running ls not ls /.

Ideally, I'd be able to run entire command lines like source foo && some_command && bash like before.

[rewitt1]

@btgoodwin,

This seems to be caused by https://github.com/crops/poky-container/commit/56ef1eeef16a8d610b59bc9bf0d1a5e6aa8fe9e4. Can you give me an example where "$*" doesn't work?

I need to write some tests for this if people are using it regularly.

[btgoodwin]

@rewitt1,

The test case was in the related PR description. It's that multi-line sh command that is followed by an if-statement. The $* expansion reformats the string such a way that it fails to parse it as valid multi-line command (actual error message).

[jpkotta]

Is that test script definitely running correctly? Maybe I'm doing something wrong, but it seems like it's just running sh because it's the first word and everything else is irrelevant (this is with the latest image where poky-launch.sh uses "$@"). Does putting sh -c false in the test script do what's expected?

[btgoodwin]

I noticed that when I ran it too, and as an automated test script, it isn't great since it's starting a sh instance. My only use of it was knowing GitLab's runner was passing that as its command to the container, and that was failing. So my "test" was to tweak the container until I could pass that same script into it.

On Wed, Sep 18, 2019 at 7:24 PM jpkotta notifications@github.com wrote:

Is that test script definitely running correctly? Maybe I'm doing something wrong, but it seems like it's just running sh because it's the first word. Does putting sh -c false in the test script do what's expected?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/crops/poky-container/issues/39?email_source=notifications&email_token=ABJLASZHUI3CC2UAAAZYBOTQKK2DBA5CNFSM4IXGYVR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7BXRAQ#issuecomment-532904066, or mute the thread https://github.com/notifications/unsubscribe-auth/ABJLAS5IWW3E67EVYFGRTZTQKK2DBANCNFSM4IXGYVRQ .

[gizero]

This issue is hitting me as well. Anyone working on a fix? I had to pull the image from scratch after cleanup and got my CI stuck... Maybe we should consider doing proper releases by tagging versions of the project instead of relying only on latest: this would allow rollback in situations like this. I know I can build a reverted image myself, but...

[jpkotta]

IMO, the fix is to go back to "$*". I don't think "$@" fixed that test script at all, I think it just stopped failing because it's ignoring everything after the sh.

[rewitt1]

I'm going to try to look into it today. I'm most likely revert https://github.com/crops/poky-container/commit/56ef1eeef16a8d610b59bc9bf0d1a5e6aa8fe9e4. Then I need to add some tests for the previous expected behavior, and lastly take some time to look into the original test script from https://github.com/crops/poky-container/pull/37#issue-313710937.

I didn't do any verification originally, because as I said, "cmd" was intended to be debug and I have limited time right now. However, I also don't like breaking existing behavior, and if we ever want to have an official docker image it has to support this behavior.

This issue is hitting me as well. Anyone working on a fix? I had to pull the image from scratch after cleanup and got my CI stuck... Maybe we should consider doing proper releases by tagging versions of the project instead of relying only on latest: this would allow rollback in situations like this.

@gizero, If we were going to have multiple versions of the images on dockerhub, I'd still want to set a reasonable limit on the number, because storage isn't free, and it would make the matrix of builds on travis go from (n distros) to (n distros) * (x versions). I want to try and be respectful of the resources travis gives us and make sure to build the minimum required.

I have considered the idea of only storing one "official distro" image on dockerhub, and if anyone wanted to use the others, they have to build the other distros. They other distros would still be tested, but not preserved on dockerhub. This should also make it easier to eventually have the building of the image done by docker.

This particular breakage in #39 was due to a lack of testing, which is my fault, but as I said, I'd always treated the behavior as "debug". If all expected behavior is tested, we shouldn't need to manage releases, which helps me keeps things working in my limited available time.

[rewitt1]

I fixed part of the problem with https://github.com/crops/extsdk-container/commit/426ebc1ea6c5329e92b34fa06b4bd06ea581bc7b. This now means that a command like the one below, should work.

docker run -it --rm crops/poky "ls / && echo foo"

However, the command below would not work.

docker run -it --rm crops/poky ls /

This is because the arguments are getting passed to "exec bash -c" instead of directly being executed. In order to better model the behavior docker would like, and thus to behave most like what is expected, I'm going to change to a direct exec. What this means is that

docker run -it --rm crops/poky "source foo && ls / && bash -i"

would become

docker run -it --rm crops/poky bash -c "source foo && ls / && bash -i"

Please let me know if anyone is opposed to that, because it seems like the most flexible and correct thing to do.

All that being said, I still couldn't get the example from @btgoodwin to work. Not even on a official distro image.

docker run -it --rm ubuntu:18.04 $(cat test.sh) 
[: 1: [: Syntax error: end of file unexpected

So even after switching to a direct exec, I still don't think that example would work. @btgoodwin, can you point me to the gitlab code that would be executing such a command?

[btgoodwin]

@rewitt1 That error is a new one for me. The one we were seeing would print the whole shell detection script that was being passed in from the runner. This seems to be where that code came from. Down on line 227 in GetConfiguration, it stitches together the docker command with that bashDetectShell block that ultimately gets passed to the container causing it to crash out with that similar syntax error.

[rewitt1]

I just merged the change to directly exec rather than use "bash -c". This will have the effects I mentioned in https://github.com/crops/poky-container/issues/39#issuecomment-534292257.

@btgoodwin, I more closely tried to model the code you linked to at gitlab and changed the script to be:

if [ -x /usr/local/bin/bash ]; then
        exec /usr/local/bin/bash 
elif [ -x /usr/bin/bash ]; then
        exec /usr/bin/bash 
elif [ -x /bin/bash ]; then
        exec /bin/bash 
elif [ -x /usr/local/bin/sh ]; then
        exec /usr/local/bin/sh 
elif [ -x /usr/bin/sh ]; then
        exec /usr/bin/sh 
elif [ -x /bin/sh ]; then
        exec /bin/sh 
elif [ -x /busybox/sh ]; then
        exec /busybox/sh 
else
        echo "shell not found"
fi

And I instead run:

docker run -it --rm ubuntu:16.04 sh -c "$(cat test.sh)"

which seems to work. So I think you'll still be good with the gitlab ci.

[btgoodwin]

Awesome, thank you. I look forward to testing it.

On Tue, Sep 24, 2019 at 2:27 PM Randy Witt notifications@github.com wrote:

I just merged the change to directly exec rather than use "bash -c". This will have the effects I mentioned in #39 (comment) https://github.com/crops/poky-container/issues/39#issuecomment-534292257 .

@btgoodwin https://github.com/btgoodwin, I more closely tried to model the code you linked to at gitlab and changed the script to be:

if [ -x /usr/local/bin/bash ]; then exec /usr/local/bin/bash elif [ -x /usr/bin/bash ]; then exec /usr/bin/bash elif [ -x /bin/bash ]; then exec /bin/bash elif [ -x /usr/local/bin/sh ]; then exec /usr/local/bin/sh elif [ -x /usr/bin/sh ]; then exec /usr/bin/sh elif [ -x /bin/sh ]; then exec /bin/sh elif [ -x /busybox/sh ]; then exec /busybox/sh else echo "shell not found" fi

And I instead run:

docker run -it --rm ubuntu:16.04 sh -c "$(cat test.sh)"

which seems to work. So I think you'll still be good with the gitlab ci.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/crops/poky-container/issues/39?email_source=notifications&email_token=ABJLAS7NMU3NCHZPWIAMEPDQLJL2XA5CNFSM4IXGYVR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7PLBYA#issuecomment-534687968, or mute the thread https://github.com/notifications/unsubscribe-auth/ABJLAS5LUECNNNICWCIWDO3QLJL2XANCNFSM4IXGYVRQ .