Skip to content
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

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e58f867
WIP: dockerize the build process
benjaoming Aug 16, 2022
651007e
Reset package-lock.json after 1b4263eadfa5dc13473cfa8797c579a78b3c175e
benjaoming Aug 17, 2022
aaf494b
Fix invalid version "1.0.1alpha1" (npm breaks) - bump some dependenci…
benjaoming Aug 17, 2022
48dab6e
First step to dockerize build process, unfortunately with Python 2
benjaoming Aug 17, 2022
9025a96
Buildable docker image with npm install
benjaoming Aug 17, 2022
5d8fbba
Fix syntax issue
benjaoming Aug 17, 2022
b654b40
Revert some updates, but okay about node-sass 4.14.1
benjaoming Aug 17, 2022
ea440e8
Add Makefile with shortcuts + updated theme file from the first try a…
benjaoming Aug 17, 2022
4577c89
Also copy package-lock.json
benjaoming Aug 17, 2022
6269d47
Update package-lock.json (after running npm audit fix)
benjaoming Aug 17, 2022
43c1ab3
Try building and running docker in Circle (not sure if works)
benjaoming Aug 17, 2022
deb984f
Try to build docker with docker
benjaoming Aug 17, 2022
7545937
Try "This step helps you avoid the Docker-in-Docker problem. In fact,…
benjaoming Aug 17, 2022
9476b62
No make in docker-ce image
benjaoming Aug 17, 2022
a6c25df
Can we bind mount?
benjaoming Aug 17, 2022
34e29e6
Fix CircleCI config
agjohnson Aug 17, 2022
2dbaef8
Fix SASS syntax errors from #967
agjohnson Aug 17, 2022
62e9976
Pin Jinja to pre-3.1 release
agjohnson Aug 17, 2022
0ac878a
Use CircleCI machine platform
benjaoming Aug 17, 2022
3eda02d
Clean up from earlier experiments
benjaoming Aug 17, 2022
9d2c22f
Merge remote-tracking branch 'upstream/agj/fix-jinja' into dockerize
benjaoming Aug 17, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@ commands:
description: "Ensure built assets are up to date"
steps:
- checkout
- run: npm ci
- run: npm run build
- setup_remote_docker
# - run: npm ci
- run: docker build -t sphinx_rtd_theme:latest .
- run: docker run --cidfile=.container_id --mount type=bind,source="${CIRCLE_WORKING_DIRECTORY}/src",target=/project/src,readonly sphinx_rtd_theme:latest
- run: docker cp "$(cat .container_id):/project/sphinx_rtd_theme" .
- run: docker cp "$(cat .container_id):/project/package-lock.json" .
Copy link
Collaborator

Choose a reason for hiding this comment

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

Noted already, but the node orb should be used here instead of Docker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now replaced with a CircleCI env that builds our Docker image like we specify and it doesn't take a lot of build time 👍

- run:
name: Ensure built assets are up to date
command: |
Expand All @@ -32,7 +36,7 @@ commands:
jobs:
build:
docker:
- image: 'cimg/python:3.9-node'
- image: docker:17.05.0-ce-git
steps:
- run-build: {}
py27:
Expand Down
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,5 @@ sphinx_rtd_theme/static/fonts/RobotoSlab/
sphinx_rtd_theme/static/js/html5shiv.min.js
sphinx_rtd_theme/static/js/html5shiv-printshiv.min.js
.nvmrc

.container_id
29 changes: 29 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
FROM node:14-alpine
FROM python:2-alpine
Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see #1311

Copy link
Collaborator

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.

Copy link
Contributor Author

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 of node-gyp that doesn't support Python 3. I finally hit the same issue in bourbon-neat and gave up, since Wyrm didn't allow an upgrade. See #1311

Copy link
Collaborator

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

Copy link
Contributor Author

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

Copy link
Collaborator

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)

Copy link
Collaborator

@agjohnson agjohnson Aug 17, 2022

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


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

# There is a very stubborn npm package that keeps complaining even though
# we try to set its environment config vars
# RUN ln -s `which python` /usr/bin/python3

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"]
13 changes: 13 additions & 0 deletions Makefile
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" .
6 changes: 6 additions & 0 deletions docker-entrypoint.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#!/bin/sh

cd /project

npm run build
Copy link
Collaborator

@agjohnson agjohnson Aug 17, 2022

Choose a reason for hiding this comment

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

You should be running npm run dev for development, not npm run build. build is for building the minified JS/CSS files, where dev runs Webpack development server and takes care of hot reload of the assets.


24 changes: 24 additions & 0 deletions docs/contributing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
===================

Expand Down Expand Up @@ -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``.
Copy link
Collaborator

Choose a reason for hiding this comment

The 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
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``.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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
=======

Expand Down