Update Dockerfile to use multi-stage builds #4746

heliocastro · 2021-11-24T08:06:09Z

Uses multi-stage build to reduce final container
Harmonize the base image to use same OpenJDK base from Eclipse Temurin and Ubuntu Focal LTS
Make all tooling versions customizable through docker build args
Each tooling is built in separate components, improving the process to add or remove specific tools

docker/python.sh

docker/ort-wrapper.sh

docker/python.sh

scripts/docker_build.sh

Dockerfile

docker/android.sh

docker/nodejs.sh

maxhbr

just some comments

Dockerfile

docker/python.sh

docker/bash_bootstrap.sh

Dockerfile

tsteenbe · 2021-11-24T18:19:02Z

Dockerfile

+    && rbenv global ${RUBY_VERSION} \
+    && gem install bundler cocoapods
+
+# Install 'CocoaPods'. As https://github.com/CocoaPods/CocoaPods/pull/10609 is needed but not yet released.


CocoaPods/CocoaPods#10609 was merged and I believe also released?

Maybe, but i do not want change this since is this way on upstream Dockerfile. If we merge, then in later step i change to newer version of CocoaPods.

Please see again how this is now done in the "legacy" Dockerfile.

Dockerfile

docker/bash_bootstrap.sh

Signed-off-by: Helio Chissini de Castro <[email protected]>

- Python default version is 3.10.2 - Python2 can be optionally installed through build-arg PYTHON2_VERSION - Scancode build was removed in favor of pip based version Signed-off-by: Helio Chissini de Castro <[email protected]>

Signed-off-by: Helio Chissini de Castro <[email protected]>

- Update Ruby to 3.1.2 ( ssl3 ) - Update PHP composer to latest using installer - Update PIP - Move work user to Ort to be Batect compliant - Update NodeJS for current LTS Signed-off-by: Helio Chissini de Castro <[email protected]>

…mand line Signed-off-by: Helio Chissini de Castro <[email protected]>

Signed-off-by: Helio Chissini de Castro <[email protected]>

- Move python packages to last stage to redice big copy and permission long chmod - Change android sdk permissions - Install pnpm ( missing ) - Update npm version - Fix pyenv version Signed-off-by: Helio Chissini de Castro <[email protected]>

- Add locale on image defaults to en_US.UTF-8 Signed-off-by: Helio Chissini de Castro <[email protected]>

Signed-off-by: Helio Chissini de Castro <[email protected]>

… poetry Signed-off-by: Helio Chissini de Castro <[email protected]>

- Fix android sdk folder permissions - Add buildx as default docker engine build and add implicit amd64 to build on non x86 based systems Signed-off-by: Helio Chissini de Castro <[email protected]>

Issue addressed

sschuberth · 2022-09-08T14:28:42Z

Cross-posting from Slack: Please don't merge this with a whopping 43 (!) commits in the history. I guess most of this should be squashable, or?

sschuberth

Sorry, the Git history in this PR is a mess. I'm blocking the merge until the TSC has decided on how to proceed.

sschuberth · 2022-09-08T14:33:34Z

Dockerfile

@@ -202,7 +202,9 @@ ENV LANG=en_US.UTF-8
 ENV LANGUAGE=en_US:en
 ENV LC_ALL=en_US.UTF-8

-RUN echo $LANG > /etc/locale.gen
+RUN echo $LANG > /etc/locale.gen \


How is this related to "Make batect understand entrypoint"?

sschuberth · 2022-09-08T14:35:31Z

@mnonnenmacher @MarcelBochtler @fviernau could you live with the Git history in this PR?

mnonnenmacher · 2022-09-08T19:13:21Z

@mnonnenmacher @MarcelBochtler @fviernau could you live with the Git history in this PR?

@sschuberth I would not merge it like this, because just from a short look I see so many issues:

43 commits is far too much for a single PR. This is impossible to review.
Many commits change multiple things at once. Just as one example, the "Revert to Java 11" commit changes the pyenv installation and there is no explanation in the commit message if this is related to the base image change or just completely unrelated.
There are a lot of typos and hard to understand sentences in the commit messages.
Several commits contain lists of changes which shows they should have been separate commits.
There is one commit that creates a legacy Dockerfile, but this is not the current version of the file but an old version. Instead, there is a separate commit that updates the legacy file.
There are version updates all over the place, it's unfeasible to check if all of these are updates related to some old version of the original Dockerfile or to the current Dockerfile. For example, the first commit downgrades ScanCode only to upgrade it again in a later commit.
Commit message prefixes are inconsistent, some start with "Docker fix:" or "Docker update:" or "Dockerfile:" or "docker:".
It's unclear why some entries were added to the .gitignore.
The link to the BuildKit docs was removed from the README.

