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

Conversation

ltfschoen
Copy link
Contributor

  • You added a brief description of the PR, e.g.:
  • 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:

$ du -hc --max-depth=1 .[^.]* * | grep [MG]
217M	.git/objects
218M	.git
16K	        .github/ISSUE_TEMPLATE
15G	        bin/node-template
4.6M	bin/node
5.4M	bin
7.6M	client/executor
1.2M	client/network
14M	        client
1.2M	frame/contracts
3.3M	frame/support
13M	        frame
4.0K	        HEADER-GPL3
36K	        LICENSE-GPL3
3.9M	primitives
4.0K	        README.md
2.8M	zombienet/0001-basic-warp-sync
2.8M	zombienet
  • 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 using git command based on this stackoverflow suggestion https://stackoverflow.com/a/57774684/3208553

  • built the image

cd docker
./build.sh
  • inspect the size of the image
$ docker images

parity/substrate   latest    31c200b06ab8   15 minutes ago   277MB
parity/substrate   v3.0.0    31c200b06ab8   15 minutes ago   277MB

$ docker run --rm -it parity/substrate substrate --version
substrate 3.0.0-dev-unknown

$ docker run --rm -it parity/substrate subkey --version
subkey 2.0.2

$ docker run --rm -it parity/substrate node-template --version
node-template 4.0.0-dev-unknown

$ docker run --rm -it parity/substrate chain-spec-builder --help
<it worked and output help stuff>

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

docker/build.sh Outdated
Comment on lines 7 to 8
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

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!

@dmitry-markin dmitry-markin added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 8, 2023
@dmitry-markin dmitry-markin merged commit 7c3d279 into paritytech:master Mar 8, 2023
@dmitry-markin
Copy link
Contributor

Thanks @ltfschoen!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants