Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

fix: Fixes #2464 Replace git use for finding project root. Dedicated dockerignore with .git/ for 277MB image #13472

Merged
merged 5 commits into from
Mar 8, 2023
Merged

fix: Fixes #2464 Replace git use for finding project root. Dedicated dockerignore with .git/ for 277MB image #13472

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fbcf8ab
fixes #13071
ltfschoen Jan 5, 2023
fcf2fb8
remove warning
ltfschoen Jan 5, 2023
0221536
replace use of git for finding project root. add dedicated dockerigno…
ltfschoen Jan 5, 2023
f56a882
Merge branch 'master' into luke/2464-optimise-docker
ltfschoen Feb 25, 2023
4760114
fix command to switch to project root folder and support calls from s…
ltfschoen Mar 8, 2023
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
7 changes: 6 additions & 1 deletion docker/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Substrate Builder Docker Image

The Docker image in this folder is a `builder` image. It is self contained and allow users to build the binaries themselves.
The Docker image in this folder is a `builder` image. It is self contained and allows users to build the binaries themselves.
There is no requirement on having Rust or any other toolchain installed but a working Docker environment.

Unlike the `parity/polkadot` image which contains a single binary (`polkadot`!) used by default, the image in this folder builds and contains several binaries and you need to provide the name of the binary to be called.
Expand All @@ -12,6 +12,11 @@ You should refer to the .Dockerfile for the actual list. At the time of editing,
- node-template
- chain-spec-builder

To generate the latest parity/substrate image. Please first run:
```sh
./build.sh
```

The image can be used by passing the selected binary followed by the appropriate tags for this binary.

Your best guess to get started is to pass the `--help flag`. Here are a few examples:
Expand Down
8 changes: 5 additions & 3 deletions docker/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ set -e

pushd .

# The following line ensure we run from the project root
PROJECT_ROOT=`git rev-parse --show-toplevel`
# The following lines ensure we run from the project root
DOCKER_DIR=$(realpath "$(dirname "${BASH_SOURCE[0]}")")
PROJECT_ROOT=$(dirname "$DOCKER_DIR")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why just first line isn't enough?

Copy link
Contributor Author

@ltfschoen ltfschoen Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

the goal i believed was to allow people to run their choice of either ./docker/build.sh or ./build.sh, which is what the current functionality of this PR provides. but in hindsight i hadn't considered allowing people to call that script from a symlink, but i've discovered a solution for that.

after reading this post https://stackoverflow.com/a/71527828/3208553, i found that even if i was calling the script with a symlink i was still able to obtain the original script folder but only after changing the first line from DOCKER_DIR=$(realpath "$(dirname "${BASH_SOURCE[0]}")") to DOCKER_DIR=$(dirname "$(realpath "${BASH_SOURCE[0]}")").

so after fixing the first line to support symlinks, i've combined it with the second line to switch to the project root folder, and we can change into it into a single command:

cd $(dirname "$(dirname "$(realpath "${BASH_SOURCE[0]}")")")

i've pushed a commit with that change

below is just a log of how i discovered the lack of support for symlinks and before i realised how to support the symlinks (you may not benefit from reading it):

the first line $(realpath "$(dirname "${BASH_SOURCE[0]}")") gets the absolute path of where the build.sh script is being run from, which may vary.

assuming the project root is in /root/substrate, if you echo that value it outputs the same result of /root/substrate/docker, regardless of whether you run it from the project root with ./docker/build.sh, or run it from within project root's docker subfolder with ./build.sh.

if we wanted to try to only use that first line, and only allow users to run the script with cd docker && ./build.sh then we could remove the lines PROJECT_ROOT=$(dirname "$DOCKER_DIR") and cd $PROJECT_ROOT, get the VERSION from the parent directory with ../bin/node/cli/Cargo.toml instead of ./bin/node/cli/Cargo.toml and change the explicit file path of the dockerfile to be the current directory where they are running the script from by changing it from -f ./docker/substrate_builder.Dockerfile to -f ./substrate_builder.Dockerfile, but when you run ./build.sh it'll crash when building the Docker container with error:

...
 => ERROR [builder 4/4] RUN cargo build --locked --release                                                
------
 > [builder 4/4] RUN cargo build --locked --release:
