Message ID | 8121be4437aeae47c5b1d6870e1561b2b088198f.1719033434.git.liezhi.yang@windriver.com |
---|---|
State | New |
Headers | show |
Series | [1/1] fetch2/git: Use basename for gitsrcname when ud.proto is file | expand |
On Fri, 2024-06-21 at 22:20 -0700, Robert Yang via lists.openembedded.org wrote: > From: Robert Yang <liezhi.yang@windriver.com> > > The previous code added local path to generated mirror tarball name which had > two potential issues when fetch from local PREMIRROR, for exapmle: > > Add github.com.lsof-org.lsof to /path/to/local/PREMIRROR/ > $ bitbake lsof -cfetch > > * There might be "Fix File name too long error" when the local path is long > since the path is translated to filename and added to downloads/git2/ as: > path.to.local.PREMIRROR.github.com.lsof-org.lsof > > * When enable BB_GENERATE_MIRROR_TARBALLS: > The tarball name will be: > git2_path.to.local.PREMIRROR.github.com.lsof-org.lsof.tar.gz > > The above tarball can't be re-used by do_fetch since bitbake looks for > 'git2_github.com.lsof-org.lsof.tar.gz', but the 'path.to.local.PREMIRROR' in > the file name breaks the match. > > Use os.path.basename(ud.path) as gitsrcname when ud.proto is file can fix the > the above two issues, now the file name in downloads/git2/ is: > github.com.lsof-org.lsof > > And the generated tarball is: > git2_github.com.lsof-org.lsof.tar.gz > > Or when BB_GIT_SHALLOW is enabled: > gitshallow_github.com.lsof-org.lsof_2e4c7a1-1_master.tar.gz > > And the tarballs can be re-used by do_fetch. > > Signed-off-by: Robert Yang <liezhi.yang@windriver.com> > --- > bitbake/lib/bb/fetch2/git.py | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py > index c7ff769fdf..89d3268f7f 100644 > --- a/bitbake/lib/bb/fetch2/git.py > +++ b/bitbake/lib/bb/fetch2/git.py > @@ -277,7 +277,10 @@ class Git(FetchMethod): > ud.unresolvedrev[name] = ud.revisions[name] > ud.revisions[name] = self.latest_revision(ud, d, name) > > - gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) > + if ud.proto == "file": > + gitsrcname = '%s' % os.path.basename(ud.path) > + else: > + gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) > if gitsrcname.startswith('.'): > gitsrcname = gitsrcname[1:] > I started to review this, then I started wondering why we have mirror tarballs being created for local file:// urls? Does that make sense? Cheers, Richard
Hi RP, On 7/14/24 19:17, Richard Purdie wrote: > On Fri, 2024-06-21 at 22:20 -0700, Robert Yang via lists.openembedded.org wrote: >> From: Robert Yang <liezhi.yang@windriver.com> >> >> The previous code added local path to generated mirror tarball name which had >> two potential issues when fetch from local PREMIRROR, for exapmle: >> >> Add github.com.lsof-org.lsof to /path/to/local/PREMIRROR/ >> $ bitbake lsof -cfetch >> >> * There might be "Fix File name too long error" when the local path is long >> since the path is translated to filename and added to downloads/git2/ as: >> path.to.local.PREMIRROR.github.com.lsof-org.lsof >> >> * When enable BB_GENERATE_MIRROR_TARBALLS: >> The tarball name will be: >> git2_path.to.local.PREMIRROR.github.com.lsof-org.lsof.tar.gz >> >> The above tarball can't be re-used by do_fetch since bitbake looks for >> 'git2_github.com.lsof-org.lsof.tar.gz', but the 'path.to.local.PREMIRROR' in >> the file name breaks the match. >> >> Use os.path.basename(ud.path) as gitsrcname when ud.proto is file can fix the >> the above two issues, now the file name in downloads/git2/ is: >> github.com.lsof-org.lsof >> >> And the generated tarball is: >> git2_github.com.lsof-org.lsof.tar.gz >> >> Or when BB_GIT_SHALLOW is enabled: >> gitshallow_github.com.lsof-org.lsof_2e4c7a1-1_master.tar.gz >> >> And the tarballs can be re-used by do_fetch. >> >> Signed-off-by: Robert Yang <liezhi.yang@windriver.com> >> --- >> bitbake/lib/bb/fetch2/git.py | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py >> index c7ff769fdf..89d3268f7f 100644 >> --- a/bitbake/lib/bb/fetch2/git.py >> +++ b/bitbake/lib/bb/fetch2/git.py >> @@ -277,7 +277,10 @@ class Git(FetchMethod): >> ud.unresolvedrev[name] = ud.revisions[name] >> ud.revisions[name] = self.latest_revision(ud, d, name) >> >> - gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) >> + if ud.proto == "file": >> + gitsrcname = '%s' % os.path.basename(ud.path) >> + else: >> + gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) >> if gitsrcname.startswith('.'): >> gitsrcname = gitsrcname[1:] >> > > I started to review this, then I started wondering why we have mirror > tarballs being created for local file:// urls? Does that make sense? This patch fixed two issues: 1) "File name too long error" when fetch from PREMIRROR 2) Git mirror tarball contains local path when file:// Create git mirror tarball for file:// doesn't make much sense, but the problem is the first issue, there would be the error when len(path) > 255 (The NAME_MAX). // Robert > > Cheers, > > Richard
On Sun, 2024-07-14 at 21:03 +0800, Robert Yang wrote: > Hi RP, > > On 7/14/24 19:17, Richard Purdie wrote: > > On Fri, 2024-06-21 at 22:20 -0700, Robert Yang via > > lists.openembedded.org wrote: > > > From: Robert Yang <liezhi.yang@windriver.com> > > > > > > The previous code added local path to generated mirror tarball > > > name which had > > > two potential issues when fetch from local PREMIRROR, for > > > exapmle: > > > > > > Add github.com.lsof-org.lsof to /path/to/local/PREMIRROR/ > > > $ bitbake lsof -cfetch > > > > > > * There might be "Fix File name too long error" when the local > > > path is long > > > since the path is translated to filename and added to > > > downloads/git2/ as: > > > path.to.local.PREMIRROR.github.com.lsof-org.lsof > > > > > > * When enable BB_GENERATE_MIRROR_TARBALLS: > > > The tarball name will be: > > > git2_path.to.local.PREMIRROR.github.com.lsof-org.lsof.tar.gz > > > > > > The above tarball can't be re-used by do_fetch since bitbake > > > looks for > > > 'git2_github.com.lsof-org.lsof.tar.gz', but the > > > 'path.to.local.PREMIRROR' in > > > the file name breaks the match. > > > > > > Use os.path.basename(ud.path) as gitsrcname when ud.proto is file > > > can fix the > > > the above two issues, now the file name in downloads/git2/ is: > > > github.com.lsof-org.lsof > > > > > > And the generated tarball is: > > > git2_github.com.lsof-org.lsof.tar.gz > > > > > > Or when BB_GIT_SHALLOW is enabled: > > > gitshallow_github.com.lsof-org.lsof_2e4c7a1-1_master.tar.gz > > > > > > And the tarballs can be re-used by do_fetch. > > > > > > Signed-off-by: Robert Yang <liezhi.yang@windriver.com> > > > --- > > > bitbake/lib/bb/fetch2/git.py | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/bitbake/lib/bb/fetch2/git.py > > > b/bitbake/lib/bb/fetch2/git.py > > > index c7ff769fdf..89d3268f7f 100644 > > > --- a/bitbake/lib/bb/fetch2/git.py > > > +++ b/bitbake/lib/bb/fetch2/git.py > > > @@ -277,7 +277,10 @@ class Git(FetchMethod): > > > ud.unresolvedrev[name] = ud.revisions[name] > > > ud.revisions[name] = self.latest_revision(ud, > > > d, name) > > > > > > - gitsrcname = '%s%s' % (ud.host.replace(':', '.'), > > > ud.path.replace('/', '.').replace('*', '.').replace(' > > > ','_').replace('(', '_').replace(')', '_')) > > > + if ud.proto == "file": > > > + gitsrcname = '%s' % os.path.basename(ud.path) > > > + else: > > > + gitsrcname = '%s%s' % (ud.host.replace(':', '.'), > > > ud.path.replace('/', '.').replace('*', '.').replace(' > > > ','_').replace('(', '_').replace(')', '_')) > > > if gitsrcname.startswith('.'): > > > gitsrcname = gitsrcname[1:] > > > > > > > I started to review this, then I started wondering why we have > > mirror > > tarballs being created for local file:// urls? Does that make > > sense? > > This patch fixed two issues: > > 1) "File name too long error" when fetch from PREMIRROR > 2) Git mirror tarball contains local path when file:// > > Create git mirror tarball for file:// doesn't make much sense, but > the problem > is the first issue, there would be the error when len(path) > 255 > (The NAME_MAX). I understand that. However: a) why it is looking for a file:// url in PREMIRRORS? b) why does it consider mirror tarballs for a file:// url? I'm not sure it should be doing either of those things? Cheers, Richard
Hi RP, On 7/14/24 21:32, Richard Purdie via lists.openembedded.org wrote: > On Sun, 2024-07-14 at 21:03 +0800, Robert Yang wrote: >> Hi RP, >> >> On 7/14/24 19:17, Richard Purdie wrote: >>> On Fri, 2024-06-21 at 22:20 -0700, Robert Yang via >>> lists.openembedded.org wrote: >>>> From: Robert Yang <liezhi.yang@windriver.com> >>>> >>>> The previous code added local path to generated mirror tarball >>>> name which had >>>> two potential issues when fetch from local PREMIRROR, for >>>> exapmle: >>>> >>>> Add github.com.lsof-org.lsof to /path/to/local/PREMIRROR/ >>>> $ bitbake lsof -cfetch >>>> >>>> * There might be "Fix File name too long error" when the local >>>> path is long >>>> since the path is translated to filename and added to >>>> downloads/git2/ as: >>>> path.to.local.PREMIRROR.github.com.lsof-org.lsof >>>> >>>> * When enable BB_GENERATE_MIRROR_TARBALLS: >>>> The tarball name will be: >>>> git2_path.to.local.PREMIRROR.github.com.lsof-org.lsof.tar.gz >>>> >>>> The above tarball can't be re-used by do_fetch since bitbake >>>> looks for >>>> 'git2_github.com.lsof-org.lsof.tar.gz', but the >>>> 'path.to.local.PREMIRROR' in >>>> the file name breaks the match. >>>> >>>> Use os.path.basename(ud.path) as gitsrcname when ud.proto is file >>>> can fix the >>>> the above two issues, now the file name in downloads/git2/ is: >>>> github.com.lsof-org.lsof >>>> >>>> And the generated tarball is: >>>> git2_github.com.lsof-org.lsof.tar.gz >>>> >>>> Or when BB_GIT_SHALLOW is enabled: >>>> gitshallow_github.com.lsof-org.lsof_2e4c7a1-1_master.tar.gz >>>> >>>> And the tarballs can be re-used by do_fetch. >>>> >>>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com> >>>> --- >>>> bitbake/lib/bb/fetch2/git.py | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/bitbake/lib/bb/fetch2/git.py >>>> b/bitbake/lib/bb/fetch2/git.py >>>> index c7ff769fdf..89d3268f7f 100644 >>>> --- a/bitbake/lib/bb/fetch2/git.py >>>> +++ b/bitbake/lib/bb/fetch2/git.py >>>> @@ -277,7 +277,10 @@ class Git(FetchMethod): >>>> ud.unresolvedrev[name] = ud.revisions[name] >>>> ud.revisions[name] = self.latest_revision(ud, >>>> d, name) >>>> >>>> - gitsrcname = '%s%s' % (ud.host.replace(':', '.'), >>>> ud.path.replace('/', '.').replace('*', '.').replace(' >>>> ','_').replace('(', '_').replace(')', '_')) >>>> + if ud.proto == "file": >>>> + gitsrcname = '%s' % os.path.basename(ud.path) >>>> + else: >>>> + gitsrcname = '%s%s' % (ud.host.replace(':', '.'), >>>> ud.path.replace('/', '.').replace('*', '.').replace(' >>>> ','_').replace('(', '_').replace(')', '_')) >>>> if gitsrcname.startswith('.'): >>>> gitsrcname = gitsrcname[1:] >>>> >>> >>> I started to review this, then I started wondering why we have >>> mirror >>> tarballs being created for local file:// urls? Does that make >>> sense? >> >> This patch fixed two issues: >> >> 1) "File name too long error" when fetch from PREMIRROR >> 2) Git mirror tarball contains local path when file:// >> >> Create git mirror tarball for file:// doesn't make much sense, but >> the problem >> is the first issue, there would be the error when len(path) > 255 >> (The NAME_MAX). > > I understand that. However: > > a) why it is looking for a file:// url in PREMIRRORS? This is a little WRLinux specific, we release git repos with the product, and set PREMIRROS as: PREMIRRORS:append = " \ git://.*/.* git://${LAYERDIR}/git/MIRRORNAME;protocol=file \n \ gitsm://.*/.* gitsm://${LAYERDIR}/git/MIRRORNAME;protocol=file \n \ > b) why does it consider mirror tarballs for a file:// url? I found the issue when I tried to generate shallow tarballs from the above PREMIRRORS. // Robert. > > I'm not sure it should be doing either of those things? > > Cheers, > > Richard > > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#16421): https://lists.openembedded.org/g/bitbake-devel/message/16421 > Mute This Topic: https://lists.openembedded.org/mt/106812686/3616940 > Group Owner: bitbake-devel+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [liezhi.yang@windriver.com] > -=-=-=-=-=-=-=-=-=-=-=- >
On Fri, 2024-06-21 at 22:20 -0700, Robert Yang via lists.openembedded.org wrote: > From: Robert Yang <liezhi.yang@windriver.com> > > The previous code added local path to generated mirror tarball name which had > two potential issues when fetch from local PREMIRROR, for exapmle: > > Add github.com.lsof-org.lsof to /path/to/local/PREMIRROR/ > $ bitbake lsof -cfetch > > * There might be "Fix File name too long error" when the local path is long > since the path is translated to filename and added to downloads/git2/ as: > path.to.local.PREMIRROR.github.com.lsof-org.lsof > > * When enable BB_GENERATE_MIRROR_TARBALLS: > The tarball name will be: > git2_path.to.local.PREMIRROR.github.com.lsof-org.lsof.tar.gz > > The above tarball can't be re-used by do_fetch since bitbake looks for > 'git2_github.com.lsof-org.lsof.tar.gz', but the 'path.to.local.PREMIRROR' in > the file name breaks the match. > > Use os.path.basename(ud.path) as gitsrcname when ud.proto is file can fix the > the above two issues, now the file name in downloads/git2/ is: > github.com.lsof-org.lsof > > And the generated tarball is: > git2_github.com.lsof-org.lsof.tar.gz > > Or when BB_GIT_SHALLOW is enabled: > gitshallow_github.com.lsof-org.lsof_2e4c7a1-1_master.tar.gz > > And the tarballs can be re-used by do_fetch. > > Signed-off-by: Robert Yang <liezhi.yang@windriver.com> > --- > bitbake/lib/bb/fetch2/git.py | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py > index c7ff769fdf..89d3268f7f 100644 > --- a/bitbake/lib/bb/fetch2/git.py > +++ b/bitbake/lib/bb/fetch2/git.py > @@ -277,7 +277,10 @@ class Git(FetchMethod): > ud.unresolvedrev[name] = ud.revisions[name] > ud.revisions[name] = self.latest_revision(ud, d, name) > > - gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) > + if ud.proto == "file": > + gitsrcname = '%s' % os.path.basename(ud.path) > + else: > + gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) > if gitsrcname.startswith('.'): > gitsrcname = gitsrcname[1:] I did look at this patch again. I'm still not convinced about the original use case but ignoring that, the patch itself isn't too safe. If you have two repositories with the same name, this can break since basename() on the URI is not a unique value. Just as a note, '%s' % x is also poor code as you can just assign the value. Cheers, Richard
> On 6 Aug 2024, at 12:06, Richard Purdie via lists.openembedded.org <richard.purdie=linuxfoundation.org@lists.openembedded.org> wrote: > I did look at this patch again. I'm still not convinced about the > original use case but ignoring that, the patch itself isn't too safe. > If you have two repositories with the same name, this can break since > basename() on the URI is not a unique value. I just tested this and verified this is the behaviour. With a recipe that fetches a local git repo called xyzzy it currently does a clone to $DL_DIR/git2/home.rosbur01.xyzzy but this changes that path to $DL_DIR/git2/xyzzy. My fear is that this is too ambiguous for a DL_DIR path. Ross
Hi RP and Ross, On 8/6/24 19:06, Richard Purdie wrote: > On Fri, 2024-06-21 at 22:20 -0700, Robert Yang via lists.openembedded.org wrote: >> From: Robert Yang <liezhi.yang@windriver.com> >> >> The previous code added local path to generated mirror tarball name which had >> two potential issues when fetch from local PREMIRROR, for exapmle: >> >> Add github.com.lsof-org.lsof to /path/to/local/PREMIRROR/ >> $ bitbake lsof -cfetch >> >> * There might be "Fix File name too long error" when the local path is long >> since the path is translated to filename and added to downloads/git2/ as: >> path.to.local.PREMIRROR.github.com.lsof-org.lsof >> >> * When enable BB_GENERATE_MIRROR_TARBALLS: >> The tarball name will be: >> git2_path.to.local.PREMIRROR.github.com.lsof-org.lsof.tar.gz >> >> The above tarball can't be re-used by do_fetch since bitbake looks for >> 'git2_github.com.lsof-org.lsof.tar.gz', but the 'path.to.local.PREMIRROR' in >> the file name breaks the match. >> >> Use os.path.basename(ud.path) as gitsrcname when ud.proto is file can fix the >> the above two issues, now the file name in downloads/git2/ is: >> github.com.lsof-org.lsof >> >> And the generated tarball is: >> git2_github.com.lsof-org.lsof.tar.gz >> >> Or when BB_GIT_SHALLOW is enabled: >> gitshallow_github.com.lsof-org.lsof_2e4c7a1-1_master.tar.gz >> >> And the tarballs can be re-used by do_fetch. >> >> Signed-off-by: Robert Yang <liezhi.yang@windriver.com> >> --- >> bitbake/lib/bb/fetch2/git.py | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py >> index c7ff769fdf..89d3268f7f 100644 >> --- a/bitbake/lib/bb/fetch2/git.py >> +++ b/bitbake/lib/bb/fetch2/git.py >> @@ -277,7 +277,10 @@ class Git(FetchMethod): >> ud.unresolvedrev[name] = ud.revisions[name] >> ud.revisions[name] = self.latest_revision(ud, d, name) >> >> - gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) >> + if ud.proto == "file": >> + gitsrcname = '%s' % os.path.basename(ud.path) >> + else: >> + gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) >> if gitsrcname.startswith('.'): >> gitsrcname = gitsrcname[1:] > > > I did look at this patch again. I'm still not convinced about the > original use case but ignoring that, the patch itself isn't too safe. Thanks for looking into this, the main problem is that we need a way to fix the filename too long error as described in the commit message above. > If you have two repositories with the same name, this can break since > basename() on the URI is not a unique value. If the two repos are identical, then it isn't a problem since bitbake can handle them correctly, if the two local repos with the same name but have different contents, I think that the user should fix their repos. > > Just as a note, '%s' % x is also poor code as you can just assign the > value. Yes, absolutely, that's my mistake. // Robert > > Cheers, > > Richard > > >
On 8/6/24 21:42, Robert Yang via lists.openembedded.org wrote: > Hi RP and Ross, Sorry, correct Ross' email. // Robert > > On 8/6/24 19:06, Richard Purdie wrote: >> On Fri, 2024-06-21 at 22:20 -0700, Robert Yang via lists.openembedded.org wrote: >>> From: Robert Yang <liezhi.yang@windriver.com> >>> >>> The previous code added local path to generated mirror tarball name which had >>> two potential issues when fetch from local PREMIRROR, for exapmle: >>> >>> Add github.com.lsof-org.lsof to /path/to/local/PREMIRROR/ >>> $ bitbake lsof -cfetch >>> >>> * There might be "Fix File name too long error" when the local path is long >>> since the path is translated to filename and added to downloads/git2/ as: >>> path.to.local.PREMIRROR.github.com.lsof-org.lsof >>> >>> * When enable BB_GENERATE_MIRROR_TARBALLS: >>> The tarball name will be: >>> git2_path.to.local.PREMIRROR.github.com.lsof-org.lsof.tar.gz >>> >>> The above tarball can't be re-used by do_fetch since bitbake looks for >>> 'git2_github.com.lsof-org.lsof.tar.gz', but the 'path.to.local.PREMIRROR' in >>> the file name breaks the match. >>> >>> Use os.path.basename(ud.path) as gitsrcname when ud.proto is file can fix the >>> the above two issues, now the file name in downloads/git2/ is: >>> github.com.lsof-org.lsof >>> >>> And the generated tarball is: >>> git2_github.com.lsof-org.lsof.tar.gz >>> >>> Or when BB_GIT_SHALLOW is enabled: >>> gitshallow_github.com.lsof-org.lsof_2e4c7a1-1_master.tar.gz >>> >>> And the tarballs can be re-used by do_fetch. >>> >>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com> >>> --- >>> bitbake/lib/bb/fetch2/git.py | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py >>> index c7ff769fdf..89d3268f7f 100644 >>> --- a/bitbake/lib/bb/fetch2/git.py >>> +++ b/bitbake/lib/bb/fetch2/git.py >>> @@ -277,7 +277,10 @@ class Git(FetchMethod): >>> ud.unresolvedrev[name] = ud.revisions[name] >>> ud.revisions[name] = self.latest_revision(ud, d, name) >>> - gitsrcname = '%s%s' % (ud.host.replace(':', '.'), >>> ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', >>> '_').replace(')', '_')) >>> + if ud.proto == "file": >>> + gitsrcname = '%s' % os.path.basename(ud.path) >>> + else: >>> + gitsrcname = '%s%s' % (ud.host.replace(':', '.'), >>> ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', >>> '_').replace(')', '_')) >>> if gitsrcname.startswith('.'): >>> gitsrcname = gitsrcname[1:] >> >> >> I did look at this patch again. I'm still not convinced about the >> original use case but ignoring that, the patch itself isn't too safe. > > Thanks for looking into this, the main problem is that we need a way to fix the > filename too long error as described in the commit message above. > >> If you have two repositories with the same name, this can break since >> basename() on the URI is not a unique value. > > If the two repos are identical, then it isn't a problem since bitbake > can handle them correctly, if the two local repos with the same > name but have different contents, I think that the user should fix their repos. > >> >> Just as a note, '%s' % x is also poor code as you can just assign the >> value. > > Yes, absolutely, that's my mistake. > > // Robert > >> >> Cheers, >> >> Richard >> >> >> > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#16447): https://lists.openembedded.org/g/bitbake-devel/message/16447 > Mute This Topic: https://lists.openembedded.org/mt/106812686/3616940 > Group Owner: bitbake-devel+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [liezhi.yang@windriver.com] > -=-=-=-=-=-=-=-=-=-=-=- >
On Tue, 2024-08-06 at 21:42 +0800, Robert Yang wrote: > Hi RP and Ross, > > On 8/6/24 19:06, Richard Purdie wrote: > > On Fri, 2024-06-21 at 22:20 -0700, Robert Yang via lists.openembedded.org wrote: > > > From: Robert Yang <liezhi.yang@windriver.com> > > > > > > The previous code added local path to generated mirror tarball name which had > > > two potential issues when fetch from local PREMIRROR, for exapmle: > > > > > > Add github.com.lsof-org.lsof to /path/to/local/PREMIRROR/ > > > $ bitbake lsof -cfetch > > > > > > * There might be "Fix File name too long error" when the local path is long > > > since the path is translated to filename and added to downloads/git2/ as: > > > path.to.local.PREMIRROR.github.com.lsof-org.lsof > > > > > > * When enable BB_GENERATE_MIRROR_TARBALLS: > > > The tarball name will be: > > > git2_path.to.local.PREMIRROR.github.com.lsof-org.lsof.tar.gz > > > > > > The above tarball can't be re-used by do_fetch since bitbake looks for > > > 'git2_github.com.lsof-org.lsof.tar.gz', but the 'path.to.local.PREMIRROR' in > > > the file name breaks the match. > > > > > > Use os.path.basename(ud.path) as gitsrcname when ud.proto is file can fix the > > > the above two issues, now the file name in downloads/git2/ is: > > > github.com.lsof-org.lsof > > > > > > And the generated tarball is: > > > git2_github.com.lsof-org.lsof.tar.gz > > > > > > Or when BB_GIT_SHALLOW is enabled: > > > gitshallow_github.com.lsof-org.lsof_2e4c7a1-1_master.tar.gz > > > > > > And the tarballs can be re-used by do_fetch. > > > > > > Signed-off-by: Robert Yang <liezhi.yang@windriver.com> > > > --- > > > bitbake/lib/bb/fetch2/git.py | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py > > > index c7ff769fdf..89d3268f7f 100644 > > > --- a/bitbake/lib/bb/fetch2/git.py > > > +++ b/bitbake/lib/bb/fetch2/git.py > > > @@ -277,7 +277,10 @@ class Git(FetchMethod): > > > ud.unresolvedrev[name] = ud.revisions[name] > > > ud.revisions[name] = self.latest_revision(ud, d, name) > > > > > > - gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) > > > + if ud.proto == "file": > > > + gitsrcname = '%s' % os.path.basename(ud.path) > > > + else: > > > + gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) > > > if gitsrcname.startswith('.'): > > > gitsrcname = gitsrcname[1:] > > > > > > I did look at this patch again. I'm still not convinced about the > > original use case but ignoring that, the patch itself isn't too safe. > > Thanks for looking into this, the main problem is that we need a way to fix the > filename too long error as described in the commit message above. > > > If you have two repositories with the same name, this can break since > > basename() on the URI is not a unique value. > > If the two repos are identical, then it isn't a problem since bitbake > can handle them correctly, if the two local repos with the same > name but have different contents, I think that the user should fix their repos. The trouble is that this doesn't just affect local repos, the contents of DL_DIR can and will be shared as a public mirror so it is any two repos with the same name, not just two local repos. This is partly why I still think mirrors of local file repos don't make sense. Cheers, Richard
Hi RP, On 8/6/24 22:10, Richard Purdie wrote: > On Tue, 2024-08-06 at 21:42 +0800, Robert Yang wrote: >> Hi RP and Ross, >> >> On 8/6/24 19:06, Richard Purdie wrote: >>> On Fri, 2024-06-21 at 22:20 -0700, Robert Yang via lists.openembedded.org wrote: >>>> From: Robert Yang <liezhi.yang@windriver.com> >>>> >>>> The previous code added local path to generated mirror tarball name which had >>>> two potential issues when fetch from local PREMIRROR, for exapmle: >>>> >>>> Add github.com.lsof-org.lsof to /path/to/local/PREMIRROR/ >>>> $ bitbake lsof -cfetch >>>> >>>> * There might be "Fix File name too long error" when the local path is long >>>> since the path is translated to filename and added to downloads/git2/ as: >>>> path.to.local.PREMIRROR.github.com.lsof-org.lsof >>>> >>>> * When enable BB_GENERATE_MIRROR_TARBALLS: >>>> The tarball name will be: >>>> git2_path.to.local.PREMIRROR.github.com.lsof-org.lsof.tar.gz >>>> >>>> The above tarball can't be re-used by do_fetch since bitbake looks for >>>> 'git2_github.com.lsof-org.lsof.tar.gz', but the 'path.to.local.PREMIRROR' in >>>> the file name breaks the match. >>>> >>>> Use os.path.basename(ud.path) as gitsrcname when ud.proto is file can fix the >>>> the above two issues, now the file name in downloads/git2/ is: >>>> github.com.lsof-org.lsof >>>> >>>> And the generated tarball is: >>>> git2_github.com.lsof-org.lsof.tar.gz >>>> >>>> Or when BB_GIT_SHALLOW is enabled: >>>> gitshallow_github.com.lsof-org.lsof_2e4c7a1-1_master.tar.gz >>>> >>>> And the tarballs can be re-used by do_fetch. >>>> >>>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com> >>>> --- >>>> bitbake/lib/bb/fetch2/git.py | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py >>>> index c7ff769fdf..89d3268f7f 100644 >>>> --- a/bitbake/lib/bb/fetch2/git.py >>>> +++ b/bitbake/lib/bb/fetch2/git.py >>>> @@ -277,7 +277,10 @@ class Git(FetchMethod): >>>> ud.unresolvedrev[name] = ud.revisions[name] >>>> ud.revisions[name] = self.latest_revision(ud, d, name) >>>> >>>> - gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) >>>> + if ud.proto == "file": >>>> + gitsrcname = '%s' % os.path.basename(ud.path) >>>> + else: >>>> + gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) >>>> if gitsrcname.startswith('.'): >>>> gitsrcname = gitsrcname[1:] >>> >>> >>> I did look at this patch again. I'm still not convinced about the >>> original use case but ignoring that, the patch itself isn't too safe. >> >> Thanks for looking into this, the main problem is that we need a way to fix the >> filename too long error as described in the commit message above. >> >>> If you have two repositories with the same name, this can break since >>> basename() on the URI is not a unique value. >> >> If the two repos are identical, then it isn't a problem since bitbake >> can handle them correctly, if the two local repos with the same >> name but have different contents, I think that the user should fix their repos. > > The trouble is that this doesn't just affect local repos, the contents > of DL_DIR can and will be shared as a public mirror so it is any two > repos with the same name, not just two local repos. This only affects protocol=file, without this patch, the git://path/to/local/path/gitrepo.git:protocol=file after do_fetch, it will be: downloads/git2/path.to.local.path.gitrepo.git The above name can't be used on public mirror because it can't be matched, unless different people and projects use the same local path, but this isn't the reality, for example, my local path is: git://home/robert/to/gitrepo.git;protocol=file When I upload it to a public mirror, it will be: home.robert.to.gitrepo.git For you, it might be: git://home/rp/to/gitrepo.git;protocol=file What your do_fetch looking for is: home.rp.to.gitrepo.git So it can't be fetched from public mirror, unless you use: git://home/robert/to/gitrepo.git;protocol=file But this isn't the reality > > This is partly why I still think mirrors of local file repos don't make > sense. It's not only for mirror, but will cause "Filename too long error" for normal build's do_fetch when: len(/home/robert/to/long/dir/gitrepo.git) > 256 You can simulate the error with the following command on Ubuntu 20.04 or 22.04: $ touch $(for i in $(seq 1 30); do echo -n longpart$i; done) File name too long This is because the current code changes the path to filename, and len(filename) > 256. // Robert > > Cheers, > > Richard >
On Wed, 2024-08-07 at 10:23 +0800, Robert Yang via lists.openembedded.org wrote: > Hi RP, > > On 8/6/24 22:10, Richard Purdie wrote: > > On Tue, 2024-08-06 at 21:42 +0800, Robert Yang wrote: > > > > The trouble is that this doesn't just affect local repos, the > > contents > > of DL_DIR can and will be shared as a public mirror so it is any > > two > > repos with the same name, not just two local repos. > > This only affects protocol=file, without this patch, the > > git://path/to/local/path/gitrepo.git:protocol=file > > after do_fetch, it will be: > > downloads/git2/path.to.local.path.gitrepo.git Currently we don't support mirroring of file protocol git urls as they don't really make sense. > The above name can't be used on public mirror because it can't be > matched, unless different people and projects use the same local > path, but this isn't the reality, for example, my local path is: > > git://home/robert/to/gitrepo.git;protocol=file Correct, but the path is uniquely identified. > When I upload it to a public mirror, it will be: > home.robert.to.gitrepo.git > > For you, it might be: > git://home/rp/to/gitrepo.git;protocol=file > > What your do_fetch looking for is: > home.rp.to.gitrepo.git > > So it can't be fetched from public mirror, unless you use: > git://home/robert/to/gitrepo.git;protocol=file > > But this isn't the reality Agreed, they simply don't work today and are not supported. > > This is partly why I still think mirrors of local file repos don't > > make > > sense. > > It's not only for mirror, but will cause "Filename too long error" > for normal build's do_fetch when: > > len(/home/robert/to/long/dir/gitrepo.git) > 256 > > You can simulate the error with the following command on Ubuntu > 20.04 or 22.04: > $ touch $(for i in $(seq 1 30); do echo -n longpart$i; done) > > File name too long > > This is because the current code changes the path to filename, and > len(filename) > 256. Sure, but this isn't a good reason to adopt an unsafe naming scheme for something we don't currently support and don't really want to add support for. Cheers, Richard
diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py index c7ff769fdf..89d3268f7f 100644 --- a/bitbake/lib/bb/fetch2/git.py +++ b/bitbake/lib/bb/fetch2/git.py @@ -277,7 +277,10 @@ class Git(FetchMethod): ud.unresolvedrev[name] = ud.revisions[name] ud.revisions[name] = self.latest_revision(ud, d, name) - gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) + if ud.proto == "file": + gitsrcname = '%s' % os.path.basename(ud.path) + else: + gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.').replace(' ','_').replace('(', '_').replace(')', '_')) if gitsrcname.startswith('.'): gitsrcname = gitsrcname[1:]