And this is not the complete list.

I understand that this PR was worked on for almost a year now, and that this was probably a bit frustrating. So I want to thank @heliocastro for being to persistent with your efforts to improve our Docker setup. But nevertheless this PR should follow our standards. We have put a lot of effort into maintaining a clean Git history for this project because it brings a lot of benefits, and I would not like to ignore this now just because someone has an urge to merge these changes.

My suggestion to move this forward in a clean way would be to squash all changes into just a few commits, so that the PR can be reviewed properly, for example:

One commit to copy the current Dockerfile to the legacy directory.
A few commits that apply the relevant changes on top of the current Dockerfile, but without all the version updates that just repeat version updates which already happened in our current Dockerfile. Or even squashing all changes into a single commit would be better than the current state of the PR.
One commit for each version update that changes a version compared to our current Dockerfile.

@sschuberth @fviernau @MarcelBochtler Does one of you maybe have a better idea how to clean this up?

And as a general comment, if you work on a contribution that takes a long time, to a project with mandatory code review, it is super important to rebase your changes on top of the current main branch on a regular basis. Yes, this can be a lot of work, but it is the responsibility of the author to make sure that their changes are in a reviewable state.

sschuberth · 2022-09-09T06:30:02Z

Does one of you maybe have a better idea how to clean this up?

Looks like a clean-up was already done in #5760. I'll close this PR in favor of that one. Let's continue the discussion over there.

heliocastro requested a review from a team as a code owner November 24, 2021 08:06

tsteenbe requested changes Nov 24, 2021

View reviewed changes

heliocastro force-pushed the docker_overhaul branch 3 times, most recently from efbd9e1 to dace4b4 Compare November 24, 2021 11:23

maxhbr reviewed Nov 24, 2021

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

docker/python.sh Outdated Show resolved Hide resolved

docker/bash_bootstrap.sh Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

heliocastro force-pushed the docker_overhaul branch 4 times, most recently from c93e88f to 998daa5 Compare November 24, 2021 13:17

heliocastro requested review from tsteenbe and removed request for a team November 24, 2021 13:19

tsteenbe requested changes Nov 24, 2021

View reviewed changes

heliocastro force-pushed the docker_overhaul branch 3 times, most recently from 0106250 to 25d0408 Compare November 25, 2021 07:37

heliocastro requested a review from tsteenbe November 25, 2021 07:50

This comment has been minimized.

Sign in to view

maxhbr reviewed Nov 25, 2021

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

heliocastro force-pushed the docker_overhaul branch from ef532fa to dfe9dcc Compare November 26, 2021 09:34

maxhbr reviewed Nov 26, 2021

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

maxhbr reviewed Nov 26, 2021

View reviewed changes

docker/bash_bootstrap.sh Outdated Show resolved Hide resolved

heliocastro force-pushed the docker_overhaul branch 3 times, most recently from 3ce67da to 1e59dca Compare December 3, 2021 10:56

heliocastro force-pushed the docker_overhaul branch 5 times, most recently from c219a54 to 319991d Compare December 9, 2021 09:06

Helio Chissini de Castro added 18 commits September 8, 2022 14:59

Docker update: Sync with upstream build script

c8fe7ce

Signed-off-by: Helio Chissini de Castro <[email protected]>

Docker fix: Don't ignore dockerignore

91d3fca

Signed-off-by: Helio Chissini de Castro <[email protected]>

Docker update: Install CocoaPods direct from GEM now

b3e1477

Signed-off-by: Helio Chissini de Castro <[email protected]>

Docker update: Add Python2 option and Scancode as pip

466811e

- Python default version is 3.10.2 - Python2 can be optionally installed through build-arg PYTHON2_VERSION - Scancode build was removed in favor of pip based version Signed-off-by: Helio Chissini de Castro <[email protected]>

Docker fix: run locale-gen one time only

bdd38d7

Signed-off-by: Helio Chissini de Castro <[email protected]>

Dockerfile: Update GRADLE_HOME and Bower

73a0521