#0 0.486 error: could not find `Cargo.toml` in `/substrate` or any parent directory

because in the dockerfile substrate_builder.Dockerfile, we're copying all the files from the current directory . into the container directory /substrate with COPY . /substrate, but since we're running the script from within the docker/ subdirectory (because we didn't run the second line and change into the project root folder) it copies across the contents of that docker/ subdirectory rather than the contents of the project root folder into the container's /substrate folder, so when it tries to build it can't find the Cargo.toml file. also, it's not possible to copy the parent directory like this: COPY ../ /substrate, as it just copies across the current directory.

so that's a couple of reasons why i believe the first line isn't adequate, since i think the script should establish the project root folder and switch to it before running its subsequent commands.

the second line PROJECT_ROOT=$(dirname "$DOCKER_DIR") uses the first line to gets the absolute path to the project root folder and then the script switches to that path with cd $PROJECT_ROOT before running subsequent commands.

but at the moment it doesn't support calling the build.sh script from a symlink, since that'd only work if the symlink calling the build.sh was in a subdirectory one level deep inside the project root folder.

reasoning as follows this docker/build.sh doesn't work if you use symlinks to run the build.sh script (unless its in a subdirectory one level deep inside the project root folder).
if you echo that first line it doesn't consistently output that value of /root/substrate/docker if the build.sh script is called using a symlink.
for example, if you create symlinks like:

mkdir -p test1 && ln -s /root/substrate/docker/build.sh ./test1/build.sh &&
mkdir -p test2 && ln -s /root/substrate/test1/build.sh ./test2/build.sh

then if you ran ./test1/build.sh from the project root or ./build.sh from in the test1/ folder then echoing the first line would output /root/substrate/test1 instead of /root/substrate/docker.
whereas if you ran ./test2/build.sh from the project root or ./build.sh from in the test2/ folder echoing the first line would output /root/substrate/test2 instead of /root/substrate/docker.
but since in this case the symlinks are in a subfolder of the project root, then if you also used the second line the way it is then it would work since the parent folder of the symlink in these cases is the same as the parent folder of the docker/ folder.
but if you ran build.sh from a symlink that wasn't in a subdirectory one level deep inside the project root folder like this:

mkdir -p ./test3/test3 && ln -s /root/code/github/ltfschoen/substrate/test1/build.sh ./test3/test3/build.sh && ./test3/test3/build.sh

then it wouldn't work because the second line would just get the parent folder /root/substrate/test3, which isn't the project root.
so i thought we'd have to prevent users from calling it from symlinks by adding [[ ! -L ${BASH_SOURCE[0]} ]] || { echo 1>&2 "Error: Calling this script from a symlink is not supported!"; exit $ERRCODE; }

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation!


cd $PROJECT_ROOT

# Find the current version from Cargo.toml
Expand All @@ -14,7 +16,7 @@ GITREPO=substrate

# Build the image
echo "Building ${GITUSER}/${GITREPO}:latest docker image, hang on!"
time docker build -f ./docker/substrate_builder.Dockerfile -t ${GITUSER}/${GITREPO}:latest .
time DOCKER_BUILDKIT=1 docker build -f ./docker/substrate_builder.Dockerfile -t ${GITUSER}/${GITREPO}:latest .
docker tag ${GITUSER}/${GITREPO}:latest ${GITUSER}/${GITREPO}:v${VERSION}

# Show the list of available images for this repo
Expand Down
4 changes: 2 additions & 2 deletions docker/substrate_builder.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ RUN useradd -m -u 1000 -U -s /bin/sh -d /substrate substrate && \
mkdir -p /data /substrate/.local/share/substrate && \
chown -R substrate:substrate /data && \
ln -s /data /substrate/.local/share/substrate && \
# unclutter and minimize the attack surface
rm -rf /usr/bin /usr/sbin && \
# Sanity checks
ldd /usr/local/bin/substrate && \
# unclutter and minimize the attack surface
rm -rf /usr/bin /usr/sbin && \
/usr/local/bin/substrate --version

USER substrate
Expand Down
11 changes: 11 additions & 0 deletions docker/substrate_builder.Dockerfile.dockerignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
doc
**target*
.idea/
.git/
.github/
Dockerfile
.dockerignore
.local
.env*
HEADER-GPL3
LICENSE-GPL3