-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Refactor dockerfiles for simplicity #1786
Refactor dockerfiles for simplicity #1786
Conversation
I would really like feedback from @maybe-sybr and @Arusekk on this, since it changes a lot of things about how the Dockerfiles are used. Specifically, with this change, If it DOES break use-cases, I can easily just change the name of the images to e.g. |
On follow-up and reconsideration, this is probably not the best way to do this. A separate docker file for development (as opposed to the |
On re-follow-up, we're already creating the |
e5d47ca
to
00de790
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.
While I am no expert on containers, this looks good enough. And it looks like there should now be less 'layers' or what they are called.
4b92872
to
d8b2d11
Compare
…e future anyway This leaves base, stable, beta, and dev as the only Dockerfile images Simplify extra/Docker so that Pwntools is already checked out, and build precedence works * Base image always gets built first * Branch-tracking Dockerfiles now require base to be built * Use locally-checked-out-repo instead of e.g. git+https://github.com/Gallopsled/pwntools@beta * Install tested with both Python2 and Python3 * Add Python3 user-script directory to $PATH Update CHANGELOG with Docker changes Add develop to Makefile, restore old Dockerfiles Make development target separate, and automatically launch "docker run -it" with suggested arguments Add develop Dockerfile
d8b2d11
to
29e1fdd
Compare
This pull request now includes a working Dockerfile at The mechanism to run the tests is:
These tests all pass, but this PR needs to be merged in order for Docker Hub to automatically build a Next will be the creation of a new GHA workflow that runs the Docker image with a bind-mount so that the Pull Request being tested is the copy of Pwntools being used. Merging this tonight because:
|
CI / test 2.7 fails with only 7 failures, all related to Corefile stuff. This is fewer failures than without this PR, so this is an improvement toward getting GitHub Actions doctests working as well. We need a working CI pipeline for the upcoming release. Please merge if you agree, @Arusekk |
Important Notes and Background
This PR modifies the
Dockerfile
s inextra/docker
so that we have a non-root user,pwntools
, and the "installed" version of pwntools is checked out in/home/pwntools/pwntools
.This makes it easier for other Docker images to build on top of this, as well as paves the way for updating
travis/docker
to usepwntools/pwntools:stable
as a base image which allows local testing to work again.Notably, pwntools is no longer installed as
root
, but is installed as an editable install in$HOME/pwntools
.Ideally, we would be able to remove the lines from each of
dev
/beta
/stable
that read:However, we cannot do this because e.g.
dev
adds new dependencies onrpyc
. This is a small penalty to pay, but overall the Docker images are much more usable for development and downstream usage.Since Pwntools is no longer installed as
root
, the baseDockerfile
now sets:Which should put e.g.
pwn
into the user path, allowing us topwn version
. Since we install Python2 first, then Python3, the Python3 version prevails. This can be verified from within the Docker image:Less Important Changes
This PR removes some
Dockerfiles
that are unlikely to have ever been used, because they are broken. These are mostly left around from when the Pwntools base image would inherit from one of these images.We now pin the
base
image to Ubuntu Bionic. If / when we decide to upgrade to Focal, we only need to change thebase/Dockerfile
and fix up any errors. For now, Bionic is fine as it is LTS and still supported.How to Verify this PR
Since this PR doesn't change any source code, it's probably not easy to validate for e.g. @Arusekk. Here's how you can do it relatively easy: