Skip to content
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 a simple dockerfile #29

Merged
merged 1 commit into from
Jan 8, 2023
Merged

add a simple dockerfile #29

merged 1 commit into from
Jan 8, 2023

Conversation

amiremohamadi
Copy link

can be simply used like this:

$ ls
input.txt

$ docker run -it -v $PWD:/app x8 -u https://4rt.one/level1 -w /app/input.txt

or

$ cat input.txt | docker run -i x8 -u https://4rt.one/level1 -w

Copy link
Owner

@Sh1Yo Sh1Yo left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the pull request. I also thought that adding a Dockerfile would be a good idea but didn't have time to create it. There's one suggestion tho, I think it's better to grab the latest release from github(https://github.com/sh1yo/x8/releases/latest/download/x86_64-linux-x8.gz) instead of compiling it every time.

@amiremohamadi
Copy link
Author

thanks for your response. actually the whole point of having a dockerfile is to make it possible to run everywhere. by forcing it to download x86_64-linux version, it won't work on for example a raspberry pi or sth like that.
I think the better approach would be pushing the docker image to https://hub.docker.com.

@Sh1Yo
Copy link
Owner

Sh1Yo commented Jan 7, 2023

Thanks for the response. That makes sense, at least when there are not a lot of precompiled binaries available. Still, your Dockerfile needs a fix - the Cargo.lock file isn't within the repository so it will throw an error after a clone. Also, I think it's better to clean temp files with cargo clean after the building. Building files take a lot of storage (over 0.5GB), and I don't push updates often to rebuild it anyway.

@amiremohamadi
Copy link
Author

oop! you're correct, we don't have Cargo.lock within the repo. though, rust documents encourage to commit Cargo.lock into repository [link] (should we?).

about calling cargo clean, we have a multi-stage Dockerfile that only copies x8 binary in the second stage. so we don't need to call it (because in the second stage everything is cleared and we don't have any temp files). the result will be a tiny and lightweight image.

@Sh1Yo
Copy link
Owner

Sh1Yo commented Jan 8, 2023

It looks like I should add it to prevent possible conflicts 🤔 It will ruin github stats a bit though..

Didn't notice the second docker container. Then it looks good.

Again, thanks for the pr. I'll add Cargo.lock and merge it today.

@Sh1Yo Sh1Yo merged commit c5c26da into Sh1Yo:main Jan 8, 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