Feat: Buildkit caching for Docker builds#56
Conversation
always use the main branch cache, so e.g. tag builds can take advantage of previously cached assets
lalver1
left a comment
lalver1
left a comment
There was a problem hiding this comment.
This is a nice feature of Buildkit! I didn't know about it so it was nice to read up on it while reviewing.
The code changes look good to me 👍 I also built the image locally and opened up a dev container without any problem. I only had a few minor comments that shouldn't block this PR.
| # pip cache dir must be created and owned by the user to work with BuildKit cache | ||
| mkdir -p ${PIP_CACHE_DIR} && \ | ||
| # own the parent directory of PIP_CACHE_DIR | ||
| chown -R ${USER}:${USER} /home/${USER}/.cache && \ |
There was a problem hiding this comment.
Maybe we could use Bash parameter expansion ${PIP_CACHE_DIR%/*} here instead of /home/${USER}/.cache. This line wouldn't be as clear as is it now, but on the other hand we'd guarantee that changes to PIP_CACHE_DIR would follow through (and I don't know how often this would happen 😅 ).
There was a problem hiding this comment.
I'm definitely a Bash noob! I was reading about parameter expansion and this section seems relevant to your comment:
${parameter%word}
${parameter%%word}The word is expanded to produce a pattern and matched according to the rules described below (see Pattern Matching). If the pattern matches a trailing portion of the expanded value of parameter, then the result of the expansion is the value of parameter with the shortest matching pattern (the
%case) or the longest matching pattern (the%%case) deleted...
Yikes, that is a mouthful!
I think I am understanding that your suggestion:
${PIP_CACHE_DIR%/*}
Follows the % case above, such that:
parameter:PIP_CACHE_DIR-- expands to the full/home/calitp/.cache/pipword:/*-- expands to any subdirectory- and the "shortest matching pattern" (any subdirectory in this case) is the
/pipportion
So the result is that /pip is deleted and we are left with... what I hardcoded /home/calitp/.cache 😅
Is this correct?
There was a problem hiding this comment.
Yep, that's correct! That's what I also understood about how parameter expansion works. I used these other docs (the section under ${variable%suffix}) that also have an example, and running it on a local branch confirmed the behavior too.
But yep, we are basically left with what you had hardcoded 😅 I was thinking that parameter expansion could make the script a bit safer in case PIP_CACHE_DIR were to change (since chown -R ${USER}:${USER} /home/${USER}/.cache would automatically point to the right place) but again, PIP_CACHE_DIR may never change so this isn't actually important, it's probably better to prioritize clarity and use the hardcoded string 👍
There was a problem hiding this comment.
Oh I totally agree with you! I just didn't understand your bash-fu and wanted to make sure!
There was a problem hiding this comment.
On second thought after discussion, we decided to leave it as-is to be more explicit.
| PYTHONUSERBASE="/home/${USER}/.local" \ | ||
| # where to store the pip cache (use the default) | ||
| # https://pip.pypa.io/en/stable/cli/pip/#cmdoption-cache-dir | ||
| PIP_CACHE_DIR="/home/${USER}/.cache/pip" \ |
There was a problem hiding this comment.
At first I thought PATH and PYTHONUSERBASE should just be set to /$USER/.local/bin and /$USER/.local respectively, but prefixing with home works too 👍
There was a problem hiding this comment.
They in fact need to be in the /home directory!
Although we have the WORKDIR directly under /$USER for the final image, during build time pip was having trouble using a cache when it was not inside /home.
pip expects to do --user installs, well, inside the user's home directory! All the overriding in the world wasn't working, but this did 😀
| # setup $USER permissions for nginx | ||
| mkdir -p /var/cache/nginx && \ | ||
| chown -R $USER /var/cache/nginx && \ | ||
| chown -R $USER:$USER /var/cache/nginx && \ |
There was a problem hiding this comment.
Just curious what the purpose of this change is
There was a problem hiding this comment.
This sets ownership for both the user=$USER (LHS of the :) and the group=$USER (RHS of the :). Maybe it isn't strictly necessary, but since I had to use groups in the cache mount, I figured better to be explicit.
The group happens to be the same name as the user.
angela-tran
left a comment
angela-tran
left a comment
There was a problem hiding this comment.
This looks good to me! I tested building the app image twice and opening the devcontainer
3 participants
Related: