Skip to content

Updated make file for docker building to perform extra sanity tests #533

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

trzecieu
Copy link
Collaborator

@trzecieu trzecieu commented Jun 24, 2020

Reapply #526

This change:

  • Adds extra sanity tests for Makefile, so that it will be easier to maintain the image
  • Switches to make commands for CircleCI, and adds test step
  • Adds small changes to RADME.md to better reflect how to build image locally

To be merged after: #530

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm % testing in CI without a CIRCLE_TAG

command: make -C ./docker version=${CIRCLE_TAG} build
- run:
name: test
command: make -C ./docker version=${CIRCLE_TAG} test
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no CIRCLE_TAG set when for build-docker-image I think. Its designed to run on all commits just not just releases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed just found that on the portal, added some fix for that in following commit.

Copy link
Collaborator Author

@trzecieu trzecieu Jun 25, 2020

Choose a reason for hiding this comment

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

Looks like it works just fine. So basically now it will always build against the latest tagged version.

@trzecieu trzecieu force-pushed the trzeci/testing_docker_image branch 6 times, most recently from 20a604f to d144cc8 Compare June 25, 2020 01:18
@trzecieu
Copy link
Collaborator Author

@sbc100 after some fighting with CircleCi I was able to reproduce an original reason why I've added entrypoint:

# compilation as non-root
docker run --rm -u `id -u`:`id -g` -v `pwd`:/src -w /src emscripten/emsdk:1.39.18 \
		bash -c "\
			mkdir -p .test \
			&& echo 'int main() { return 0; }' > .test/main.c \
			&& emcc -c .test/main.c -o .test/main\
		"
Traceback (most recent call last):
  File "/emsdk/upstream/emscripten/emcc.py", line 42, in <module>
    import emscripten
  File "/emsdk/upstream/emscripten/emscripten.py", line 24, in <module>
    from tools import building
  File "/emsdk/upstream/emscripten/tools/building.py", line 23, in <module>
    from . import shared
  File "/emsdk/upstream/emscripten/tools/shared.py", line 1801, in <module>
    check_vanilla()
  File "/emsdk/upstream/emscripten/tools/shared.py", line 698, in check_vanilla
    is_vanilla_file = temp_cache.get('is_vanilla.txt', get_vanilla_file)
  File "/emsdk/upstream/emscripten/tools/cache.py", line 108, in get
    self.acquire_cache_lock()
  File "/emsdk/upstream/emscripten/tools/cache.py", line 55, in acquire_cache_lock
    self.filelock.acquire(60)
  File "/emsdk/upstream/emscripten/tools/filelock.py", line 240, in acquire
    self._acquire()
  File "/emsdk/upstream/emscripten/tools/filelock.py", line 360, in _acquire
    fd = os.open(self._lock_file, open_mode)
PermissionError: [Errno 13] Permission denied: '/.emscripten_cache.lock'
make: *** [test] Error 1

This is result of non-standard user used in Circle Ci (that is not 1000:1000).

Should I revert entrypoint and add support for that?

@sbc100
Copy link
Collaborator

sbc100 commented Jun 25, 2020

I think the fix is that the emscripten root needs to be writable due to the code in emscripten that checks if the emscripten root is writable.

I think adding this line should fix it:

&&  chmod 777 ${EMSDK}/upstream/emscripten \ 

We should probably update the emscripten code so that only the cache directory (and not the containing directory need to be writable).

@trzecieu trzecieu force-pushed the trzeci/testing_docker_image branch from d144cc8 to 4774741 Compare July 4, 2020 22:21
# compilation as non-root
docker run --rm -u `id -u`:`id -g` -e HOME=/tmp -v `pwd`:/src -w /src emscripten/emsdk:${version} \
bash -c "\
mkdir -p .test \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the . prefix here? Why not just test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not a particular reason, just convention - I can change that.

docker run --rm -u `id -u`:`id -g` -e HOME=/tmp --net=host emscripten/emsdk:${version} embuilder build zlib

# compilation without entrypoint
docker run --rm -e /bin/bash -v `pwd`:/src -w /src emscripten/emsdk:${version} \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to not include -u here?

@kleisauke kleisauke mentioned this pull request Jul 25, 2020
vargaz pushed a commit to vargaz/emsdk that referenced this pull request Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants