-
Notifications
You must be signed in to change notification settings - Fork 159
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
Rework devcontainer to use proxygen server #1107
Conversation
.deploy/init.sh
Outdated
echo "** Installing apt dependencies" | ||
# This is done by the dockerfile, but the intermediate issue can be cached, so do | ||
# it again here. | ||
apt-get clean | ||
apt-get update -y | ||
|
||
apt-get install -y ruby php-cli zip unzip locales | ||
LC_ALL=C apt-get install -y ruby php-cli zip unzip locales |
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.
vscode executes stuff a bit differently, including implicitly copying locale around. Installing the locales
package will fail if the locale is set to en_US.UTF-8
before we have ran locale-gen en_US.UTF-8
... which we can't do earlier as it's provided by the locales package.
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.
Would you mind including the comment into the shall script, please?
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.
Shall we move apt-get install
into the Dockerfile
, to allow docker caching across Codespaces?
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 script is used by a few different dockerfiles at the moment, though it's probalby worth splitting the stuff that doesn't require a copy of the source into a separate script which can then be called from the dockerfile and cache
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.
... which won't work as that script isn't available until the source is mounted.
This means that `hhvm` is the actual docker command; the bad news is that if hhvm crashes, the container goes down. I'm doing this anyway as there doesn't appear to be support in devcontainer.json for a separate long-running command - only via docker compose, which: - feels like overkill - appears not to support devcontainer variables like containerWorkspaceFolder; this means that it can't work with GitHub codespaces, as the `workspaceMount` devcontainer.json option is not supported on codespaces The lack of support for `workspaceMount` is also why there's the fun messing around with symlinking /var/www and configuration files.
9e44809
to
b5ca417
Compare
microsoft/vscode-remote-release#4093 and microsoft/vscode-remote-release#3034 are the primary design constraints leading to this solution :( |
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.
Overall looks good with some minor questions. See comments.
.deploy/init.sh
Outdated
echo "** Installing apt dependencies" | ||
# This is done by the dockerfile, but the intermediate issue can be cached, so do | ||
# it again here. | ||
apt-get clean | ||
apt-get update -y | ||
|
||
apt-get install -y ruby php-cli zip unzip locales | ||
LC_ALL=C apt-get install -y ruby php-cli zip unzip locales |
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.
Would you mind including the comment into the shall script, please?
.devcontainer/devcontainer.json
Outdated
"postCreateCommand": "touch /docker_build; .deploy/init.sh", | ||
"postStartCommand": "cd public; hhvm -m server -p 8080 -vServer.AllowRunAsRoot=1 -c ../hhvm.dev.ini", | ||
"forwardPorts": [ 8080 ] | ||
"extensions": [ "pranayagarwal.vscode-hack" ], |
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.
Weird spacing
.devcontainer/Dockerfile
Outdated
|
||
RUN rm -rf /var/www; ln -s ${WORKSPACE} /var/www | ||
RUN ln -sf /var/www/hhvm.dev.ini /etc/hhvm/site.ini | ||
RUN touch /docker_build |
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.
Is there a standard way to detect if it's in a container? For example: https://stackoverflow.com/questions/20010199/how-to-determine-if-a-process-runs-inside-lxc-docker
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.
Looks like it could use /.dockerenv nowadays
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.
Going to make it a dotfile, but stick with touching this. The methods in that stackoverflow post are valid when running, but not when building.
.deploy/init.sh
Outdated
echo "** Installing apt dependencies" | ||
# This is done by the dockerfile, but the intermediate issue can be cached, so do | ||
# it again here. | ||
apt-get clean | ||
apt-get update -y | ||
|
||
apt-get install -y ruby php-cli zip unzip locales | ||
LC_ALL=C apt-get install -y ruby php-cli zip unzip locales |
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.
Shall we move apt-get install
into the Dockerfile
, to allow docker caching across Codespaces?
By the way, shall we add a step in the Github Actions to test the devcontainer? For example: https://github.com/stuartleeks/devcontainer-build-run |
Either fixed or responded, except for:
Given that this is re-using our existing testing container scripts where possible, I don't think it adds much, especially as it's a third-party 'pet project' - and it limits experimentation with a better compose-based. Definitely worth adding if GitHub introduce a more complete official one in the future. |
if ! [ -e /.docker_build ]; then | ||
echo "This script should only be ran from a Dockerfile" | ||
exit 1 | ||
fi |
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.
.dockerenv
does not exist on buildkit but the cgroup approach should work.
This means that
hhvm
is the actual docker command; the bad news is that ifhhvm crashes, the container goes down.
I'm doing this anyway as there doesn't appear to be support in
devcontainer.json for a separate long-running command - only via docker
compose, which:
containerWorkspaceFolder; this means that it can't work with GitHub
codespaces, as the
workspaceMount
devcontainer.json option is notsupported on codespaces
The lack of support for
workspaceMount
is also why there's the funmessing around with symlinking /var/www and configuration files.