[bitbake-devel,20/20] fetch: allow regexps in mirror protocol

Submitted by Enrico Scholz on June 27, 2012, 2:52 p.m.

Details

Message ID 1340808774-24884-1-git-send-email-enrico.scholz@sigma-chemnitz.de
State New
Headers show

Commit Message

Enrico Scholz June 27, 2012, 2:52 p.m.
Last mirror rewrite caused a regression not accepting

   .*://.*/.* file://${DL_DIR}/../local/

like specifications anymore. Patch restores old behavior by using regexp
matching when checking protocol.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 lib/bb/fetch2/__init__.py |    2 +-
 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)

Patch hide | download patch | download mbox

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index 6f3d88c..75ce01b 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -207,7 +207,7 @@  def uri_replace(ud, uri_find, uri_replace, d):
         elif loc == 0:
             # Principle of least surprise. We could end up with https matching against http and 
             # generating "files://" urls if we use the regexp engine below.
-            if i != uri_decoded[loc]:
+            if not re.match(i, uri_decoded[loc]):
                 return None
             result_decoded[loc] = uri_replace_decoded[loc]
         elif (re.match(i, uri_decoded[loc])):

Comments

Robert P. J. Day June 27, 2012, 2:59 p.m.
On Wed, 27 Jun 2012, Enrico Scholz wrote:

> Last mirror rewrite caused a regression not accepting
>
>    .*://.*/.* file://${DL_DIR}/../local/
>
> like specifications anymore. Patch restores old behavior by using regexp
> matching when checking protocol.
>
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> ---
>  lib/bb/fetch2/__init__.py |    2 +-
>  1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)
>
> diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> index 6f3d88c..75ce01b 100644
> --- a/lib/bb/fetch2/__init__.py
> +++ b/lib/bb/fetch2/__init__.py
> @@ -207,7 +207,7 @@ def uri_replace(ud, uri_find, uri_replace, d):
>          elif loc == 0:
>              # Principle of least surprise. We could end up with https matching against http and
>              # generating "files://" urls if we use the regexp engine below.
> -            if i != uri_decoded[loc]:
> +            if not re.match(i, uri_decoded[loc]):
>                  return None
>              result_decoded[loc] = uri_replace_decoded[loc]
>          elif (re.match(i, uri_decoded[loc])):
> --
> 1.7.10.2

  that certainly seems to have solved my issue with using
"own-mirrors".

rday
Richard Purdie June 27, 2012, 8:35 p.m.
On Wed, 2012-06-27 at 16:52 +0200, Enrico Scholz wrote:
> Last mirror rewrite caused a regression not accepting
> 
>    .*://.*/.* file://${DL_DIR}/../local/
> 
> like specifications anymore. Patch restores old behavior by using regexp
> matching when checking protocol.
> 
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> ---
>  lib/bb/fetch2/__init__.py |    2 +-
>  1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)
> 
> diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> index 6f3d88c..75ce01b 100644
> --- a/lib/bb/fetch2/__init__.py
> +++ b/lib/bb/fetch2/__init__.py
> @@ -207,7 +207,7 @@ def uri_replace(ud, uri_find, uri_replace, d):
>          elif loc == 0:
>              # Principle of least surprise. We could end up with https matching against http and 
>              # generating "files://" urls if we use the regexp engine below.
> -            if i != uri_decoded[loc]:
> +            if not re.match(i, uri_decoded[loc]):
>                  return None
>              result_decoded[loc] = uri_replace_decoded[loc]
>          elif (re.match(i, uri_decoded[loc])):

I'm not picking on you (Enrico) here, there is a problem with my change
and we probably should revert my change in this area.

However the above patch fails "bitbake-selftest" and introduces a
regression. I wrote regression tests for the fetcher for a reason.

The trouble is its sees:

https://.*/.* file:///someplace/

but returns a url like files:// which the fetcher doesn't understand.
Suggestions on how to fix this are welcome.

Cheers,

Richard
Enrico Scholz June 28, 2012, 12:18 a.m.
Richard Purdie <richard.purdie@linuxfoundation.org> writes:

