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

Refactor Dockerfile #861

Closed
wants to merge 2 commits into from
Closed

Conversation

JuryTionasho
Copy link

  1. Use alpine image for build to have musl by default as needed for final image
  2. Use fixed version for both image and packages, that are known to work until a new version is needed (rust installer is the exception)
  3. Use digest when using a docker image since tags can be overwritten
  4. Install rust through official installer to take care of adding necessary architecture depending on the system used for building trippy (x86_64, arm, etc)
  5. Add whole contents of the repository in one go instead of file by file
  6. Run cargo build --release without architecture specified for anyone not running x86_64 to have a working trippy

1. Use alpine image for build to have musl by default as needed for final image
2. Use fixed version for both image and packages, that are known to work until a new version is needed (rust installer is the exception)
3. Use digest when using a docker image since tags can be overwritten
4. Install rust through official installer to take care of adding necessary architecture depending on the system used for building trippy (x86_64, arm, etc)
5. Add whole contents of the repository in one go instead of file by file
6. Run cargo build --release without architecture specified for anyone not running x86_64 to have a working trippy
@fujiapple852
Copy link
Owner

Thank you for the PR @JuryTionasho !

I agree with the points about using fixed image versions (though digest is overkill IMHO, fully reproducible builds isn't the goal here) and with making the Dockerfile platform independent.

I don't think using alpine for both Docker layers just to get musl by default is the right trade off. It also, as you note, means the Rust installation is not locked to a version and that is more significant (Trippy has a MSRV of 1.70 today).

I don't think it makes sense to copy in all files from the repo to the image, this includes things like assets/ which contains large animated gifs. Arguably, if anything we should remove the README.md file that is currently copied in (i'd leave the LICENCE file however).

Note also that the Dockerfile has been setup specifically to avoid having to download and build the dependancies every time the Trippy code changes (lines https://github.com/fujiapple852/trippy/blob/master/Dockerfile#L4-L8) and I'd want to see that preserved.

Finally, please be aware of #277 (PR #397) which forced the switch to alpine instead of gcr.io/distroless/cc, ideally we'd be able to go back to using a distroless distro instead of alpine to reduce the size of the image.

@JuryTionasho
Copy link
Author

After a bit more digging, I have found a way to install a specific version of rust, tho this should be on their installation page as well.

I don't think using alpine for both Docker layers just to get musl by default is the right trade off

I'm not sure what you mean. Why would you want to use a debian image(rust:1.74) which has gnu by default
GNU
and then add musl L2 and build for it L8? Why not just build for gnu at that point?

Using alpine for the build and then installing rust adds it as musl
MUSL
When compiling, it uses musl target as well.
LDD

This is besides the smaller image on the alpine counterpart, tho I don't know how much that matters since it's only the first build stage.
SMALLER

Also, let me remind you that you are making a multi stage build and since L15 you are not getting any of the files you copied earlier because you are only copying the binary from the build-env stage.

Your intentions were not clear to me, thus me adding the whole contents in one go to not add files one by one.
If you want to keep the license in the final image you should copy it in the final stage, but there is no reason to not add the whole contents after the first build since you are not copying them to the final image anyway.

ideally we'd be able to go back to using a distroless distro instead of alpine to reduce the size of the image

Let me do you one better.
IMAGE
And it does work with crossterm 0.23.2 and tui 0.18.0
WORKING

@JuryTionasho
Copy link
Author

If what you care is size, you should also look into rust size optimizations since I was able to make quite a much smaller trippy image.
trippy

@fujiapple852
Copy link
Owner

fujiapple852 commented Dec 14, 2023

Hi again @JuryTionasho,

A bit of background and context; the Docker image was previously a gnu build and used gcr.io/distroless/cc for the image used at runtime, just copying in the executable.

The issue I linked to was that ncurses needed to be available in the runtime environment, as tput was an (undocumented) Crossterm dependency. I therefore moved to Alpine as the next best (i.e. smallest) base image so I could install ncurses, and therefore also had to switch to a musl build.

Now, interestingly, when I try your Dockerfile today, which does not include ncurses, I am not able to reproduce the bug (usually occurs if you run the container with run -it a few times in rapid succession). However I can still see the offending code in Crossterm so i'm unsure why this is no longer an issue. There does seem to be some other changes and refactoring there so perhaps it isn't used in the same way any more.

If the bug is fixed then I would prefer to revert to a gnu build and use the gcr.io/distroless/cc base image. I don't object to a musl build on Alpine either. if we're not sure that the bug is really fixed then it maybe safer to stick to the Alpine image and include ncurses.

WDYT?

Also, let me remind you that you are making a multi stage build and since L15 you are not getting any of the files you copied earlier because you are only copying the binary from the build-env stage.

Yes indeed, thanks for pointing that out and fixing it (doh!).

curl=8.5.0-r0 \
build-base=0.5-r3 \
--repository=https://dl-cdn.alpinelinux.org/alpine/v3.19/main
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this needed?

@fujiapple852
Copy link
Owner

If what you care is size, you should also look into rust size optimizations since I was able to make quite a much smaller trippy image.

Thanks, i'm familiar with these. I'd like to make the trip binary as small as it can be, but I don't feel the techniques presented there are the right tradeoffs for Trippy (except perhaps LTO). For example, I would not want to strip the binary, in fact I'd meant to add debug symbols to the release build.

@fujiapple852
Copy link
Owner

Based on the latest update in crossterm-rs/crossterm#741 I think we can revert to using gcr.io/distroless/cc and so I will close this PR.

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