Re: [bitbake-devel] [PATCH 00/12] prserv: add support for an "upstream" server

Re: [bitbake-devel] [PATCH 00/12] prserv: add support for an "upstream" server

[Michael Opdenacker]

On 3/29/24 at 15:39, Michael Opdenacker via lists.openembedded.org wrote:
> From: Michael Opdenacker <michael.opdenacker@bootlin.com>
>
> This makes it possible to customize an "upstream" distribution
> by modifying local packages. If the "upstream" package bears
> revision "x", the local one will have revision "x.y", this
> having priority over the upstream one.
>
> Take advantage of this work to clean-up and update the prserv code too.
>
> Michael Opdenacker (12):
>    prserv: simplify the PRServerClient() interface
>    prserv: use double quotes by default
>    bitbake-prserv: replace deprecated optparse by argparse
>    prserv: use self.logger instead of logger directly
>    asyncrpc: include parse_address from hashserv
>    prserv: capitalization and spacing improvements
>    prserv: fix read_only test
>    prserv: add extra requests
>    prserv: remove redundant exception handler
>    prserv: correct error message
>    prserv: remove unnecessary code
>    prserv: add "upstream" server support
>
>   bin/bitbake-prserv        |  99 ++++++++++++-----
>   lib/bb/asyncrpc/client.py |  23 ++++
>   lib/hashserv/__init__.py  |  27 +----
>   lib/prserv/__init__.py    |  21 +++-
>   lib/prserv/client.py      |  42 +++++--
>   lib/prserv/db.py          | 176 ++++++++++++++++++++---------
>   lib/prserv/serv.py        | 228 ++++++++++++++++++++++++++++----------
>   7 files changed, 434 insertions(+), 182 deletions(-)


Note that this code has a remaining issue that appears sometimes.

Even though the revision stored in the database was indeed "0.3", I 
eventually got this package name:
tmp/deploy/ipk/core2-64/hello-dev_1.0-r0.0.30000000000000004_core2-64.ipk

This may be an issue in OE-core, in the way the "0.3" value can be 
treated by default as a float by Python3, and then converted into an 
incorrect string when creating the output package name. Investigating, 
but if you already know where this happens, I'm interested!

Cheers
Michael.

-- 
Michael Opdenacker, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [bitbake-devel] [PATCH 00/12] prserv: add support for an "upstream" server

[Bruce Ashfield]

