-
Notifications
You must be signed in to change notification settings - Fork 282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: development setup in Docker #1478
base: master
Are you sure you want to change the base?
Conversation
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: Lines 55 to 99 in c21d25e
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. |
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. |
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. |
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!
Left some commends.
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 |
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.
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.
RUN esy bootstrap | ||
RUN esy build | ||
|
||
CMD esy run |
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.
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 \ |
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.
👏🏼
@@ -0,0 +1,35 @@ | |||
FROM node:13.10.1-slim |
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 use not use a LTS version? 14.15.4 is the latest LTS.
https://nodejs.org/en/download/releases/
No description provided.