-
Notifications
You must be signed in to change notification settings - Fork 939
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
Add Dockerfiles and instructions for using docker containers #60
Conversation
Continuing @Manouchehri 's PR with an additional development dockerfile. #3 |
Thank you for continuing the work on Docker support! I will review the changes and will let you know. |
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 have reviewed the PR. Apart from a few suggestions, the only issue that has to be resolved is with the use of the container to perform decompilations. After that, we will gladly accept the PR!
Dockerfile
Outdated
ENV HOME /home/retdec | ||
|
||
RUN apt-get -y update && \ | ||
DEBIAN_FRONTEND=noninteractive apt-get install -y build-essential git bc graphviz upx cmake python zlib1g-dev flex bison libtinfo-dev autoconf pkg-config m4 libtool wget |
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 suggest using the same dependencies as are given in the current README file:
build-essential cmake git perl python3 bash coreutils wget bc doxygen graphviz upx flex bison zlib1g-dev libtinfo-dev autoconf automake pkg-config m4 libtool
For example, RetDec requires Python 3, but the python
package provides only Python 2. Also, having only a single package list for Debian-based distributions will simplify maintenance.
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.
Agreed, done.
Dockerfile.dev
Outdated
ENV HOME /home/retdec | ||
|
||
RUN apt-get -y update && \ | ||
DEBIAN_FRONTEND=noninteractive apt-get install -y build-essential git bc graphviz upx cmake python zlib1g-dev flex bison libtinfo-dev autoconf pkg-config m4 libtool wget |
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.
The same comment as for Dockerfile:8
.
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.
Done.
@@ -0,0 +1,23 @@ | |||
FROM ubuntu:bionic |
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.
Could you please unify the use of tabs vs spaces? Dockerfile
uses tabs for indentation, but Dockerfile.dev
uses spaces. I suggest converting spaces in Dockerfile.dev
to tabs so both dockerfiles use the same indentation.
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.
Done.
Dockerfile.dev
Outdated
RUN chown -R retdec:retdec retdec | ||
|
||
USER retdec | ||
|
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.
Nitpick: I would remove this empty line as there is no empty line between USER
and RUN
in Dockerfile
.
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.
Done.
Dockerfile.dev
Outdated
DEBIAN_FRONTEND=noninteractive apt-get install -y build-essential git bc graphviz upx cmake python zlib1g-dev flex bison libtinfo-dev autoconf pkg-config m4 libtool wget | ||
|
||
COPY . retdec | ||
RUN chown -R retdec:retdec retdec |
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 would maybe suggest replacing
COPY . retdec
RUN chown -R retdec:retdec retdec
with
COPY --chown=retdec:retdec . retdec
as the standalone recursive chown is very slow (there is a lot of files). On my system, this speeds up the image creation by about 3 minutes. The only problem may be that the --chown
parameter of COPY
is available only since v17.09.0-ce
.
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 was going to use the --chown
flag, but my docker version didn't support it. I wasn't able to find out what docker version comes with Ubuntu. If you're comfortable with the docker version dependency I'll change this to the --chown
flag.
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.
The 17.x Docker release is pretty new, and ships only with the upcoming Ubuntu 18.04 (available in April next year). I guess that if we used the --chown
flag, a lot of users would complain that they are unable to build the image. Therefore, at the end, I think we should keep the current version that uses a standalone chown
. In this way, the Dockerfile can be used by a wider range of users.
Nevertheless, maybe we should add the COPY --chown
version at least in a comment with a note? Or is there a way of writing a conditional based on the Docker version?
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.
As far as I understand, you can't conditionally run different docker commands (RUN, COPY etc).
I'll add the --chown command as a comment, perhaps in the future it will be more reasonable to use that version.
Dockerfile.dev
Outdated
USER retdec | ||
|
||
RUN cd retdec && \ | ||
ls -alh && \ |
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.
What is the purpose of this ls
? Is it just for debugging?
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.
Yes that was for debugging. Sorry, forgot to remove that. Removed.
README.md
Outdated
Run the decompiler: | ||
|
||
``` | ||
docker run retdec decompile.sh /destination/path/of/binary |
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 process does not work:
$ docker create --name retdec retdec
464c16d576d6acda87caed2e1143460a958dcc57571be51b0328be7c39f3a6fd
$ docker cp test.exe retdec:/home/retdec/test.exe
$ docker run retdec decompile.sh /home/retdec/test.exe
Error: The input file '/home/retdec/test.exe' does not exist or is not readable
I believe that the problem is that docker cp
copies the file into the retdec
image but docker run
runs the command in the retdec
container, which does not have the file. Only after I committed the image via docker commit
, I was able to decompile the file in the container that the commit produced.
What should be the correct process of using the container?
Full disclosure: I am a Docker beginner, so feel free to correct me when I did something wrong. However, I followed the instructions that are provided in the README.
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 must have omitted the docker commit
step in the readme.
Fixed and tested.
I think you have terminology backwards; docker cp
copies into the created container, but docker run
runs images. docker commit
turns the container into an image.
I am also a Docker beginner, using little projects like this to become more comfortable.
8c4497c
to
f72cd56
Compare
Dockerfile
Outdated
|
||
RUN apt-get -y update && \ | ||
DEBIAN_FRONTEND=noninteractive apt-get install -y \ | ||
build-essential \ |
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 looks 'normal' with 4 wide tabs :\
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.
Here, I would either keep the original form (all packages on a single line), or use tabs only for the initial indentation from the beginning of lines (and then use spaces after package names), or use spaces throughout the entire file (I don't know what is the generally preferred style when indenting Dockerfiles).
Sorry for submitting that PR with obvious errors. I'm still getting used to the github workflow. |
No problem ;-). |
* Development dockerfile builds from the current source directory * Add instructions to build and use docker container
f72cd56
to
6c44324
Compare
Responded to indentation comments. :) |
Thank you, merged! |
Dockerfile
creates an image from the master branch of this repository.Dockerfile.dev
creates an image from the local repository tree.Added instructions to README.md with how to use these containers in a simple use case.