Skip to content

chore: development setup in Docker - #1478

Open
MasseGuillaume wants to merge 1 commit into
onivim:masterfrom
MasseGuillaume:docker-setup
Open

chore: development setup in Docker#1478
MasseGuillaume wants to merge 1 commit into
onivim:masterfrom
MasseGuillaume:docker-setup

Conversation

@MasseGuillaume

Copy link
Copy Markdown

No description provided.

@CLAassistant

CLAassistant commented Mar 18, 2020

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@glennsl

glennsl commented Mar 18, 2020

Copy link
Copy Markdown
Member

Thanks for the PR! I feel it's lacking a bit of rationale though. We already have a dockerfile that sets up CentOS for CI, as well as a CI box running Ubuntu that's set up here:

oni2/azure-pipelines.yml

Lines 55 to 99 in c21d25e

- job: Linux
displayName: 'Linux - Ubuntu 18.04'
timeoutInMinutes: 60
pool:
vmImage: 'ubuntu-18.04'
variables:
STAGING_DIRECTORY: $(Build.StagingDirectory)
ESY__CACHE_INSTALL_PATH: /home/vsts/.esy/3_____________________________________________________________________/i
ESY__CACHE_SOURCE_TARBALL_PATH: /home/vsts/.esy/source/i
# ESY__NPM_ROOT: /opt/hostedtoolcache/node/8.14.0/x64/lib/node_modules/esy
steps:
- template: .ci/use-node.yml
- script: sudo apt-get update
- script: sudo apt-get install -y libncurses5-dev libacl1-dev libxrandr-dev libxinerama-dev libxcursor-dev libxi-dev libgl1-mesa-dev libglu1-mesa-dev mesa-utils mesa-utils-extra ragel libgtk-3-dev nasm
- template: .ci/js-build-steps.yml
- template: .ci/esy-build-steps.yml
parameters:
platform: linux
- script: _release/Onivim2.AppDir/usr/bin/Oni2 -f --no-log-colors --checkhealth
displayName: 'Release: --checkhealth'
- template: .ci/publish-build-cache.yml
- job: CentOS
displayName: 'Linux - CentOS - Docker Image'
timeoutInMinutes: 0
pool:
vmImage: 'Ubuntu 16.04'
variables:
STAGING_DIRECTORY: $(Build.StagingDirectory)
ESY__CACHE_INSTALL_PATH: /home/vsts/.esy/3_____________________________________________________________________/i
ESY__CACHE_SOURCE_TARBALL_PATH: /home/vsts/.esy/source/i
# ESY__NPM_ROOT: /opt/hostedtoolcache/node/8.14.0/x64/lib/node_modules/esy
steps:
- script: docker build -t centos scripts/docker/centos
displayName: 'docker build'
- script: docker run --rm --mount src=`pwd`,target=/oni2,type=bind centos /bin/bash -c 'which ragel'
- script: docker run --rm --mount src=`pwd`,target=/oni2,type=bind centos /bin/bash -c 'cd oni2 && ls -a'
- script: docker run --cap-add SYS_ADMIN --device /dev/fuse --security-opt apparmor:unconfined --rm --mount src=`pwd`,target=/oni2,type=bind centos /bin/bash -c 'cd oni2 && ./scripts/docker-build.sh'
- script: _release/Onivim2.AppDir/usr/bin/Oni2 -f --checkhealth
displayName: 'Release: --checkhealth'
- template: .ci/publish-linux.yml

Not sure what this docker setup would add, other than an increased maintenance burden since if we accept this we ought to commit to keeping it up-to-date as well. I would really appreciate an explanation of what benefit this brings that would outweigh the cost.

@MasseGuillaume

Copy link
Copy Markdown
Author

Yup I’m indeed I saw the docker image after the CI ran this PR. I can update the documentation to point to that Dockerfile instead.

The motivation: it’s an easy way to setup the dev environment. It isolates your system.

@glennsl

glennsl commented Mar 18, 2020

Copy link
Copy Markdown
Member

It's ultimately up to @bryphe to decide, but yeah I think offering the CentOS Dockerfile as an alternative way of setting up a dev environment would be nice, I'd prefer a few sentences explaining why you might want that though, which of the other steps it replaces, and underline that it's an alternative, not a requirement for anything.

@glennsl glennsl changed the title Feature: Development setup in Docker chore: development setup in Docker May 5, 2020

@goetzc goetzc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks!
Left some commends.

Comment thread Dockerfile.dev
Comment on lines +3 to +23
RUN apt-get update
RUN apt-get install -y yarn
RUN apt-get install -y git
RUN apt-get install -y libacl1-dev
RUN apt-get install -y libncurses-dev

#esy install
RUN apt-get install -y curl

#esy bootstrap
RUN apt-get install -y clang
RUN apt-get install -y make
RUN apt-get install -y m4
RUN apt-get install -y nasm
RUN apt-get install -y libfontconfig1-dev
RUN apt-get install -y libx11-dev
RUN apt-get install -y libsdl2-dev
RUN apt-get install -y libbz2-dev
RUN apt-get install -y libgtk-3-dev

RUN apt-get install -y opam

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All apt operations should be put in a single RUN, see: https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#run

Something like:

RUN apt-get update -q && apt-get install -y --no-install-recommends \
    yarn \
    git \
    …
    …
    …
 && rm -rf /var/lib/apt/lists/*

At the end the apt cache is no longer required and deleted, for a slimmer container image.

Same with the rest of the similar RUN instructions.

Comment thread Dockerfile.dev
RUN esy bootstrap
RUN esy build

CMD esy run No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On the CMD instruction, it is preferred to use arguments JSON notation.
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#cmd
https://github.com/hadolint/hadolint/wiki/DL3025

CMD ["esy", "run"]

docker build --network=host -t oni2-dev -f Dockerfile.dev .
# Creates a 12GB image !!!

docker run -it \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👏🏼

Comment thread Dockerfile.dev
@@ -0,0 +1,35 @@
FROM node:13.10.1-slim

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why use not use a LTS version? 14.15.4 is the latest LTS.
https://nodejs.org/en/download/releases/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants