-
Notifications
You must be signed in to change notification settings - Fork 78
fix: run docker/podman container once #1225
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
base: main
Are you sure you want to change the base?
fix: run docker/podman container once #1225
Conversation
Instead of running multiple containers, it is enough to use just one. Signed-off-by: Evgeny Semenov <[email protected]>
db529af to
77c8385
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think several of these commands are already elided because they are gated by a file existence check (for example, bundle install only runs if $ROOT/.home/.gems is missing).
Is that also your understanding, or is there something I'm missing here?
Also,
| OLDDIR=$PWD | ||
| cd $ROOT | ||
| ${RUN} bundle install | ||
| run bundle install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this (and other) setup commands already run just once since they are conditioned on the existence of file ($ROOT/.home/.gems) in this case. Are you seeing them run multiple times anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't refuse existence checks. If we shouldn't do something based on checks then this command won't be accumulated to CONTAINER_SETUP_CMD and won't be executed. It does full build inside only one container instead of multiple needless creation and removing of containers.
| fi | ||
|
|
||
| source ${ROOT}/.home/.venv/bin/activate | ||
| run source ${ROOT}/.home/.venv/bin/activate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this (I think) sets environment variables in the host, it might need to run every time anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if we build docs inside container so we need to activate Python venv there. Activation on the host doesn't share environment variables implicitly into container.
riscv-unified-db$ export ROOT=$PWD
riscv-unified-db$ source ${ROOT}/.home/.venv/bin/activate
(.venv) riscv-unified-db$ env | grep VIRTUAL_ENV
VIRTUAL_ENV=/home/q/workspace/sync_upstream/riscv-unified-db/.home/.venv
VIRTUAL_ENV_PROMPT=(.venv)
(.venv) riscv-unified-db$ docker run --rm -v ${ROOT}:${ROOT} -w ${ROOT} riscvintl/udb:0.9 bash -c "env"
HOSTNAME=2fe0d945961c
PWD=/home/q/workspace/sync_upstream/riscv-unified-db
container=podman
HOME=/root
TERM=xterm
SHLVL=0
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
DEBIAN_FRONTEND=noninteractive
_=/usr/bin/env
# WRONG: We don't have venv variables inside container.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I use singularity, where environment variables from the host appear in the container by default (and do not persist in the container). In Docker, will the environment variable persist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is --env option which allows to set variable inside. But it's also possible to define through shell call to use it one time.
Instead of running multiple containers, it is enough to use just one.