-
Notifications
You must be signed in to change notification settings - Fork 2k
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
docker: Use system GIT_CACHE_DIR if available #9651
docker: Use system GIT_CACHE_DIR if available #9651
Conversation
pkg/pkg.mk
Outdated
@@ -31,7 +31,7 @@ endif | |||
$(PKG_BUILDDIR)/.git-downloaded: | |||
rm -Rf $(PKG_BUILDDIR) | |||
mkdir -p $(PKG_BUILDDIR) | |||
$(GITCACHE) clone "$(PKG_URL)" "$(PKG_VERSION)" "$(PKG_BUILDDIR)" | |||
GIT_CACHE_DIR=$(GIT_CACHE_DIR) $(GITCACHE) clone "$(PKG_URL)" "$(PKG_VERSION)" "$(PKG_BUILDDIR)" |
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.
Do you need to write this environment variable on the command line if it is already exported (vars.mk)?
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.
That is a remnant of when I tried to not export the variable, but it failed because of sub makefiles.
I left it to show that the script was using the variable but I can remove it.
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 it, it is an internal script anyway and git grep GIT_CACHE_DIR
finds it.
I removed the trailing slash for I could go for a |
635c281
to
eee0740
Compare
@jia200x It would be great to have this for the release to be able to test building in |
without this PR:
with this PR:
So, it works for me |
makefiles/docker.inc.mk
Outdated
@@ -83,6 +83,12 @@ DOCKER_OVERRIDE_CMDLINE := $(strip $(DOCKER_OVERRIDE_CMDLINE)) | |||
# Overwrite if you want to use `docker` with sudo | |||
DOCKER ?= docker | |||
|
|||
# Mounted volumes and exported environment variables | |||
|
|||
# Add GIT_CACHE_DIR if the direcotry exists |
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.
typo here:
s/direcotry/directory
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.
Fixed
ACK on my side. Please squash! |
Currently the default value was set by `dist/tools/git/git-cache`. This moves/duplicates the default value in the build system.
If GIT_CACHE_DIR is a directory make it available to docker. This will allow using the system git_cache also in the docker container.
2be71cd
to
c603247
Compare
Squashed, I put some more description in the commit messages. |
ok, all good. GO |
Thank you for reviewing. |
thanks to you for the feature. It saves tons of time |
Contribution description
If GIT_CACHE_DIR is a directory make it available to docker.
I used the same naming convention I used in #9646
Optional
I did not remove the default value for
GIT_CACHE_DIR
fromdist/tools/git/git-cache
but duplicated it in the build system to minimize changes to the script. I could do it in this PR or another if wanted.Testing
You must have
${HOME}/.gitcache
directory created to use git-cache.You can notice the
-e GIT_CACHE_DIR=/data/riotbuild/gitcache
andgit-cache: cloning from cache
.Issues/PRs references
Fixes #8267 if it is ok that I do not create the cache dir by default but only use the existing one.