| Message ID | 20240530165719.3115747-1-richard.purdie@linuxfoundation.org |
|---|---|
| State | Accepted, archived |
| Commit | c56bd9a9280378bc64c6a7fe6d7b70847e0b9e6d |
| Headers | show |
| Series | fetch2/wget: Fix failure path for files that are empty or don't exist | expand |
On Thu, May 30, 2024 at 9:57 AM Richard Purdie via lists.openembedded.org <richard.purdie=linuxfoundation.org@lists.openembedded.org> wrote: > When we intercepted the file download to a temp file, we broke the > exist/size checks which need to happen before the rename. Correct > the ordering. > > For some reason, python 3.12 exposes this problem in the selftests > differently to previous versions. > > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> > --- > lib/bb/fetch2/wget.py | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/lib/bb/fetch2/wget.py b/lib/bb/fetch2/wget.py > index fbfa6938ac..3c213370b5 100644 > --- a/lib/bb/fetch2/wget.py > +++ b/lib/bb/fetch2/wget.py > @@ -139,19 +139,19 @@ class Wget(FetchMethod): > # source, one with an incorrect checksum) > bb.fetch2.verify_checksum(ud, d, localpath=localpath, > fatal_nochecksum=False) > > - # Remove the ".tmp" and move the file into position atomically > - # Our lock prevents multiple writers but mirroring code may grab > incomplete files > - os.rename(localpath, localpath[:-4]) > - > # Sanity check since wget can pretend it succeed when it didn't > # Also, this used to happen if sourceforge sent us to the mirror > page > - if not os.path.exists(ud.localpath): > + if not os.path.exists(localpath): > raise FetchError("The fetch command returned success for url > %s but %s doesn't exist?!" % (uri, ud.localpath), uri) > Do we also want to change 'ud.localpath' to 'localpath' in the FetchError message? > > - if os.path.getsize(ud.localpath) == 0: > - os.remove(ud.localpath) > + if os.path.getsize(localpath) == 0: > + os.remove(localpath) > raise FetchError("The fetch of %s resulted in a zero size > file?! Deleting and failing since this isn't right." % (uri), uri) > > + # Remove the ".tmp" and move the file into position atomically > + # Our lock prevents multiple writers but mirroring code may grab > incomplete files > + os.rename(localpath, localpath[:-4]) > + > return True > > def checkstatus(self, fetch, ud, d, try_again=True): > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#16293): > https://lists.openembedded.org/g/bitbake-devel/message/16293 > Mute This Topic: https://lists.openembedded.org/mt/106392412/924729 > Group Owner: bitbake-devel+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [ > ticotimo@gmail.com] > -=-=-=-=-=-=-=-=-=-=-=- > >
On Thu, 2024-05-30 at 10:11 -0700, Tim Orling wrote: > On Thu, May 30, 2024 at 9:57 AM Richard Purdie via lists.openembedded.org <richard.purdie=linuxfoundation.org@lists.openembedded.org> wrote: > > > > # Sanity check since wget can pretend it succeed when it didn't > > # Also, this used to happen if sourceforge sent us to the mirror page > > - if not os.path.exists(ud.localpath): > > + if not os.path.exists(localpath): > > raise FetchError("The fetch command returned success for url %s but %s doesn't exist?!" % (uri, ud.localpath), uri) > > > > > Do we also want to change 'ud.localpath' to 'localpath' in the FetchError message? We do, yes. However whilst this fixed things in my local test, the queued commit in master-next didn't work on the f40 worker. Something odd is still going on which we need to get to the bottom of :/. Cheers, Richard
diff --git a/lib/bb/fetch2/wget.py b/lib/bb/fetch2/wget.py index fbfa6938ac..3c213370b5 100644 --- a/lib/bb/fetch2/wget.py +++ b/lib/bb/fetch2/wget.py @@ -139,19 +139,19 @@ class Wget(FetchMethod): # source, one with an incorrect checksum) bb.fetch2.verify_checksum(ud, d, localpath=localpath, fatal_nochecksum=False) - # Remove the ".tmp" and move the file into position atomically - # Our lock prevents multiple writers but mirroring code may grab incomplete files - os.rename(localpath, localpath[:-4]) - # Sanity check since wget can pretend it succeed when it didn't # Also, this used to happen if sourceforge sent us to the mirror page - if not os.path.exists(ud.localpath): + if not os.path.exists(localpath): raise FetchError("The fetch command returned success for url %s but %s doesn't exist?!" % (uri, ud.localpath), uri) - if os.path.getsize(ud.localpath) == 0: - os.remove(ud.localpath) + if os.path.getsize(localpath) == 0: + os.remove(localpath) raise FetchError("The fetch of %s resulted in a zero size file?! Deleting and failing since this isn't right." % (uri), uri) + # Remove the ".tmp" and move the file into position atomically + # Our lock prevents multiple writers but mirroring code may grab incomplete files + os.rename(localpath, localpath[:-4]) + return True def checkstatus(self, fetch, ud, d, try_again=True):
When we intercepted the file download to a temp file, we broke the exist/size checks which need to happen before the rename. Correct the ordering. For some reason, python 3.12 exposes this problem in the selftests differently to previous versions. Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> --- lib/bb/fetch2/wget.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)