Build assets in Docker container + use same container setup in CI #1312

benjaoming · 2022-08-17T14:45:47Z

…es in preparation for python3

:not(kbd) > kbd, ^ Property "not" must be followed by a ':' in /project/src/sass/_theme_rst.sass (line 518, column 3) at /project/node_modules/webpack/lib/NormalModule.js:316:20 at /project/node_modules/loader-runner/lib/LoaderRunner.js:367:11 at /project/node_modules/loader-runner/lib/LoaderRunner.js:233:18 at context.callback (/project/node_modules/loader-runner/lib/LoaderRunner.js:111:13) at Object.callback (/project/node_modules/sass-loader/dist/index.js:89:7) at Object.done [as callback] (/project/node_modules/neo-async/async.js:8069:18) at options.error (/project/node_modules/node-sass/lib/index.js:294:32) @ multi ./src/theme.js ./src/sass/theme.sass theme[1] Child mini-css-extract-plugin node_modules/css-loader/dist/cjs.js!node_modules/sass-loader/dist/cjs.js??ref--5-2!src/sass/badge_only.sass: Entrypoint mini-css-extract-plugin = * [0] ./node_modules/css-loader/dist/cjs.js!./node_modules/sass-loader/dist/cjs.js??ref--5-2!./src/sass/badge_only.sass 4.69 KiB {0} [built] + 7 hidden modules Child mini-css-extract-plugin node_modules/css-loader/dist/cjs.js!node_modules/sass-loader/dist/cjs.js??ref--5-2!src/sass/theme.sass: Entrypoint mini-css-extract-plugin = * [0] ./node_modules/css-loader/dist/cjs.js!./node_modules/sass-loader/dist/cjs.js??ref--5-2!./src/sass/theme.sass 227 bytes {0} [built] [failed] [1 error] ERROR in ./src/sass/theme.sass (./node_modules/css-loader/dist/cjs.js!./node_modules/sass-loader/dist/cjs.js??ref--5-2!./src/sass/theme.sass) Module build failed (from ./node_modules/sass-loader/dist/cjs.js): :not(kbd) > kbd, ^ Property "not" must be followed by a ':' in /project/src/sass/_theme_rst.sass (line 518, column 3)

…t running the build

… we’re setting up an environment that is isolated from the CI (or primary) container, then using the remote host’s Docker Engine."

agjohnson

I hesitate a bit about changing the development and packaging process this much. Is there a strong reason to switch CI and/or local builds to use Docker?

Are you hitting specific issues with local development otherwise? Perhaps it helps to raise those issues first, before changing things too much.

Generally speaking, giving contributors multiple conflicting paths for development is a bit confusing, so worth being careful here.

agjohnson · 2022-08-17T17:10:56Z

docker-entrypoint.sh

+
+cd /project
+
+npm run build


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.

agjohnson · 2022-08-17T17:13:08Z

docs/contributing.rst

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


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

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.

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.

agjohnson · 2022-08-17T17:16:30Z

docs/contributing.rst

+
+    $ 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


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?

agjohnson · 2022-08-17T17:35:58Z

Dockerfile

@@ -0,0 +1,29 @@
+FROM node:14-alpine
+FROM python:2-alpine


This should be Python 3, we don't want to support Python 2 anymore

Please see #1311

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.

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

The theme has been building on Python 3 for some time. This PR has passing builds for all Pythons: #1316

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

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)

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

agjohnson · 2022-08-17T17:36:45Z

.circleci/config.yml

+      - 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" .


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

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 👍

agjohnson · 2022-08-17T17:52:42Z

Also, I should note that I raised issues on this PR, but I think there are simpler alternatives to this and it is probably more helpful to discuss some build/development issues before going too much further.

Build with Node 14 installed through orbs. Fixes readthedocs#1275

This introduced syntax errors that were not picked up in review, the `:not()` psuedo selector can't lead a selector in SASS.

agjohnson · 2022-08-17T19:10:13Z

I addressed issues with CI building in #1314, issues with a merge error in #1315, and dependency issues with #1316

benjaoming · 2022-08-17T22:42:07Z

For the record: This approach worked fine and builds were ✔️. I'd like to contest that Dockerized build environments are the way forward 😉

agjohnson · 2022-08-18T00:07:39Z

From chat, I think this PR should still continue as an additional piece to local development. This seemed close, but using Python3 and Node 14 would be issues to resolve.

I would still hold off on altering local development workflow or our CI builds though. Switching to Docker has side effects that require a bit more consideration, and we'd rather avoid disparate development workflows for now.

benjaoming added 8 commits August 16, 2022 17:30

WIP: dockerize the build process

e58f867

Reset package-lock.json after 1b4263e

651007e

