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

chore: Speed up docker builds a bit by reducing layer count. #2603

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jan 26, 2024

This is especially noticeable on local builds (less so on the github workers).


This change is Reviewable

@iphydf iphydf added this to the v0.2.19 milestone Jan 26, 2024
@iphydf iphydf marked this pull request as ready for review January 26, 2024 09:18
@iphydf iphydf requested a review from a team as a code owner January 26, 2024 09:18
@iphydf iphydf force-pushed the dockerfiles branch 3 times, most recently from e0503f2 to 992c174 Compare January 26, 2024 13:56
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d6d67d5) 73.77% compared to head (8328449) 73.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2603      +/-   ##
==========================================
- Coverage   73.77%   73.68%   -0.09%     
==========================================
  Files         148      148              
  Lines       30366    30366              
==========================================
- Hits        22401    22375      -26     
- Misses       7965     7991      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This is especially noticeable on local builds (less so on the github
workers).
nurupo

This comment was marked as outdated.

@nurupo
Copy link
Member

nurupo commented Jan 26, 2024

Ok, I see where my confusion came from. I misread docker/build-push-action@v4 as docker/build-publish-action@v4. It's also that each analysis tool does that, not just one job.

So, each analysis tool job gathers the source files from the local git repo checkout and builds an image containing it, and then runs the tool-specific Docker build that uses that local image. No Dockerhub involved at all. That makes a lot more sense :)

other/docker/misra/Makefile Show resolved Hide resolved
other/docker/tokstyle/tokstyle.Dockerfile Show resolved Hide resolved
@nurupo
Copy link
Member

nurupo commented Jan 26, 2024

Somewhat curious as by how much does this speed up the local builds. It can't be more than 1-5 seconds on a modern computer with an ssd, can it? I would actually think that it makes them slower, since for every tool you run you now also run other/docker/sources/build which calls docker to build the source image, with the docker having to analyze the fs to see if any files have were modified/added/removed.

@pull-request-attention pull-request-attention bot assigned nurupo and unassigned iphydf Jan 26, 2024
@nurupo
Copy link
Member

nurupo commented Jan 26, 2024

LGFM, just want to get those conversations resolved (and possibly the speed-up question answered).

@iphydf
Copy link
Member Author

iphydf commented Jan 26, 2024

Somewhat curious as by how much does this speed up the local builds. It can't be more than 1-5 seconds on a modern computer with an ssd, can it? I would actually think that it makes them slower, since for every tool you run you now also run other/docker/sources/build which calls docker to build the source image, with the docker having to analyze the fs to see if any files have were modified/added/removed.

Not everybody has SSD :). On SSD, it almost doesn't matter, but on HDD it's quite significant.

@iphydf
Copy link
Member Author

iphydf commented Jan 26, 2024

It matters more when you run multiple of these docker builds in sequence, so you only need to build sources once.

@pull-request-attention pull-request-attention bot assigned iphydf and unassigned nurupo Jan 26, 2024
@toktok-releaser toktok-releaser merged commit 8328449 into TokTok:master Jan 26, 2024
57 checks passed
@iphydf iphydf deleted the dockerfiles branch January 26, 2024 18:51
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.

3 participants