autobuild-run: remove only tarballs from download dir

scripts/autobuild-run def prepare_build(**kwargs): · line 296

Quoted 12 lines

scripts/autobuild-run +3 -0

index 33d0ae9..6a01151 100755
@@ -291,6 +291,9 @@ def prepare_build(**kwargs):
     # recursively find files under root
     def find_files(root):
         for r, d, f in os.walk(root):
+            if '.git' in d:
+                d[:] = list()
+                continue

Just to check how this is supposed to work. We have this:

dl/<package>/git/.git

Correct ?

So you're idea is that when we are inside dl/<package>/git/, one of the sub-directories is .git, and therefore we shouldn't remove anything in dl/<package>/git/ ?

My concern is that I'm not sure if what you've done prevents from removing files inside dl/<package>/git or only inside dl/<package>/git/.git. I would find it more to do something like:

	if "git" in d:
		d.remove("git")

but perhaps you haven't done this for some good reason ?

Another concern is how to fix those autobuilders that have already removed some random files from their cached Git repositories? Should we ask the people who run those autobuilders to entirely wipe the download folders of their autobuilder instances ? Or do we have a smart (but simple) thing to do to avoid this ?

Quoted 17 lines

Just to check how this is supposed to work. We have this:

dl/<package>/git/.git

Correct ?

So you're idea is that when we are inside dl/<package>/git/, one of the sub-directories is .git, and therefore we shouldn't remove anything in dl/<package>/git/ ?

My concern is that I'm not sure if what you've done prevents from removing files inside dl/<package>/git or only inside dl/<package>/git/.git. I would find it more to do something like:

	if "git" in d:
		d.remove("git")

Quoted 7 lines

but perhaps you haven't done this for some good reason ?

Another concern is how to fix those autobuilders that have already removed some random files from their cached Git repositories? Should we ask the people who run those autobuilders to entirely wipe the download folders of their autobuilder instances ? Or do we have a smart (but simple) thing to do to avoid this ?

For this last part, we've already discused this with Ricardo in another
thread: we run git-fsck, and if there is an error, we ditch the git tree and clone from scratch.

Quoted 25 lines

Quoted 24 lines

Just to check how this is supposed to work. We have this:

dl/<package>/git/.git

Correct ?

So you're idea is that when we are inside dl/<package>/git/, one of the sub-directories is .git, and therefore we shouldn't remove anything in dl/<package>/git/ ?

My concern is that I'm not sure if what you've done prevents from removing files inside dl/<package>/git or only inside dl/<package>/git/.git. I would find it more to do something like:

	if "git" in d:
		d.remove("git")

but perhaps you haven't done this for some good reason ?

Another concern is how to fix those autobuilders that have already removed some random files from their cached Git repositories? Should we ask the people who run those autobuilders to entirely wipe the download folders of their autobuilder instances ? Or do we have a smart (but simple) thing to do to avoid this ?

Quoted 3 lines

For this last part, we've already discused this with Ricardo in another
thread: we run git-fsck, and if there is an error, we ditch the git tree and clone from scratch.

OK, so in practice removing random files inside the git/ folders should not cause any problem ?

Quoted 2 lines

OK, so in practice removing random files inside the git/ folders should not cause any problem ?

Well, if you remove files from the working copy, no. The next git-checkout would fix it to a certain extent, which can be covered by a mix of git-clean + git-checkout, again to a certain extent...

However, if you remove files from below .git, you can cause heavy breakage. Even removing a single object will cause the tree to get uterly broken, not telling what happens when one removes a ref or HEAD.

So, runniong git-fsck would catch the cases where something is missing in .git, in which case we're toast and the git cache is as good as if it were not present.

Quoted 8 lines

Quoted 3 lines

OK, so in practice removing random files inside the git/ folders should not cause any problem ?

Well, if you remove files from the working copy, no. The next git-checkout would fix it to a certain extent, which can be covered by a mix of git-clean + git-checkout, again to a certain extent...

Quoted 8 lines

However, if you remove files from below .git, you can cause heavy breakage. Even removing a single object will cause the tree to get uterly broken, not telling what happens when one removes a ref or HEAD.

So, runniong git-fsck would catch the cases where something is missing in .git, in which case we're toast and the git cache is as good as if it were not present.

Still not following you. Are you saying that today we do *not* run git-fsck, and therefore a borked repository is not detected, but that we could add a call to git-fsck to detect borked repositories, and therefore avoid the problems of the autobuilders already having borked Git caches ?

Quoted 5 lines

Another concern is how to fix those autobuilders that have already removed some random files from their cached Git repositories? Should we ask the people who run those autobuilders to entirely wipe the download folders of their autobuilder instances ? Or do we have a smart (but simple) thing to do to avoid this ?

For this last part, we've already discused this with Ricardo in another
thread: we run git-fsck, and if there is an error, we ditch the git tree and clone from scratch.

See the current work I have to further robustify (does that even exist?) the git backend:
https://git.buildroot.org/~ymorin/git/buildroot/log/?h=yem/git-robust

scripts/autobuild-run def prepare_build(**kwargs): · line 296

Quoted 11 lines · Ricardo Martincoski

scripts/autobuild-run +3 -0

index 33d0ae9..6a01151 100755
@@ -291,6 +291,9 @@ def prepare_build(**kwargs):
     # recursively find files under root
     def find_files(root):
         for r, d, f in os.walk(root):
+            if '.git' in d:
+                d[:] = list()
+                continue

Just to check how this is supposed to work. We have this:

dl/<package>/git/.git

Correct ?

