-
Notifications
You must be signed in to change notification settings - Fork 344
[cpp,go,python,full] Sync single language images with full image if possible and update versions #372
Conversation
83fd810
to
ae4e215
Compare
Thanks for this PR, @chrbayer - This looks like a nice update! |
ln -s /usr/bin/clang-tidy-$LLVM /usr/bin/clang-tidy && \ | ||
rm -rf /var/lib/apt/lists/* |
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.
Why remove this "cleanup" line?
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 removed the line at this position because I thought it would be a good idea to do the cleanup only once after the last install with apt:
# Theia application
##Needed for node-gyp, nsfw build
RUN apt-get update && apt-get install -y python build-essential && \
apt-get clean && \
apt-get autoremove -y && \
rm -rf /var/cache/apt/* && \
rm -rf /var/lib/apt/lists/* && \
rm -rf /tmp/*
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 way of doing it once at the end is good for the final tidiness of the image's file system, but IIUC has a drawback: unless unwanted files are removed from the image in the same docker command (i.e. same image layer) they were created, they will still be in the image but hidden.
So ideally such cleanups would happen for every apt-get
command. However I can't say that we've been very careful so far to do this everywhere in our images, so it's not necessarily a deal breaker for this PR.
WDYT?
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 is a good point which I have not thought about. It will have only a minimum effect on build times, so I'm going to copy the cleanup-block to the end of each apt-get RUN command.
Changes made and pushed, the size of the final image stays the same.
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.
Changes made and pushed, the size of the final image stays the same.
That part is a bit unexpected. I wonder why? Any theories? I wonder if you were to remove all these cleanups related to installing Debian packages, including the final one, would the image size increase?
e988dc2
to
d06349f
Compare
theia-full-docker/Dockerfile
Outdated
ENV LC_ALL=C.UTF-8 | ||
ENV LANG=C.UTF-8 | ||
|
||
ENTRYPOINT [ "yarn", "theia", "start", "/home/project", "--hostname=0.0.0.0" ] | ||
ENTRYPOINT [ "node", "/home/theia/src-gen/backend/main.js", "/home/project", "--hostname=0.0.0.0" ] | ||
# ENTRYPOINT [ "yarn", "theia", "start", "/home/project", "--hostname=0.0.0.0" ] |
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.
Remove commented-out line?
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.
BTW, I am curious: did you find a functional difference between the two forms of ENTRYPOINT
above?
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'm going to delete the commented line after you review and tests are finished, maybe there will be some more changes. I didn't noticed any difference between the two lines, just took the change out of the cpp docker script.
Thanks @chrbayer - I just did another pass and found just one minor thing. I need to test a bit, but so far the changes look good! I'll let you know how it goes with the tests, |
@marcdumais-work Thank you very much for reviewing and testing. Please tell me if all tests and reviews are finished, so that I can include all findings in the PR. Should I make a sperate commit or just re-push fixed? |
It's fine to force-push your fix(es). Normally we prefer squashing the commits of PRs but in this case, looking a the commit messages, the breakdown looks logical and if you think you've maintained the changes pretty well insulated, each in their own commit, I have no problem retaining the various commits you have in the PR currently. Though if we conclude it will make no difference in image size, we could return to a single cleanup at the end for apt temp files (i.e. drop last commit) |
Signed-off-by: Christoph Bayer <chrbayer@criby.de>
Signed-off-by: Christoph Bayer <chrbayer@criby.de>
Signed-off-by: Christoph Bayer <chrbayer@criby.de>
Signed-off-by: Christoph Bayer <chrbayer@criby.de>
Signed-off-by: Christoph Bayer <chrbayer@criby.de>
Signed-off-by: Christoph Bayer <chrbayer@criby.de>
This PR introduces lots of changes, so it will be hard to test everything. I've done some sanity testing, just to confirm that the main things seem to still work. No issue found! [full]:
[cpp]
[python]
|
@chrbayer Ready for your final update. Can you let me know if anything substantial changes vs the current version, that would make it worth it to redo some of the testing? |
@marcdumais-work Thank you very much for all you support! The only changes I did are the two proposed changes:
|
For me this PR should now be complete :-) |
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.
LGTM, thanks @chrbayer for this contribution!
Where possible sync the single language images and the full images and the update versions of development tools in full and other images when in sync: