Skip to content
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.

Sign up for GitHub

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

Update Makefile.docker-base #965

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update Makefile.docker-base #965

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions apps/docker-base/Makefile.docker-base
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,40 @@
# -*- makefile -*-
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't get rid of this line. It's magic that helps some editors.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your reply.
I will check all this

# Makefile for Docker image setup

help:
@echo "This should be run as part of the Dockerfile"
false

user-setup:
# Create app group and user
groupadd --gid 1000 app
useradd -d /app --uid 1000 --gid app app
chown -R app:app /app

apt-setup:
# Configure APT to keep downloaded packages
Copy link
Collaborator

Choose a reason for hiding this comment

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

because we cache them as part of the build so want them to be retained.

rm -f /etc/apt/apt.conf.d/docker-clean
echo 'Binary::apt::APT::Keep-Downloaded-Packages "true";' > /etc/apt/apt.conf.d/keep-cache

apt-install:
DEBIAN_FRONTEND=noninteractive apt update
# gcc and python3-dev needed on arm for guidance
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't lose this comment. It's very important to understand why we're installing gcc & python3-dev (which otherwise we wouldn't expect to need).

DEBIAN_FRONTEND=noninteractive apt -y install --no-install-recommends python3-poetry gcc python3-dev
# Install necessary packages without recommended packages
DEBIAN_FRONTEND=noninteractive apt update && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to do the && thing here.
make will run both of the steps and will abort if the previous one fails.

DEBIAN_FRONTEND=noninteractive apt -y install --no-install-recommends python3-poetry gcc python3-dev && \
# Clean up to reduce image size
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't want this because of the caching. From docker-base/Dockerfile.buildx

RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \
    --mount=type=cache,target=/var/lib/apt,sharing=locked \
    make -f Makefile.docker-base apt-install

The first two lines are caching the directories, so they don't show up in the image.

You can verify that on the existing containers:

% docker run -it -u root arynai/sycamore-base bash
root@9983ae28df15:/app# find /var/cache/apt
/var/cache/apt
/var/cache/apt/archives
/var/cache/apt/archives/lock
/var/cache/apt/archives/partial
root@9983ae28df15:/app# find /var/lib/apt
/var/lib/apt
/var/lib/apt/mirrors
/var/lib/apt/mirrors/partial
/var/lib/apt/periodic
/var/lib/apt/extended_states
/var/lib/apt/lists

apt clean && rm -rf /var/lib/apt/lists/*

non-root-files-check:
# Check for any files owned by root
find . -uid 0 -ls
test $$(find . -uid 0 -print | wc -w) = 0

record-version:
# Ensure GIT_COMMIT is set and record the version
test "$(GIT_COMMIT)" != ""
test "$(GIT_COMMIT)" != "unknown"
touch .git.commit.$(GIT_COMMIT)

# Allow images that depend on the docker base image to verify that the version for their
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't get rid of the longer comment that explains why we are doing this.
Feel free to move it under the target for consistency, but readers of the code need to understand what version consistency is being checked and why.

# source code is consistent with the version in the base image. If the code is inconsistent,
# the resulting image could behave unexpectedly.
check-version-compatibility:
# Verify the version consistency
test "$(GIT_COMMIT)" != ""
test "$(GIT_COMMIT)" != "unknown"
ls .git.commit.*
Expand Down
Loading