[-- Attachment #1: Type: text/plain, Size: 4732 bytes --]

On Fri, Apr 12, 2024 at 5:02 AM Michael Opdenacker via
lists.openembedded.org <michael.opdenacker=
bootlin.com@lists.openembedded.org> wrote:

> From: Michael Opdenacker <michael.opdenacker@bootlin.com>
>
> This makes it possible to customize an "upstream" distribution
> by modifying local packages. If the "upstream" package bears
> revision "x", the local one will have revision "x.y", this
> having priority over the upstream one.
>
> Multiple levels of upstream servers are supported, so "x.y.z" revisions
> are possible too.
>
> Take advantage of this work to clean-up and update the prserv code too.
>
> Manual "sanity" tests have been run so far but automated tests
> (based on Python unittest, as for the hash server tests)
> are being prepared for the next iteration. They will also to
> test also supported scenarios.
>
> Code reviews are welcome, especially for more Pythonic
> ways of implementing some details.
>
>
I went through the code and commented on a few of the
larger patches.

I can't say that I'm an expert in the existing PR server and
I don't tend to write overly pythonic code .. but I did look
through things and can add:

Reviewed-by: Bruce Ashfield <bruce.ashfield@gmail.com>




> ---
>
> Changes in V3:
>
> - Revert the commit removing the so far unused "hist" mode, which
>   we wish to keep for binary reproducibility sake.
>
> - Simplification of get_value() function to take
>   both "hist" and "nohist" modes with the same shared code.
>
> - Add "history" parameter to the "getPR" request,
>   so that the client can ask for the mode of its choice.
>   This will also make it possible to implement tests
>   for both modes.
>
>   Note that more requests ("export", "import"...)
>   will also need a "history" parameter, in a future version,
>   after the first tests are implemented.
>
> - Several fixes to bugs either present in V2 or introduced
>   by the V3 code changes.
>
> - Put all the new features at the tip of the branch,
>   to make the cleanup commits easier to merged.
>
> Changes in V2:
>
> - Add this new commit:
>   prserv: remove unused "hist" mode in the database backend
>
> - Squash commit "prserv: fix read_only test" into
>   commit "prserv: simplify the PRServerClient() interface"
>   (Reported by Richard Purdie)
>
> - Fix the code to support increasing "x.y.z" values, thus
>   supporting several levels of upstream servers.
>
> - db.py: remove duplicate definition of find_max_value() function in db.py
>
> - prserv.py: remove tabs before comments (Python didn't complain)
>
> - db.py: now stores the revision ("value") as TEXT.
>   This way we can store "1.0" without having it transformed to "1"
>   when the default type was INTEGER.
>
> - This allows to fix a regression when the first packages were created
>   with 'r0.1' instead of 'r0.0' initially.
>
> - find_max_value: now returns None instead of '0' when no value is found
>   Before we couldn't tell the difference between a '0'
>   max value and the absence of such a value.
>
> Michael Opdenacker (12):
>   prserv: simplify the PRServerClient() interface
>   prserv: use double quotes by default
>   bitbake-prserv: replace deprecated optparse by argparse
>   prserv: use self.logger instead of logger directly
>   asyncrpc: include parse_address from hashserv
>   prserv: capitalization and spacing improvements
>   prserv: add extra requests
>   prserv: remove redundant exception handler
>   prserv: correct error message
>   prserv: remove unnecessary code
>   prserv: add "upstream" server support
>   prserv: add "history" argument to "get-pr" request
>
>  bin/bitbake-prserv        |  99 ++++++++++++-----
>  lib/bb/asyncrpc/client.py |  23 ++++
>  lib/hashserv/__init__.py  |  27 +----
>  lib/prserv/__init__.py    |  21 +++-
>  lib/prserv/client.py      |  44 ++++++--
>  lib/prserv/db.py          | 225 ++++++++++++++++++++++----------------
>  lib/prserv/serv.py        | 220 ++++++++++++++++++++++++++-----------
>  7 files changed, 436 insertions(+), 223 deletions(-)
>
> --
> 2.34.1
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#16075):
> https://lists.openembedded.org/g/bitbake-devel/message/16075
> Mute This Topic: https://lists.openembedded.org/mt/105479088/1050810
> Group Owner: bitbake-devel+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [
> bruce.ashfield@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>

-- 
- Thou shalt not follow the NULL pointer, for chaos and madness await thee
at its end
- "Use the force Harry" - Gandalf, Star Trek II

[-- Attachment #2: Type: text/html, Size: 7126 bytes --]

Re: [bitbake-devel] [PATCH 00/12] prserv: add support for an "upstream" server

[Michael Opdenacker]

Greetings,

On 4/12/24 at 11:02, Michael Opdenacker via lists.openembedded.org wrote:
> From: Michael Opdenacker <michael.opdenacker@bootlin.com>
>
> This makes it possible to customize an "upstream" distribution
> by modifying local packages. If the "upstream" package bears
> revision "x", the local one will have revision "x.y", this
> having priority over the upstream one.
>
> Multiple levels of upstream servers are supported, so "x.y.z" revisions
> are possible too.
>
> Take advantage of this work to clean-up and update the prserv code too.
>
> Manual "sanity" tests have been run so far but automated tests
> (based on Python unittest, as for the hash server tests)
> are being prepared for the next iteration. They will also to
> test also supported scenarios.
>
> Code reviews are welcome, especially for more Pythonic
> ways of implementing some details.
>
> ---
>
> Changes in V3:
>
> - Revert the commit removing the so far unused "hist" mode, which
>    we wish to keep for binary reproducibility sake.
>
> - Simplification of get_value() function to take
>    both "hist" and "nohist" modes with the same shared code.
>
> - Add "history" parameter to the "getPR" request,
>    so that the client can ask for the mode of its choice.
>    This will also make it possible to implement tests
>    for both modes.
>
>    Note that more requests ("export", "import"...)
>    will also need a "history" parameter, in a future version,
>    after the first tests are implemented.
>
> - Several fixes to bugs either present in V2 or introduced
>    by the V3 code changes.
>
> - Put all the new features at the tip of the branch,
>    to make the cleanup commits easier to merged.


This is obviously a V3, even if I forgot to add the "v3" prefix :-I

Michael.

-- 
Michael Opdenacker, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [bitbake-devel] [PATCH 00/12] prserv: add support for an "upstream" server

[Richard Purdie]

On Wed, 2024-04-03 at 17:33 +0200, Michael Opdenacker wrote:
> Hi Richard,
> 
> Many thanks for the first review!
> 
> On 4/3/24 at 17:09, Richard Purdie wrote:
> > On Fri, 2024-03-29 at 15:39 +0100, Michael Opdenacker via
> > lists.openembedded.org wrote:
> > > From: Michael Opdenacker <michael.opdenacker@bootlin.com>
> > > 
> > > This makes it possible to customize an "upstream" distribution
> > > by modifying local packages. If the "upstream" package bears
> > > revision "x", the local one will have revision "x.y", this
> > > having priority over the upstream one.
> > > 
> > > Take advantage of this work to clean-up and update the prserv code
> > > too.
> > > 
> > > Michael Opdenacker (12):
> > >    prserv: simplify the PRServerClient() interface
> > >    prserv: use double quotes by default
> > >    bitbake-prserv: replace deprecated optparse by argparse
> > >    prserv: use self.logger instead of logger directly
> > >    asyncrpc: include parse_address from hashserv
> > >    prserv: capitalization and spacing improvements
> > >    prserv: fix read_only test
> > >    prserv: add extra requests
> > >    prserv: remove redundant exception handler
> > >    prserv: correct error message
> > >    prserv: remove unnecessary code
> > >    prserv: add "upstream" server support
> > Most of these series looks like cleanups to make prserv look more like
> > hashserv, which is fine.
> > 
> > The piece I'm struggling to understand is what happens if we have three
> > prservs where A has B an upstream and B has C as an upstream. Does the
> > system cope with multiple levels of increment, x.y.z and so on?
> 
> 
> I haven't tested that yet, but wasn't sure we wanted to support such as 
> multi-level scenario. If you confirm that's within the scope, I can try 
> to make this work!

I believe we will need to support a potential hierarchy of these
servers in the same way hash equivalence can.

> > I'm a bit worried we're seeing floats in the results, which suggests
> > that generic x.y.z wouldn't work?
> 
> 
> I first tried to explicitly convert all the responses from the PR server 
> to strings, but that didn't help. Could it be that a return value like 
> '0.1' could be automatically interpreted as a float if it can be 
> converted to one? There is no explicit conversion to float in the PR 
> server code, so it seemed to me that the conversion was done automatically.

I suspect that it is SQL that is making these floats and you're just
passing the results around from there. The trouble is that whilst X+0.1
will work for x.y, it isn't going to work for x.y.z and beyond. That is
one reason I'm mentioning this now, at same time as we're seening
issues with the float() values.

> > Also, where do we stand on automated tests for this code?
> 
> 
> Not done yet. I was waiting for the reviews on the first series, but I 
> understand that tests will be necessary :)

I would have a bit more confidence if there were some test cases
showing what was working!

Cheers,

Richard

Re: [bitbake-devel] [PATCH 00/12] prserv: add support for an "upstream" server

[Michael Opdenacker]

Hi Richard,

Many thanks for the first review!

On 4/3/24 at 17:09, Richard Purdie wrote:
> On Fri, 2024-03-29 at 15:39 +0100, Michael Opdenacker via
> lists.openembedded.org wrote:
>> From: Michael Opdenacker <michael.opdenacker@bootlin.com>
>>
>> This makes it possible to customize an "upstream" distribution
>> by modifying local packages. If the "upstream" package bears
>> revision "x", the local one will have revision "x.y", this
>> having priority over the upstream one.
>>
>> Take advantage of this work to clean-up and update the prserv code
>> too.
>>
>> Michael Opdenacker (12):
>>    prserv: simplify the PRServerClient() interface
>>    prserv: use double quotes by default
>>    bitbake-prserv: replace deprecated optparse by argparse
>>    prserv: use self.logger instead of logger directly
>>    asyncrpc: include parse_address from hashserv
>>    prserv: capitalization and spacing improvements
>>    prserv: fix read_only test
>>    prserv: add extra requests
>>    prserv: remove redundant exception handler
>>    prserv: correct error message
>>    prserv: remove unnecessary code
>>    prserv: add "upstream" server support
> Most of these series looks like cleanups to make prserv look more like
> hashserv, which is fine.
>
> The piece I'm struggling to understand is what happens if we have three
> prservs where A has B an upstream and B has C as an upstream. Does the
> system cope with multiple levels of increment, x.y.z and so on?


I haven't tested that yet, but wasn't sure we wanted to support such as 
multi-level scenario. If you confirm that's within the scope, I can try 
to make this work!

>
> I'm a bit worried we're seeing floats in the results, which suggests
> that generic x.y.z wouldn't work?


I first tried to explicitly convert all the responses from the PR server 
to strings, but that didn't help. Could it be that a return value like 
'0.1' could be automatically interpreted as a float if it can be 
converted to one? There is no explicit conversion to float in the PR 
server code, so it seemed to me that the conversion was done automatically.

>
> Also, where do we stand on automated tests for this code?


Not done yet. I was waiting for the reviews on the first series, but I 
understand that tests will be necessary :)
Cheers
Michael.

-- 
Michael Opdenacker, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [bitbake-devel] [PATCH 00/12] prserv: add support for an "upstream" server

[Richard Purdie]

On Fri, 2024-03-29 at 15:39 +0100, Michael Opdenacker via
lists.openembedded.org wrote:
> From: Michael Opdenacker <michael.opdenacker@bootlin.com>
> 
> This makes it possible to customize an "upstream" distribution
> by modifying local packages. If the "upstream" package bears
> revision "x", the local one will have revision "x.y", this
> having priority over the upstream one.
> 
> Take advantage of this work to clean-up and update the prserv code
> too.
> 
> Michael Opdenacker (12):
>   prserv: simplify the PRServerClient() interface
>   prserv: use double quotes by default
>   bitbake-prserv: replace deprecated optparse by argparse
>   prserv: use self.logger instead of logger directly
>   asyncrpc: include parse_address from hashserv
>   prserv: capitalization and spacing improvements
>   prserv: fix read_only test
>   prserv: add extra requests
>   prserv: remove redundant exception handler
>   prserv: correct error message
>   prserv: remove unnecessary code
>   prserv: add "upstream" server support

Most of these series looks like cleanups to make prserv look more like
hashserv, which is fine.

The piece I'm struggling to understand is what happens if we have three
prservs where A has B an upstream and B has C as an upstream. Does the
system cope with multiple levels of increment, x.y.z and so on?

I'm a bit worried we're seeing floats in the results, which suggests
that generic x.y.z wouldn't work?

Also, where do we stand on automated tests for this code?

Cheers,

Richard