Docker improvements #571

kleisauke · 2020-07-25T18:44:29Z

This PR is built on top of #533 with these additional changes:

Shallow clone emsdk (saves ~2MB).
Test tip-of-tree build within CI.
Run the test suite outside the Docker image.
Avoid copying the Dockerfile into the final image (it already exists in /emsdk/docker).

Testing the tip-of-tree build within CI hasn't been discussed, hence I've marked this PR as a draft.

/cc @trzecieu, as he's the author of #533.

sbc100

Thanks! Mostly looks great

sbc100 · 2020-07-25T20:41:33Z

docker/README.md


    # Install required tools that are useful for your project i.e. ninja-build
    RUN apt update && apt install -y ninja-build

    ```
 2. build it
-    ```shell
+    ```bash


Does this make a difference anywhere?

It doesn't make any difference, bash is an alias for shell within GitHub flavored markdown. I think @trzecieu changed this to align it with the other code blocks.

sbc100 · 2020-07-25T20:42:50Z

docker/test_dockerimage.sh

+emcc -c test/main.c -o test/main
+
+# test embuilder
+embuilder build zlib


Should we test this doesn't actually build anything? (since the re-loaded cache should contain these libraries).

I added the --force parameter with commit d702167 to ensure it's not built from the cache.

sbc100 · 2020-07-25T20:43:40Z

docker/test_dockerimage.sh

 # cleanup after test
+rm -rf test
 find ${EMSDK} -name "*.pyc" -exec rm {} \;


Why cleanup here at all.. isn't this run with --rm meaning its just gets thrown away anyway?

The --rm parameter will not remove files within the mounted volume. I removed these cleanups with commit d702167 and compiled the test to /tmp instead.

docker/Dockerfile

sbc100 · 2020-07-25T20:46:14Z

docker/Dockerfile

@@ -19,11 +19,12 @@ RUN echo "## Start building" \
    && echo "## Done"

 RUN echo "## Get EMSDK" \
-    && git clone https://github.com/emscripten-core/emsdk.git ${EMSDK} \
+    && git clone --depth=1 https://github.com/emscripten-core/emsdk.git ${EMSDK} \


This whole step seems a little odd given that this Dockerfile now lives in the emsdk repo itself. I've been working on a PR to remove this step and just have the Dockerfile operate of the current checkout that contains it. I'm still up for landing this change first as-is though.

Great! Copying the current checkout to the Dockerfile is indeed a better way to handle this.

Commit 118f091 operates on the current checkout directory within the Dockerfile. This ensures that any changes in emsdk itself or in the Docker test script (i.e. docker/test_dockerimage.sh) are tested during pull requests.

sbc100 · 2020-07-26T15:18:38Z

.circleci/config.yml

@@ -88,16 +88,16 @@ jobs:
            python scripts/test.py

  build-docker-image:
-    executor: bionic
+    machine: true # required for testing files permissions


This is kind of sad.. I it probably slower to start up and perhaps more costly.

Can you explain a little more why the regular executor doesn't work?

I'm not sure why the regular executor didn't work. This was probably necessary to run privileged Docker containers.

I reverted the executor back to bionic with commit f5f2fbf.

Argh, it looks like mounting volumes doesn't work for the CircleCI Docker executor, that's why CI failed for f5f2fbf. See:
https://discuss.circleci.com/t/docker-compose-yml-and-volume-mounts/10043/7

Fixed with 75ca515.

docker/Dockerfile

sbc100 · 2020-07-26T15:22:32Z

docker/test_dockerimage.sh

+fi
+
+# test embuilder
+embuilder build zlib --force


Sorry I under standstand now, the cleanup was being done not inside the docker image but here in the docker subdirectory.

In that I case I think the old way was better. No need to use /tmp just write the files here.

Since I cannot mount volumes using the CircleCI Docker executor, writing the compiled output to the docker subdirectory is not possible. Would you rather use the CircleCI Docker executor (current state) or write to the compiled output to the docker subdirectory (and therefore using the machine executor)?

Sorry, I think its fine to use the /tmp directory inside the container assuming that is what is happening? Since that directory will be destroyed once the container finished running right?

You're right. The /tmp is only used inside the container and then cleaned up afterwards once the container is finished (thanks to the --rm parameter).

sbc100 · 2020-07-28T04:21:08Z

docker/Makefile

+	# FIXME: the `-e HOME=/tmp` is a workaround for:
+	# https://github.com/emscripten-core/emsdk/issues/535
+	docker run --rm -u `id -u`:`id -g` -e HOME=/tmp --net=host emscripten/emsdk:${version} \
+		/bin/bash -c "`cat test_dockerimage.sh`"


This construction seems a little bit hacky... why not just leave test_dockerimage.sh inside the container?

Sorry, I was overthinking the issue. We could indeed just run /emsdk/docker/test_dockerimage.sh instead.

Commit 118f091 removes this hackish solution.

sbc100 · 2020-07-28T20:22:53Z

.dockerignore

+.flake8
+
+# Makefile is not needed
+Makefile


Is there some say to inherit everything from .gitignore? if not perhaps just include it with a comment.

Perhaps we could symlink it with .gitignore? Anyways, I cleaned up the file a bit with commit 4b64400.

sbc100 · 2020-07-28T20:27:07Z

docker/Dockerfile

-    && git clone https://github.com/emscripten-core/emsdk.git ${EMSDK} \
-    && echo "## Done"
+# Copy the contents of this repository to the container
+COPY . ${EMSDK}


I my version of this change I used ADD . /emsdk: https://github.com/emscripten-core/emsdk/compare/docker_use_local_emsdk?expand=1#diff-ebacf6f6ae4ee68078bb16454b23247dR26

I guess they are the same .. i can't remember why I chose one over the other.

It doesn't make much difference: https://stackoverflow.com/a/24958548/10952119. I think COPY is fine here.

sbc100 · 2020-07-28T20:29:49Z

Makefile

@@ -0,0 +1,30 @@
+#!/usr/bin/env make


Perhaps comment at the top saying what the purpose of this makefile is? i.e. to build the docker image.

If you could leave the Makefile where it was and rung it with make -f to avoid polluting the top level?

(You can also remove the #! line from the top, I don't think anyone expects to run the Makefile like that).

All done with commit 531f6c8.

sbc100 · 2020-07-31T17:13:04Z

Is this ready t do now? Its still marked as a draft.

kleisauke · 2020-07-31T18:28:53Z

Yes, this is now ready for final review. I've removed the draft status.

Since this PR contains some outdated details, would you like me to squash all the commits and update the commit message?

sbc100 · 2020-07-31T20:45:15Z

Yes please. We always squash all out PRs on commit anyway by policy.

IMHO, if something it worth its own commit its probably also work its own PR which means I basically never look at individual commits in PRs.

This also means that each commit in git also goes through CI on its own which makes bisecting easier.

sbc100 · 2020-07-31T20:45:39Z

I think you might also want rebase/integrate #578

sbc100 · 2020-07-31T20:46:56Z

I was going to suggest that we split out the "use emsdk inplace" change since it a fairly major one, and commit the other misc improvements separately. But I don't feel strongly about it.

docker/Dockerfile

- Copy the current checkout to the Dockerfile. - Add .dockerignore to exclude files while copying. - Test tip-of-tree build within CI. - Run the test suite outside the Docker image. - Perform extra sanity tests. - Switch to make commands for CircleCI. - Improve Docker README. Co-authored-by: trzeci <[email protected]>

kleisauke · 2020-08-03T09:16:03Z

docker/test_dockerimage.sh

 set -ex

-sudo -u nobody `which emcc` --version
+if [ $EUID -eq 0 ]; then


Note that this check is needed for Podman (which is the default container engine on Fedora 32). Podman provides a Docker-compatible command line front end that can simply alias the Docker cli (alias docker=podman).

Without this check, it will fail to perform the tests (probably because Podman is running in rootless mode).

Details

$ make -C ./docker version=tot test make: Entering directory '/home/kleisauke/emsdk/docker' # test as non-root docker run --rm -u `id -u`:`id -g` -w /emsdk/docker --net=host emscripten/emsdk:tot \ bash test_dockerimage.sh ++ which emcc + sudo -u nobody /emsdk/upstream/emscripten/emcc --version sudo: unable to resolve host pc-kaw: Name or service not known We trust you have received the usual lecture from the local System Administrator. It usually boils down to these three things: #1) Respect the privacy of others. #2) Think before you type. #3) With great power comes great responsibility. sudo: no tty present and no askpass program specified make: *** [Makefile:20: test] Error 1 make: Leaving directory '/home/kleisauke/emsdk/docker'

kleisauke · 2020-08-03T09:24:06Z

Rebased and squashed as requested. I've also polished the commit message and added @trzecieu as co-author (thanks!).

If you want, I could make a separate PR for the "emsdk inplace" change. Otherwise, I think this PR is ready to land.

kleisauke · 2020-08-03T09:34:16Z

@trzecieu I've used trzeci <[email protected]> as co-author, as this seems your primary commit email and username. Let me know if something needs to be changed.

This check became obsolete after WebAssembly/binaryen#3042.

sbc100 · 2020-08-22T09:04:22Z

I this ready to land now?

kleisauke · 2020-08-22T09:18:44Z

This PR is ready to land (I was waiting for confirmation regarding the added Co-authored-by: ... line, but I think it's fine).

The CI failures in this PR are probably because it hasn't been rebased. Commit 1274e7a should fix the CI failure for the publish-docker-image job (as a result 2.0.1 has not been published on Docker Hub).

emscripten-core#571) * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20231023.4 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.23519.2 -> To Version 9.0.0-alpha.1.23523.4 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20231023.4 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.23519.2 -> To Version 9.0.0-alpha.1.23523.4 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20231025.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.23519.2 -> To Version 9.0.0-alpha.1.23525.1 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20231025.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.23519.2 -> To Version 9.0.0-alpha.1.23525.1 --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

kleisauke mentioned this pull request Jul 25, 2020

Fix sanity file location within Dockerfile #568

Merged

sbc100 reviewed Jul 25, 2020

View reviewed changes

sbc100 approved these changes Jul 26, 2020

View reviewed changes

sbc100 approved these changes Jul 28, 2020

View reviewed changes

sbc100 reviewed Jul 28, 2020

View reviewed changes

kleisauke marked this pull request as ready for review July 31, 2020 18:28

trzecieu approved these changes Jul 31, 2020

View reviewed changes

docker/Dockerfile Show resolved Hide resolved

docker/Dockerfile Show resolved Hide resolved

kleisauke force-pushed the docker-improvements branch from 4b64400 to da0432a Compare August 1, 2020 13:11

kleisauke commented Aug 3, 2020

View reviewed changes

Remove check for asm2wasm

1274e7a

This check became obsolete after WebAssembly/binaryen#3042.

sbc100 approved these changes Aug 22, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into docker-improvements

bbcc24c

kleisauke mentioned this pull request Sep 1, 2020

2.0.2 #604

Merged

sbc100 merged commit 645d276 into emscripten-core:master Sep 2, 2020

Docker improvements #571

Docker improvements #571

Uh oh!

Conversation

kleisauke commented Jul 25, 2020

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kleisauke Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Jul 31, 2020

Uh oh!

kleisauke commented Jul 31, 2020

Uh oh!

sbc100 commented Jul 31, 2020

Uh oh!

sbc100 commented Jul 31, 2020

Uh oh!

sbc100 commented Jul 31, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kleisauke commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kleisauke commented Aug 3, 2020

kleisauke Jul 26, 2020 •

edited

Loading

kleisauke commented Aug 3, 2020 •

edited

Loading