pkg/pkg.mk: Avoid doing a git fetch if we already have the commit.#11491
pkg/pkg.mk: Avoid doing a git fetch if we already have the commit.#11491jcarrano wants to merge 2 commits intoRIOT-OS:masterfrom
Conversation
If the commit is already in the local repo, we should not need to do any network operation.
jcarrano
added
Type: bug
pkg/pkg.mk
Outdated
| if [ $(shell git -C $(PKG_BUILDDIR) rev-parse HEAD) != $(PKG_VERSION) ] ; then \ | ||
| git -C $(PKG_BUILDDIR) clean -xdff ; \ | ||
| git -C $(PKG_BUILDDIR) fetch "$(PKG_URL)" "$(PKG_VERSION)" ; \ | ||
| if git cat-file -e -t '$(PKG_VERSION)^{commit}' ; then \ |
There was a problem hiding this comment.
On ubuntu bionic, git does not like -e -t.
error: switch t' : incompatible with -e`.
I would like to maybe put it outside in a "git_check_version_is_a_commit" so document that it detects branches.
There was a problem hiding this comment.
git cat-file -e 2019.04^{commit} && echo commit
commit
git cat-file -e 2019.04-branch^{commit}
fatal: Not a valid object name 2019.04-branch^{commit}
Also works with a hash as expected.
cladmi
left a comment
cladmi
left a comment
There was a problem hiding this comment.
Does not work with ubuntu bionic and comments inline.
I forgot the -C in git, also negate the condition and remove the -t.
|
It does not re-fetch with |
c25519 is not using git. |
Ahhhh did not pay attention to that >< |
|
This PR is an improvement (unnecessary fetches are avoided). ACK. |
|
please rebase & squash! |
|
There are some other issues with "git_ensure_version":
The following "git clean" removes ".git_downloaded" and ".git_patched" unconditionally, causing any recursive make to re-do the clean and patch again. |
|
I found another issue: this currently does A check whether the to-be-cleaned folder is a git checkout needs to be added. @jcarrano @MrKevinWeiss I think this is release critical, I lost quite some work because of this. |
Noted, I push for it |
MrKevinWeiss
added
the
Process: needs backport
All the code from |
|
However, this is somehow unrelated |
|
@jcarrano Is there anything holding up the squash? |
|
I can't reproduce this issue in master, I believe this is fixed, can I close? |
fjmolinas
removed
the
Process: needs backport
nope, it is just quieted. Use e.g., this patch: https://gist.github.com/kaspar030/d0bcf4f38d91d2b7e785b8327da608be/raw ... then type make a couple of times in a git package with patches, e.g., |
That is not master. |
|
Or maybe this one is still a smarter way of fetching event if its not fetch on each make incocation? |
|
The problem targeted by this PR was fixed by #13036, which is already merged. I'm closing this one but feel free to re-open if you disagree. |
|
I don't thing it is fixed #13036 does not address smart fetching. |
Can you elaborate on this ? I'm trying to reproduce the issue on master and I can't: git fetch is not called anymore on a package that is already cloned/patched. |
|
@fjmolinas: Ping? |
|
Is this still a thing? Anyhow, I'll postpone this as it doesn't seem to break anything ATM. |
10 participants
Contribution description
D'oh! I screwed up a little in #11129 and a git fetch was being done for each package every time the user ran make.
If the commit is already in the local repo, we should not need to do any network operation.
Testing procedure
Type
maketwice in any example that uses packages. See how it is fetched twice. With this patch it is not.Issues/PRs references
Bug introduced in #11129.