-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix: Fixes #2464 Replace git use for finding project root. Dedicated dockerignore with .git/ for 277MB image #13472
fix: Fixes #2464 Replace git use for finding project root. Dedicated dockerignore with .git/ for 277MB image #13472
Conversation
…re with .git folder for 277MB image size
docker/build.sh
Outdated
DOCKER_DIR=$(realpath "$(dirname "${BASH_SOURCE[0]}")") | ||
PROJECT_ROOT=$(dirname "$DOCKER_DIR") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation!
Thanks @ltfschoen! |
followed these instructions https://stackoverflow.com/a/57774684/3208553 to create a custom ./docker/substrate_builder.Dockerfile.dockerignore that is dedicated to ./docker/substrate_builder.Dockerfile with fallback to ./.dockerignore
checked current disk usage:
added contents of .dockerignore to ./docker/substrate_builder.Dockerfile.dockerignore,
then added extra folders like .git/ to it too
used a different approach to obtaining the
PROJECT_ROOT
that didn't involve usinggit
command based on this stackoverflow suggestion https://stackoverflow.com/a/57774684/3208553built the image
Fixes #228
orRelated #1337
.@bkchr @ggwpez @nukemandan
Note: In addition to this PR that uses Ubuntu, I also tried using Alpine Linux instead of Ubuntu, but i could only reduce it to 236MB instead of 277MB, and i could only get it to work if i disabled the
ldd ...
command because there are some issues with the Alpine libraries that are beyond me. so that couldn't be considered until that's resolved. i've included steps i took here in the PR with the changes i made so far incase that is of interest ltfschoen#2