So you're idea is that when we are inside dl/<package>/git/, one of the sub-directories is .git, and therefore we shouldn't remove anything in dl/<package>/git/ ?

My concern is that I'm not sure if what you've done prevents from removing files inside dl/<package>/git or only inside dl/<package>/git/.git. I would find it more to do something like:

	if "git" in d:
		d.remove("git")

but perhaps you haven't done this for some good reason ?

Another concern is how to fix those autobuilders that have already removed some random files from their cached Git repositories? Should we ask the people who run those autobuilders to entirely wipe the download folders of their autobuilder instances ? Or do we have a smart (but simple) thing to do to avoid this ?

Quoted 3 lines

So you're idea is that when we are inside dl/<package>/git/, one of the sub-directories is .git, and therefore we shouldn't remove anything in dl/<package>/git/ ?

Yes. My idea is to keep the behavior described in the comment:
# Remove 5 random files from the download directory. Removing

# random files from the download directory allows to ensure we
# regularly re-download files to check that their upstream
# location is still correct.

So we keep testing in autobuilders that the upstream location is correct (the git fetch fails and therefore the download script fails if the server does not respond even in the case we already have the commits and references we need in the cache; it is the correct behavior IMO), and we also test the generation of tarball, but we do not test the re-download with a clean git cache; this could be done later on autobuilder with extra code (maybe remove only 1 git cache every run together to 5 tarballs?), but the most important scenario is already
tested: the use of git cache in the long run.

Quoted 5 lines

Another concern is how to fix those autobuilders that have already removed some random files from their cached Git repositories? Should we ask the people who run those autobuilders to entirely wipe the download folders of their autobuilder instances ? Or do we have a smart (but simple) thing to do to avoid this ?

I agree with Yann about the ditch + restart for broken git cache. But if you want, maybe in one autobuilder instance we can manually remove all git caches just to know earlier (before applying Yann's series) that no more download issues will occur, since this change to the script prevents corrupting new git caches.
I guess but did not tested this command would be enough:
rm -r instance-0/dl/*/git
Your call. I have no preference.

But leaving at least one instance not fixed (with some git cache broken) can help to test Yann's series.

Quoted 3 lines

So you're idea is that when we are inside dl/<package>/git/, one of the sub-directories is .git, and therefore we shouldn't remove anything in dl/<package>/git/ ?

Yes. My idea is to keep the behavior described in the comment:
# Remove 5 random files from the download directory. Removing

# random files from the download directory allows to ensure we
# regularly re-download files to check that their upstream
# location is still correct.

So we keep testing in autobuilders that the upstream location is correct (the git fetch fails and therefore the download script fails if the server does not respond even in the case we already have the commits and references we need in the cache; it is the correct behavior IMO), and we also test the generation of tarball, but we do not test the re-download with a clean git cache; this could be done later on autobuilder with extra code (maybe remove only 1 git cache every run together to 5 tarballs?), but the most important scenario is already
tested: the use of git cache in the long run.

It prevents from removing files inside dl/<package>/git

Quoted 5 lines

	if "git" in d:
		d.remove("git")

but perhaps you haven't done this for some good reason ?

Yes, we want to eventually remove dl/git/git-2.16.3.tar.xz

Quoted 5 lines

Another concern is how to fix those autobuilders that have already removed some random files from their cached Git repositories? Should we ask the people who run those autobuilders to entirely wipe the download folders of their autobuilder instances ? Or do we have a smart (but simple) thing to do to avoid this ?

I agree with Yann about the ditch + restart for broken git cache. But if you want, maybe in one autobuilder instance we can manually remove all git caches just to know earlier (before applying Yann's series) that no more download issues will occur, since this change to the script prevents corrupting new git caches.
I guess but did not tested this command would be enough:
rm -r instance-0/dl/*/git
Your call. I have no preference.

scripts/autobuild-run def prepare_build(**kwargs): · line 295

Quoted 4 lines

def find_files(root):
for r, d, f in os.walk(root):

scripts/autobuild-run +2 -0

+            if '.git' in d:
+                d[:] = list()

Python question: is there any advantage in doing d[:] = list() instead of just d = list() ?

Quoted 5 lines

Quoted 5 lines

a=[1,2]
b=a
a=list()
a.append(3)
print(a,b)

Quoted 5 lines

Quoted 5 lines

a=[1,2]
b=a
a[:]=list()
a.append(3)
print(a,b)

Also, do we need to actually set d to the empty list ? We're anyway doing a continue, and skipping over to the next iteration, where a new value of d will be returned by os.walk(). Am I missing something ?

I think I'm also missing how this will prevent from iterating in sub-directories, like git/<something>/foo/. I guess I need to read again how os.walk() is working exactly.

The current logic that removes 5 files at random to force the download to be tested eventually ends up removing files from the git repositories used as cache by the download infra.

When a file that was previously checked out in a git cache is removed, the next 'git checkout' succeeds but does not checkout that file. The repo remains with 'Changes not staged for commit', the tarball is generated and the hash mismatches, causing a build failure.

Ensure only the tarballs are removed by excluding any file inside a git repository from the list of potential files to remove.
The download infra for git method uses 'git fetch' when regenerating the tarball, this command fails as expected if the upstream location changed.

Reported-by Thomas Petazzoni <[email protected]>

Reported-by Yann E. MORIN <[email protected]>

Signed-off-by Ricardo Martincoski <[email protected]>

Cc Peter Korsgaard <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Yann E. MORIN <[email protected]>

---
Changes v1 -> v2: (after review by Thomas Petazzoni)
- add comment to the code to clarify why we test for '.git' and not

    'git';

- use the most correct syntax: del;