Fix invalid version "1.0.1alpha1" (npm breaks) - bump some dependenci…

aaf494b

…es in preparation for python3

First step to dockerize build process, unfortunately with Python 2

48dab6e

Buildable docker image with npm install

9025a96

Revert some updates, but okay about node-sass 4.14.1

b654b40

Add Makefile with shortcuts + updated theme file from the first try a…

ea440e8

…t running the build

benjaoming force-pushed the dockerize branch from 22ab0f9 to ea440e8 Compare August 17, 2022 16:50

benjaoming added 2 commits August 17, 2022 19:00

Also copy package-lock.json

4577c89

Update package-lock.json (after running npm audit fix)

6269d47

benjaoming requested a review from agjohnson August 17, 2022 17:01

benjaoming changed the title ~~WIP: Build assets in Docker container~~ Build assets in Docker container Aug 17, 2022

benjaoming marked this pull request as ready for review August 17, 2022 17:01

benjaoming requested a review from a team as a code owner August 17, 2022 17:01

benjaoming force-pushed the dockerize branch from 3527651 to 994730f Compare August 17, 2022 17:13

Try building and running docker in Circle (not sure if works)

43c1ab3

benjaoming force-pushed the dockerize branch from 994730f to 43c1ab3 Compare August 17, 2022 17:16

benjaoming added 3 commits August 17, 2022 19:22

Try to build docker with docker

deb984f

Try "This step helps you avoid the Docker-in-Docker problem. In fact,…

7545937

… we’re setting up an environment that is isolated from the CI (or primary) container, then using the remote host’s Docker Engine."

No make in docker-ce image

9476b62

benjaoming force-pushed the dockerize branch from 5193930 to 9476b62 Compare August 17, 2022 17:27

Can we bind mount?

a6c25df

agjohnson reviewed Aug 17, 2022

View reviewed changes

agjohnson added 3 commits August 17, 2022 11:27

Fix CircleCI config

34e29e6

Build with Node 14 installed through orbs. Fixes readthedocs#1275

Fix SASS syntax errors from readthedocs#967

2dbaef8

This introduced syntax errors that were not picked up in review, the `:not()` psuedo selector can't lead a selector in SASS.

Pin Jinja to pre-3.1 release

62e9976

benjaoming mentioned this pull request Aug 17, 2022

Update package lock #1317

Closed

benjaoming force-pushed the dockerize branch 2 times, most recently from d21b190 to a9aff93 Compare August 17, 2022 21:07

Use CircleCI machine platform

0ac878a

benjaoming force-pushed the dockerize branch from a9aff93 to 0ac878a Compare August 17, 2022 21:14

Clean up from earlier experiments

3eda02d

benjaoming force-pushed the dockerize branch from 222b517 to 3eda02d Compare August 17, 2022 21:20

benjaoming changed the title ~~Build assets in Docker container~~ Build assets in Docker container + use same container setup in CI Aug 17, 2022

Merge remote-tracking branch 'upstream/agj/fix-jinja' into dockerize

9d2c22f

benjaoming force-pushed the dockerize branch from 17d4695 to 9d2c22f Compare August 17, 2022 21:58

benjaoming closed this Aug 17, 2022

benjaoming mentioned this pull request Aug 19, 2022

Supplemental development process for Docker #1319

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build assets in Docker container + use same container setup in CI #1312

Build assets in Docker container + use same container setup in CI #1312

benjaoming commented Aug 17, 2022

agjohnson left a comment

agjohnson Aug 17, 2022 •

edited

agjohnson Aug 17, 2022

benjaoming Aug 17, 2022

agjohnson Aug 17, 2022

agjohnson Aug 17, 2022

agjohnson Aug 17, 2022

benjaoming Aug 17, 2022

agjohnson Aug 17, 2022

benjaoming Aug 17, 2022

agjohnson Aug 17, 2022

benjaoming Aug 17, 2022

agjohnson Aug 17, 2022

agjohnson Aug 17, 2022 •

edited

agjohnson Aug 17, 2022

benjaoming Aug 17, 2022

agjohnson commented Aug 17, 2022

agjohnson commented Aug 17, 2022

benjaoming commented Aug 17, 2022

agjohnson commented Aug 18, 2022


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

Build assets in Docker container + use same container setup in CI #1312

Build assets in Docker container + use same container setup in CI #1312

Conversation

benjaoming commented Aug 17, 2022

agjohnson left a comment

Choose a reason for hiding this comment

agjohnson Aug 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agjohnson Aug 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agjohnson commented Aug 17, 2022

agjohnson commented Aug 17, 2022

benjaoming commented Aug 17, 2022

agjohnson commented Aug 18, 2022

agjohnson Aug 17, 2022 •

edited

agjohnson Aug 17, 2022 •

edited