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

Add an option to develop in a reproducible and containerized environment #10794

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions Dockerfile.develop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
FROM node:10-alpine
Copy link
Member

Choose a reason for hiding this comment

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

. o ( We should probably bump this to 11 or 12 soon. Something for a different PR )

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely need to bump it to node-14 now, as we don't support anything older than that.

Choose a reason for hiding this comment

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

you can use node:lts-alpine if you want


# Support custom branches of the react-sdk and js-sdk. This also helps us build
# images of riot-web develop.
ARG USE_CUSTOM_SDKS=false
ARG REACT_SDK_REPO="https://github.com/matrix-org/matrix-react-sdk.git"
Copy link
Member

Choose a reason for hiding this comment

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

we should actually be using locally-linked react-sdk and such when doing development, as the bulk of Riot is in the react-sdk.

Copy link
Author

Choose a reason for hiding this comment

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

Working on it 👍

Copy link
Member

Choose a reason for hiding this comment

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

the variable is still here :(

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it is, otherwise the container would require people to mount files to certain location. Currently it by default pulls the repos, however allows to override this behavior. This way not only you can choose what repo/folder to use, but also allows you to have only this project as editable without even pulling the SDKs. See this change.

Copy link
Member

Choose a reason for hiding this comment

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

for a development environment where 95% of the app is in the react-sdk, I would definitely expect that people have a local copy of the react-sdk.

Copy link
Author

Choose a reason for hiding this comment

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

The current approach works for both cases and at the same time pulls the app dependencies first which means that there already are dependencies for react-sdk and riot-web already cached in the image itself. That means a user that mounts a locally cloned react-sdk does not have to wait long for the installation because the only thing that's going to install with docker run is riot-web + different versions of deps that are not cached. This is good for switching between branches since docker container by default does not save the changes in its filesystem(image) unless docker commit is used.

Copy link
Member

Choose a reason for hiding this comment

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

The installation should be pretty quick if the dependencies have already been pulled the first time. Unfortunately the environment doesn't provide much value if most of the app isn't editable.

Copy link
Author

Choose a reason for hiding this comment

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

Docker does not work that way, it does not preserve anything that happens in the container which means every docker run command requires the installation from scratch i.e. pulling each dependency. No quick install because the pull would happen every time. That's why I left the original repositories there so the docker image already contains the cached dependencies. Only image preserves the state, not container. If I remove the repositories from Dockerfile, everything will be slowed down because the whole fetching/hashing/installing/linking/preparing process would need to be executed in full on each docker run. Currently if you do docker build the image (which contains the state used by the container) will have all the deps + a default installation according to that ARGS instruction, however you don't need to care about that because as said in the linked instructions from README.md with docker run you can specify to use custom, locally available, repositories/folders which will then replace the default installation from the image and will be both used for the next installation run with docker run command (installs only the new SDKs, uses cached deps, i.e. the case you describe) and will be editable from the host machine i.e. if I change something in react-sdk and press F5 the changes will be immediately reflected in the container. If anything goes wrong a user can then use docker commit to create a new image from the container and e.g. send it to you/other devs or just share with colleagues.

ARG REACT_SDK_BRANCH="master"
ARG JS_SDK_REPO="https://github.com/matrix-org/matrix-js-sdk.git"
ARG JS_SDK_BRANCH="master"

# CRLF -> LF conversion
RUN apk add --no-cache git dos2unix

WORKDIR /src

# Install deps-only into separate Docker layer and run Riot when the container
# starts, mount source as a volume and keep dev environment consistent
COPY scripts /src/scripts
RUN dos2unix /src/scripts/docker-link-repos.sh \
&& sh /src/scripts/docker-link-repos.sh

# Cache package.json
COPY package.json yarn.lock /src/

# Remove 'prepare' script because there is NO config in yarn and npm for
Copy link
Member

Choose a reason for hiding this comment

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

meh - this is fine tbh. It shouldn't cause problems, it's just a waste of cycles.

tbh I'm more inclined to leave it there than rip it out for future maintenance things.

Copy link
Author

Choose a reason for hiding this comment

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

I've tried removing it again today and it throws this error:

Step 12/15 : RUN yarn --network-timeout=100000 install
 ---> Running in a026b8e776e8
yarn install v1.17.3
[1/4] Resolving packages...
[2/4] Fetching packages...
warning Pattern ["gemini-scrollbar@matrix-org/gemini-scrollbar#b302279"] is trying to unpack in the same destination "/usr/local/share/.cache/yarn/v4/npm-gemini-scrollbar-1.4.3/node_modules/gemini-scrollbar" as pattern ["gemini-scrollbar@github:matrix-org/gemini-scrollbar#b302279"]. This could result in non-deterministic behavior, skipping.
info fsevents@1.2.9: The platform "linux" is incompatible with this module.
info "fsevents@1.2.9" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
warning "matrix-react-sdk > emojibase-data@4.0.1" has unmet peer dependency "emojibase@*".
warning "matrix-react-sdk > slate@0.41.3" has unmet peer dependency "immutable@>=3.8.1".
warning "matrix-react-sdk > slate-html-serializer@0.6.32" has unmet peer dependency "immutable@>=3.8.1".
warning "matrix-react-sdk > slate-md-serializer@3.1.0" has unmet peer dependency "immutable@>=3.0.0".
warning "matrix-react-sdk > slate-react@0.18.11" has unmet peer dependency "immutable@>=3.8.1".
warning "matrix-react-sdk > diff-dom > rollup-plugin-terser@4.0.4" has unmet peer dependency "rollup@>=0.66.0 <2".
warning "matrix-react-sdk > slate-react > react-immutable-proptypes@2.1.0" has unmet peer dependency "immutable@>=3.6.2".
warning "matrix-react-sdk > slate-react > slate-plain-serializer@0.6.39" has unmet peer dependency "immutable@>=3.8.1".
warning "matrix-react-sdk > slate-react > slate-prop-types@0.4.67" has unmet peer dependency "immutable@>=3.8.1".
warning " > electron-builder-squirrel-windows@21.2.0" has unmet peer dependency "app-builder-lib@~21.2.0".
[4/4] Building fresh packages...
$ yarn clean && yarn build:compile
yarn run v1.17.3
$ rimraf lib webapp electron_app/dist
Done in 0.24s.
yarn run v1.17.3
$ yarn reskindex && babel --source-maps -d lib src
$ reskindex -h src/header
src doesn't exist
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
The command '/bin/sh -c yarn --network-timeout=100000 install' returned a non-zero code: 2

Copy link
Member

Choose a reason for hiding this comment

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

we should fix that error then - that file definitely exists. Is it just not making its way over to the container?

Copy link
Author

Choose a reason for hiding this comment

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

Eh, this is embarrassing... for some reason it never occurred to me it's a file ^_^" Fixed!

# disabling it and is not necessary for development build
RUN sed -i -e '/"prepare":/d' /src/package.json
RUN yarn --network-timeout=100000 install

# Overlay src files with the original ones in case something was left out
# from explicit copying above and doesn't work with mounting properly such as:
#
# "ERROR in Entry module not found: Error: Can't resolve './src' in '/src'"
# "ERROR in multi (webpack)-dev-server/client?http://0.0.0.0 ./src"
COPY . /src

# Assemble entrypoint that copies the config at container start
# and runs webpack server. Mount volume to /src/src for developing.
RUN echo 'mkdir /src/webapp' >> /entry.sh \
&& echo 'cp /src/config.sample.json /src/webapp/config.json' >> /entry.sh \
&& echo 'yarn start' >> /entry.sh \
&& chmod +x /entry.sh

# run webpack server to change the code on-the-go
ENTRYPOINT /entry.sh
37 changes: 37 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,43 @@ If any of these steps error with, `file table overflow`, you are probably on a m
which has a very low limit on max open files. Run `ulimit -Sn 1024` and try again.
You'll need to do this in each new terminal you open before building Riot.

Using Docker image for development
turt2live marked this conversation as resolved.
Show resolved Hide resolved
----------------------------------

Avoids installation of Node, NPM and the rest of the environment by providing
the same environment that's used in serving Riot via Dockerfile officially
and at the same time allows to plug in the folder with source files so that
you can edit outside of Docker container and let it compile inside where
it is safely separated from your machine or any custom configuration.

Using this approach provides a reproducible environment between you and
the maintainers if you share your changes in a pull request.

Run it with:

# build image
turt2live marked this conversation as resolved.
Show resolved Hide resolved
docker build --tag vectorim/riot-web:develop --file Dockerfile.develop .

# remove old container or do nothing
docker rm -f riot || true

# run a new container with src folder mounted from git repository
# located on the host machine. You can mount additional folders
# such as res with additional --volume $PWD/res:/src/res
docker run --detach --interactive --tty \
--publish <port-on-host>:8080 \
--volume $PWD/src:/src/src \
turt2live marked this conversation as resolved.
Show resolved Hide resolved
--name riot \
vectorim/riot-web:develop

And access ``yarn`` and other commands from inside of the container:

# run shell process from container
turt2live marked this conversation as resolved.
Show resolved Hide resolved
docker exec -it riot sh

# run yarn process from container
docker exec -it riot yarn

Running the tests
-----------------

Expand Down