>>              # Principle of least surprise. We could end up with https matching against http and 
>>              # generating "files://" urls if we use the regexp engine below.
>> -            if i != uri_decoded[loc]:
>> +            if not re.match(i, uri_decoded[loc]):
>>                  return None
>>              result_decoded[loc] = uri_replace_decoded[loc]
>>          elif (re.match(i, uri_decoded[loc])):
>
> I'm not picking on you (Enrico) here, there is a problem with my change
> and we probably should revert my change in this area.
>
> However the above patch fails "bitbake-selftest" and introduces a
> regression.

selftest fails here in

| ERROR: test_gitfetch_premirror3 (bb.tests.fetch.FetcherTest)
| CalledProcessError: Command 'git clone git://git.openembedded.org/bitbake /tmp/tmpt_dDVD/sourcemirror.git 2> /dev/null' returned non-zero exit status 128

and

| FAIL: test_urilist2 (bb.tests.fetch.FetcherTest)
| AssertionError: Lists differ: ['file:///somepath/downloads/b... != ['file:///someotherpath/downlo...
| - ['file:///somepath/downloads/bitbake-1.0.tar.gz',
| -  'file:///someotherpath/downloads/bitbake-1.0.tar.gz']
| ? ^
| 
| + ['file:///someotherpath/downloads/bitbake-1.0.tar.gz']
| ? ^

(I am some days behind current bitbake master, so these might be fixed
in the meantime)


Nothing which seems to be related to the patch.


> The trouble is its sees:
>
> https://.*/.* file:///someplace/
>
> but returns a url like files:// which the fetcher doesn't understand.
> Suggestions on how to fix this are welcome.

I do not see how my patch can cause such a rewrite. The troublesome
're.sub()' is done in a branch for loc != 0 only and won't be applied on
the protocol part.


Enrico
Enrico Scholz June 28, 2012, 12:27 a.m.
Enrico Scholz <enrico.scholz@sigma-chemnitz.de> writes:

>>> -            if i != uri_decoded[loc]:
>>> +            if not re.match(i, uri_decoded[loc]):
>
> | FAIL: test_urilist2 (bb.tests.fetch.FetcherTest)
> | AssertionError: Lists differ: ['file:///somepath/downloads/b... != ['file:///someotherpath/downlo...
> | - ['file:///somepath/downloads/bitbake-1.0.tar.gz',
> | -  'file:///someotherpath/downloads/bitbake-1.0.tar.gz']
> | ? ^
> | 
> | + ['file:///someotherpath/downloads/bitbake-1.0.tar.gz']
> | ? ^
>
> Nothing which seems to be related to the patch.

I have to correct me... the new line should be

| +            if not re.match(i + '$', uri_decoded[loc]):

to match the complete protocol.



Enrico
Richard Purdie June 28, 2012, 11:42 a.m.
On Thu, 2012-06-28 at 02:27 +0200, Enrico Scholz wrote:
> Enrico Scholz <enrico.scholz@sigma-chemnitz.de> writes:
> 
> >>> -            if i != uri_decoded[loc]:
> >>> +            if not re.match(i, uri_decoded[loc]):
> >
> > | FAIL: test_urilist2 (bb.tests.fetch.FetcherTest)
> > | AssertionError: Lists differ: ['file:///somepath/downloads/b... != ['file:///someotherpath/downlo...
> > | - ['file:///somepath/downloads/bitbake-1.0.tar.gz',
> > | -  'file:///someotherpath/downloads/bitbake-1.0.tar.gz']
> > | ? ^
> > | 
> > | + ['file:///someotherpath/downloads/bitbake-1.0.tar.gz']
> > | ? ^
> >
> > Nothing which seems to be related to the patch.
> 
> I have to correct me... the new line should be
> 
> | +            if not re.match(i + '$', uri_decoded[loc]):
> 
> to match the complete protocol.

Agreed, this is going to make most sense. I've merged a patch which
reverts my original change and adds something like this (but checks the
pattern doesn't end with this already).

Hopefully this resolves the issues everyone was having.

Cheers,

Richard