- Remove temporary apt files
- Removes LLVM16 TODO
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rGd22cab2e11f4: [libc++][CI] Improves Dockerfile
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/utils/ci/Dockerfile | ||
---|---|---|
83–87 | I like to keep the comments for clarity. |
libcxx/utils/ci/Dockerfile | ||
---|---|---|
83–87 | I really don't see how describing what the lines do on-by-one help in any way. They just make it harder to read IMO. |
I like that we are removing the LLVM 16 todo, but I am not sold on the other changes to the Dockerfile. I'd like to understand your rationale for them.
libcxx/utils/ci/Dockerfile | ||
---|---|---|
78–79 | Isn't it better to have more layers? That way you get more layers of caching and can potentially re-download less when you update the image, no? |
As discussed, I am not sure this actually solves a problem we encounter -- our image isn't that large and I don't think the image download times have been a problem. If that's incorrect, please LMK since that changes my opinion about this patch.
But assuming that we're not really gaining something noticeable in terms of speed/size, IMO this patch is a small net loss since it's a bit less readable than what we had before. This is a part of our CI infrastructure and IMO we should strive to keep it as simple as possible to keep the barrier for people maintaining it as low as possible -- to me that has more value than a bit more size in the image.
FWIW I'd be fine with cleaning up the Debian packages in a separate RUN command. This won't reduce the size we have to download, but would make the image itself a bit smaller, I guess.
Thanks for the review. I will basically revert these changes and only remove the unneeded package and the apt removal.
libcxx/utils/ci/Dockerfile | ||
---|---|---|
83–87 | As discussed with Louis this entire hunk will be reverted. |
Could you align all the backslashes to 120 columns to make them less annoying?