Signed-off-by: Helio Chissini de Castro <[email protected]>

Dockerfile: Add missing openssl on Ubuntu install

604c239

Signed-off-by: Helio Chissini de Castro <[email protected]>

Dockerfile: Update legacy dockerfile

cc45df2

Signed-off-by: Helio Chissini de Castro <[email protected]>

Dockerfile: Add tooling versions in docker_build.sh to easy updates

826acfd

Signed-off-by: Helio Chissini de Castro <[email protected]>

docker: Update to Ubuntu latest LTS ( Jammy )

14d8d13

- Update Ruby to 3.1.2 ( ssl3 ) - Update PHP composer to latest using installer - Update PIP - Move work user to Ort to be Batect compliant - Update NodeJS for current LTS Signed-off-by: Helio Chissini de Castro <[email protected]>

Docker: Allow user to decide which buildArgs is needed passing on com…

cb88aac

…mand line Signed-off-by: Helio Chissini de Castro <[email protected]>

batect: Remove enable-buildkit as new docker has it embedded

27f3c30

Signed-off-by: Helio Chissini de Castro <[email protected]>

docker: Add python-inspector and update python patch version

b39363a

Signed-off-by: Helio Chissini de Castro <[email protected]>

docker: Small fixes

259f6e0

- Move python packages to last stage to redice big copy and permission long chmod - Change android sdk permissions - Install pnpm ( missing ) - Update npm version - Fix pyenv version Signed-off-by: Helio Chissini de Castro <[email protected]>

docker: Make batect understand entrypoint

9af1ac0

- Add locale on image defaults to en_US.UTF-8 Signed-off-by: Helio Chissini de Castro <[email protected]>

docker: Add buildkit back to batect

980c7cd

Signed-off-by: Helio Chissini de Castro <[email protected]>

docker: Restore python to build stage and fix versions for pipenv and…

3afb2ba

… poetry Signed-off-by: Helio Chissini de Castro <[email protected]>

docker: Minor fixes

fd06d32

- Fix android sdk folder permissions - Add buildx as default docker engine build and add implicit amd64 to build on non x86 based systems Signed-off-by: Helio Chissini de Castro <[email protected]>

heliocastro force-pushed the docker_overhaul branch from a11d7ff to fd06d32 Compare September 8, 2022 12:59

tsteenbe approved these changes Sep 8, 2022

View reviewed changes

nicorikken mentioned this pull request Sep 8, 2022

JAVA_HOME is set to an invalid directory alliander-opensource/ort-container#11

Open

sschuberth requested changes Sep 8, 2022

View reviewed changes

sschuberth closed this Sep 9, 2022

sschuberth mentioned this pull request Sep 9, 2022

Docker: Component based Multi stage docker #5760

Closed

heliocastro deleted the docker_overhaul branch November 12, 2023 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Dockerfile to use multi-stage builds #4746

Update Dockerfile to use multi-stage builds #4746

heliocastro commented Nov 24, 2021 •

edited

Loading

maxhbr left a comment

tsteenbe Nov 24, 2021

heliocastro Nov 25, 2021

sschuberth Feb 22, 2022

This comment has been minimized.

sschuberth commented Sep 8, 2022

sschuberth left a comment

sschuberth Sep 8, 2022

sschuberth commented Sep 8, 2022

mnonnenmacher commented Sep 8, 2022 •

edited

Loading

sschuberth commented Sep 9, 2022

Update Dockerfile to use multi-stage builds #4746

Update Dockerfile to use multi-stage builds #4746

Conversation

heliocastro commented Nov 24, 2021 • edited Loading

maxhbr left a comment

Choose a reason for hiding this comment

tsteenbe Nov 24, 2021

Choose a reason for hiding this comment

heliocastro Nov 25, 2021

Choose a reason for hiding this comment

sschuberth Feb 22, 2022

Choose a reason for hiding this comment

This comment has been minimized.

sschuberth commented Sep 8, 2022

sschuberth left a comment

Choose a reason for hiding this comment

sschuberth Sep 8, 2022

Choose a reason for hiding this comment

sschuberth commented Sep 8, 2022

mnonnenmacher commented Sep 8, 2022 • edited Loading

sschuberth commented Sep 9, 2022

heliocastro commented Nov 24, 2021 •

edited

Loading

mnonnenmacher commented Sep 8, 2022 •

edited

Loading