Skip to content

Dockerfile improvements #342

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

Closed
wants to merge 3 commits into from

Conversation

clushie
Copy link

@clushie clushie commented Feb 22, 2021

First of all tracking this issue #341

In addition I've cleaned up the Dockerfile a bit, such as by adding SHELL as a global parameter instead of running set -ue couple of times.

Last of all, I'm adding a check in Debian images that fixes incorrectly trying to apt-manual a library that is under /usr/local/bin, making it optionally possible to add -o pipefail as an additional build-time fail-safe.

@clushie clushie force-pushed the fix_pipefailure branch 2 times, most recently from e745fb0 to 265702f Compare February 22, 2021 14:08
@tianon
Copy link
Member

tianon commented Feb 22, 2021

While interesting (and I'm sure fun to work on!), these changes are a bit too invasive, especially to fix an otherwise pretty minor issue.

For example, the changed SHELL instruction will be inherited by our users, so this has the potential to break their Dockerfiles (and I'm not sure it's worth changing at the start to change back at the end).

I think the solution with an if statement + explicit exit would be preferred (and needs to be applied to all the templated files too), if this is something you wish to continue working on. 👍

(Also, IMO it's worth verifying that the current code actually does fail to exit properly when ruby is installed.)

@clushie
Copy link
Author

clushie commented Feb 22, 2021

Hey, thanks for the quick response :D.

I've ended up tweaking those things since we needed Jessie/2.4.10 support and figured it might be worth contributing my findings upstream.

I've split them into separate commits so it's easier to drop what isn't needed.

Of course, you are right - assuming set -uex for everyone else is definitively wrong. I should fix that in my own build too. Perhaps we can set it afterwards to the default SHELL directive again? Otherwise I'm happy to just drop the commit.
So the default-shell for docker is ["/bin/sh", "-c"], though you can't "reset" the SHELL parameter to nothing. When you docker image inspect you can see that most images don't have a Shell configured. Even if you configure a shell to the "default" value it will still be different to having nothing in the manifest.

I'll verify that it fails with ruby packages installed and report back.

I think the two important commits are the /usr/local/lib and the ruby installed check.

I didn't commit the ./update.sh, do you want it to be included in the final commit/s?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants