-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Build assets in Docker container + use same container setup in CI #1312
Changes from all commits
e58f867
651007e
aaf494b
48dab6e
9025a96
5d8fbba
b654b40
ea440e8
4577c89
6269d47
43c1ab3
deb984f
7545937
9476b62
a6c25df
34e29e6
2dbaef8
62e9976
0ac878a
3eda02d
9d2c22f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
FROM node:14-alpine | ||
FROM python:2-alpine | ||
|
||
RUN apk add --update npm | ||
|
||
RUN mkdir -p /project/src/ | ||
|
||
WORKDIR /project | ||
|
||
COPY package.json /project/ | ||
|
||
# COPY package-lock.json /project/ | ||
|
||
COPY bin/preinstall.js /project/bin/preinstall.js | ||
|
||
RUN cd /project | ||
|
||
RUN npm install --package-lock-only &&\ | ||
npm audit fix &&\ | ||
npm install | ||
|
||
COPY webpack.common.js webpack.dev.js webpack.prod.js /project/ | ||
|
||
COPY docker-entrypoint.sh /entrypoint.sh | ||
RUN chmod +x /entrypoint.sh | ||
|
||
ENTRYPOINT ["/entrypoint.sh"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
SHELL := /bin/bash | ||
CWD := $(shell cd -P -- '$(shell dirname -- "$0")' && pwd -P) | ||
|
||
docker-images: | ||
docker build -t sphinx_rtd_theme:latest . | ||
|
||
docker-run: | ||
rm -f .container_id | ||
docker run --cidfile=.container_id --mount type=bind,source="$(CWD)/src",target=/project/src,readonly sphinx_rtd_theme:latest | ||
|
||
docker-copy-assets: | ||
docker cp "$(shell cat .container_id):/project/sphinx_rtd_theme" . | ||
docker cp "$(shell cat .container_id):/project/package-lock.json" . |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
#!/bin/sh | ||
|
||
cd /project | ||
|
||
npm run build | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should be running |
||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -6,6 +6,9 @@ This project follows the Read the Docs :doc:`code of conduct | |||
<rtd:code-of-conduct>`. If you are not familiar with our code of conduct policy, | ||||
take a minute to read the policy before starting with your first contribution. | ||||
|
||||
.. tip:: | ||||
There is a new dockerized build environment, see :ref:`dockerized-build`. | ||||
|
||||
Modifying the theme | ||||
=================== | ||||
|
||||
|
@@ -62,6 +65,27 @@ can be used to test built assets: | |||
.. _Wyrm: http://www.github.com/snide/wyrm/ | ||||
.. _Sphinx: http://www.sphinx-doc.org/en/stable/ | ||||
|
||||
|
||||
_dockerized-build:: | ||||
|
||||
Dockerized build | ||||
================ | ||||
|
||||
If you have Docker available on your platform, you can get started building CSS and JS artifacts a bit faster and won't have to worry about any of the setup spilling over into your general environment. | ||||
|
||||
When building with Docker, we create an image containing the build dependencies, such as `SASS`_ , `Wyrm` and `Webpack`_ in the anticipated versions. Some of these are quite outdated and therefore ideal to isolate a container. The image is tagged as ``sphinx_rtd_theme:latest``. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is true without Docker as well, everything is installed via NPM. Just to be clear to the contributor, I think this should be removed.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the Node version and Python versions expected for the build environment weren't previously made explicit, this comment felt very spot on :) I would always advocate having a Dockerfile in order to manifest the build environment. Not trying to make it mandatory, though, that's why it's just an additional development suggestion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a change for the contributing doc instead. The docker environment isn't going to be any different than the current build environment. |
||||
|
||||
Inside the running docker image, we mount the working copy of the repository, build the artifacts and finally copy out those artifacts into your external environment. | ||||
|
||||
Use the following steps: | ||||
|
||||
.. code-block:: console | ||||
|
||||
$ make docker-images # Builds an updated version of the docker image | ||||
$ make docker-run # Runs the docker environment and builds the assets. The container exits after completing the build. | ||||
$ make docker-copy-assets # Copies out the assets from the most recent docker-run | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This process doesn't quite feel ready for contributors, it's a bit more complicated than the normal development pattern. Is there a reason to not just mount this path as a volume? |
||||
|
||||
|
||||
Testing | ||||
======= | ||||
|
||||
|
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.
This should be Python 3, we don't want to support Python 2 anymore
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.
Please see #1311
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.
Yeah, I already commented there as well. Python 3 is supported currently, Python 2 is not required and might be documented as not support at all even.
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.
This is not true AFAICT. We are using a version of
node-sass
that uses a version ofnode-gyp
that doesn't support Python 3. I finally hit the same issue inbourbon-neat
and gave up, since Wyrm didn't allow an upgrade. See #1311There 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 theme has been building on Python 3 for some time. This PR has passing builds for all Pythons: #1316
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.
I haven't found an explanation for that TBH, but I'm sure it's good :) When I change the Dockerfile to use
FROM python:3-alpine
, I start getting errors that I never resolved, but I really did try to do it without touching our node stack. The "internet" seems to have similar issues.sass/node-sass#2877
nodejs/node-gyp#1687
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.
Odd, the errors don't necessarily surprise me, but I certainly can't offer an explanation.
node-gyp
apparently has python 3 support since 4.0.0 -- I hit this issue back in #771 -- #771 (comment)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.
We figured this out, or at least it's a little more clear why building on python3 image is not working. Seems Alpine is somehow using Node 18 and also cannot find the
/usr/bin/python3
binary (or Alpine's python package only installs a/usr/bin/python
binary