Create a docker build script #1138
Conversation
erubboli
requested review from
muursh,
iljakuklic,
TheQuantumPhysicist and
azarovh
as code owners
.dockerignore
Outdated
| @@ -0,0 +1,9 @@ | |||
| **/target/ | |||
There was a problem hiding this comment.
This pattern will exclude any directory named 'target' anywhere in the source tree. IMO we should only exclude the 'target' directory at the root, which is the one that cargo uses.
So, this line can be just:
target
.dockerignore
Outdated
| **/*.rs.bk | ||
| **/Dockerfile | ||
| **/.dockerignore | ||
| **/.git | ||
| **/.gitignore | ||
| **/tmp | ||
| **/Cargo.lock | ||
| .docker/ |
There was a problem hiding this comment.
IMO these lines are not needed (probably except for the top-level .git directory). The purpose of .dockerignore is to exclude the "heavy" or sensitive stuff that we wouldn't want to accidentally copy into the image. IMO there is no harm in copying extra small files into "builder" image.
Makefile
Outdated
| .PHONY: build_docker_images | ||
|
|
||
| # python scripts are requiring `python3.11-venv` and `python3-pip` installed | ||
| install-pip-dependencies: | ||
| python3 -m venv env && . env/bin/activate && python3 -m pip install --upgrade pip && python3 -m pip install toml | ||
|
|
||
| build_docker_images: install-pip-dependencies | ||
| . env/bin/activate && python3 build-tools/docker/build.py | ||
|
|
||
| push_docker_images: install-pip-dependencies | ||
| . env/bin/activate && python3 build-tools/docker/build.py --build=false --push --latest |
There was a problem hiding this comment.
IMO makefiles are worse than bash scripts. So, if we really need a non-python script on top of 'build.py', I'd say let's go with bash.
But I'm not sure we need one. I'd say that things like installing the appropriate python packages and managing virtual environments should be done manually by the developer, because different developers might have different preferences in this regard (personally, I wouldn't want to have the "env" directory inside the source tree). And if that part of the script is gone, it'll only contain a couple of calls to build.py, which probably doesn't require a wrapper script.
There was a problem hiding this comment.
I'll put the dependencies in the readme
build-tools/docker/BUILD.md
Outdated
|
|
||
| This document outlines the steps to build Docker images for the Mintlayer node daemon, wallet-cli, and node-gui. | ||
|
|
||
| Before building make sure you clone the repository and change directory in the root of the repository. |
There was a problem hiding this comment.
"change directory in" -> "change the current directory to"?
build-tools/docker/README.md
Outdated
|
|
||
| The `-v` option is used to mount a local directory (in this case `~/.mintlayer`) as a volume in the Docker container. | ||
| The `--user` option is used to specify the user that will write the `~/.mintlayer` directory. | ||
| NOTE: this wont work on windows hosts. |
build-tools/docker/README.md
Outdated
| ``` | ||
|
|
||
| The `-v` option is used to mount a local directory (in this case `~/.mintlayer`) as a volume in the Docker container. | ||
| The `--user` option is used to specify the user that will write the `~/.mintlayer` directory. |
There was a problem hiding this comment.
"will write the" -> "will write to the"?
build-tools/docker/build.py
Outdated
| return version | ||
|
|
||
| def build_docker_image(dockerfile_path, image_name, version): |
There was a problem hiding this comment.
A nitpick: according to PEP8, top-level function definitions should be separated by 2 blank lines.
build-tools/docker/build.py
Outdated
| process = subprocess.Popen(command, shell=True) | ||
| output, error = process.communicate() | ||
|
|
||
| if error: | ||
| print(f"Error occurred: {error}") | ||
| else: | ||
| print(f"Built {image_name}:{version} successfully.") |
There was a problem hiding this comment.
Shouldn't we stop the script if docker build fails? (i.e. why not just use check_call here like in other places?)
|
|
||
| COPY . . | ||
|
|
||
| ARG NUM_JOBS=1 |
There was a problem hiding this comment.
At this moment it's impossible to specify this value when using build.py. I'd be nice if the latter had an additional parameter for this.
This script build and pushes the docker images, it retrieve the version from the Cargo file and can be invoked by make as
build_docker_images.