diff mbox series

[RFC,2/3] prserv: add "history" argument to "get-pr" request

Message ID 20240416171945.3799445-3-michael.opdenacker@bootlin.com
State New
Headers show
Series Call for help for prserv selftests | expand

Commit Message

Michael Opdenacker April 16, 2024, 5:19 p.m. UTC 
From: Michael Opdenacker <michael.opdenacker@bootlin.com>

This makes it possible to test the "history" mode in the
future bitbake selftests, to make sure that both "history"
and "no history" modes work as expected.

Signed-off-by: Michael Opdenacker <michael.opdenacker@bootlin.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Joshua Watt <JPEWhacker@gmail.com>
Cc: Tim Orling <ticotimo@gmail.com>
---
 lib/prserv/client.py |  4 ++--
 lib/prserv/db.py     | 16 ++++++++--------
 lib/prserv/serv.py   |  5 +++--
 3 files changed, 13 insertions(+), 12 deletions(-)

Comments

Joshua Watt April 20, 2024, 8:45 p.m. UTC | #1 
On Tue, Apr 16, 2024 at 12:20 PM <michael.opdenacker@bootlin.com> wrote:
>
> From: Michael Opdenacker <michael.opdenacker@bootlin.com>
>
> This makes it possible to test the "history" mode in the
> future bitbake selftests, to make sure that both "history"
> and "no history" modes work as expected.

Is this useful in normal day-to-day usage? I don't generally like to
add functionality just for testing if it can be helped, as the closer
you can get to actual end user API calls, the better it covers the end
user experience.

It looks like PRData has an (unused) argument to enable history mode?
If this is unused do we even actually care about it at all? Maybe it
would be better to remove an unused feature than try to test it?

>
> Signed-off-by: Michael Opdenacker <michael.opdenacker@bootlin.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Joshua Watt <JPEWhacker@gmail.com>
> Cc: Tim Orling <ticotimo@gmail.com>
> ---
>  lib/prserv/client.py |  4 ++--
>  lib/prserv/db.py     | 16 ++++++++--------
>  lib/prserv/serv.py   |  5 +++--
>  3 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/lib/prserv/client.py b/lib/prserv/client.py
> index 89760b6f74..389172f055 100644
> --- a/lib/prserv/client.py
> +++ b/lib/prserv/client.py
> @@ -14,9 +14,9 @@ class PRAsyncClient(bb.asyncrpc.AsyncClient):
>      def __init__(self):
>          super().__init__("PRSERVICE", "1.0", logger)
>
> -    async def getPR(self, version, pkgarch, checksum):
> +    async def getPR(self, version, pkgarch, checksum, history=False):
>          response = await self.invoke(
> -            {"get-pr": {"version": version, "pkgarch": pkgarch, "checksum": checksum}}
> +            {"get-pr": {"version": version, "pkgarch": pkgarch, "checksum": checksum, "history": history}}
>          )
>          if response:
>              return response["value"]
> diff --git a/lib/prserv/db.py b/lib/prserv/db.py
> index 8305238f7a..d41b781622 100644
> --- a/lib/prserv/db.py
> +++ b/lib/prserv/db.py
> @@ -163,7 +163,7 @@ class PRTable(object):
>
>          self.dirty = True
>
> -    def _get_value(self, version, pkgarch, checksum):
> +    def _get_value(self, version, pkgarch, checksum, history):
>
>          max_value = self.find_max_value(version, pkgarch)
>
> @@ -177,7 +177,11 @@ class PRTable(object):
>              # version, pkgarch found but not checksum. Create a new value from the maximum one
>              return increase_revision(max_value)
>
> -        if self.nohist:
> +        if history:
> +            # "history" mode: we found an existing value. We can return it
> +            # whether it's the maximum one or not.
> +            return value
> +        else:
>              # "no-history" mode: only return a value if that's the maximum one for
>              # the version and architecture, otherwise create a new one.
>              # This means that the value cannot decrement.
> @@ -185,13 +189,9 @@ class PRTable(object):
>                  return value
>              else:
>                  return increase_revision(max_value)
> -        else:
> -            # "hist" mode: we found an existing value. We can return it
> -            # whether it's the maximum one or not.
> -            return value
>
> -    def get_value(self, version, pkgarch, checksum):
> -        value = self._get_value(version, pkgarch, checksum)
> +    def get_value(self, version, pkgarch, checksum, history):
> +        value = self._get_value(version, pkgarch, checksum, history)
>          self.store_value(version, pkgarch, checksum, value)
>          return value
>
> diff --git a/lib/prserv/serv.py b/lib/prserv/serv.py
> index 9e07a34445..f692d050b8 100644
> --- a/lib/prserv/serv.py
> +++ b/lib/prserv/serv.py
> @@ -76,9 +76,10 @@ class PRServerClient(bb.asyncrpc.AsyncServerConnection):
>          version = request["version"]
>          pkgarch = request["pkgarch"]
>          checksum = request["checksum"]
> +        history = request["history"]
>
>          if self.upstream_client is None:
> -            value = self.server.table.get_value(version, pkgarch, checksum)
> +            value = self.server.table.get_value(version, pkgarch, checksum, history)
>              return {"value": value}
>
>          # We have an upstream server.
> @@ -105,7 +106,7 @@ class PRServerClient(bb.asyncrpc.AsyncServerConnection):
>              # The package is not known upstream, must be a local-only package
>              # Let's compute the PR number using the local-only method
>
> -            value = self.server.table.get_value(version, pkgarch, checksum)
> +            value = self.server.table.get_value(version, pkgarch, checksum, history)
>              return {"value": value}
>
>          # The package is known upstream, let's ask the upstream server
> --
> 2.34.1
>
Michael Opdenacker April 23, 2024, 12:26 p.m. UTC | #2 
Hi Joshua

On 4/20/24 at 22:45, Joshua Watt wrote:
> On Tue, Apr 16, 2024 at 12:20 PM <michael.opdenacker@bootlin.com> wrote:
>> From: Michael Opdenacker <michael.opdenacker@bootlin.com>
>>
>> This makes it possible to test the "history" mode in the
>> future bitbake selftests, to make sure that both "history"
>> and "no history" modes work as expected.
> Is this useful in normal day-to-day usage? I don't generally like to
> add functionality just for testing if it can be helped, as the closer
> you can get to actual end user API calls, the better it covers the end
> user experience.
>
> It looks like PRData has an (unused) argument to enable history mode?
> If this is unused do we even actually care about it at all? Maybe it
> would be better to remove an unused feature than try to test it?

One version of my series actually removed that feature, but then Richard 
asked me to keep the code, because of the usefulness of the "history" 
mode for binary reproducibility. This way, we always generate the same 
package and revision if we have the same output hash as in a prior 
build. That's different from the default behavior that increases the 
revision every time we have a new output hash.

This being said, supporting both modes added substantial complexity to 
the code, and I wish I had more time to consolidate all this and reduce 
as much redundancy as possible between modes.

Cheers
Michael.
Joshua Watt April 23, 2024, 3:02 p.m. UTC | #3 
On Tue, Apr 23, 2024 at 6:26 AM Michael Opdenacker
<michael.opdenacker@bootlin.com> wrote:
>
> Hi Joshua
>
> On 4/20/24 at 22:45, Joshua Watt wrote:
> > On Tue, Apr 16, 2024 at 12:20 PM <michael.opdenacker@bootlin.com> wrote:
> >> From: Michael Opdenacker <michael.opdenacker@bootlin.com>
> >>
> >> This makes it possible to test the "history" mode in the
> >> future bitbake selftests, to make sure that both "history"
> >> and "no history" modes work as expected.
> > Is this useful in normal day-to-day usage? I don't generally like to
> > add functionality just for testing if it can be helped, as the closer
> > you can get to actual end user API calls, the better it covers the end
> > user experience.
> >
> > It looks like PRData has an (unused) argument to enable history mode?
> > If this is unused do we even actually care about it at all? Maybe it
> > would be better to remove an unused feature than try to test it?
>
> One version of my series actually removed that feature, but then Richard
> asked me to keep the code, because of the usefulness of the "history"
> mode for binary reproducibility. This way, we always generate the same
> package and revision if we have the same output hash as in a prior
> build. That's different from the default behavior that increases the
> revision every time we have a new output hash.

Ah, that's a different story. API used by end users (but not by
bitbake during normal execution) is fine, and a different story than
API added just for testing :)

>
> This being said, supporting both modes added substantial complexity to
> the code, and I wish I had more time to consolidate all this and reduce
> as much redundancy as possible between modes.
>
> Cheers
> Michael.
>
> --
> Michael Opdenacker, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
diff mbox series

Patch

diff --git a/lib/prserv/client.py b/lib/prserv/client.py
index 89760b6f74..389172f055 100644
--- a/lib/prserv/client.py
+++ b/lib/prserv/client.py
@@ -14,9 +14,9 @@  class PRAsyncClient(bb.asyncrpc.AsyncClient):
     def __init__(self):
         super().__init__("PRSERVICE", "1.0", logger)
 
-    async def getPR(self, version, pkgarch, checksum):
+    async def getPR(self, version, pkgarch, checksum, history=False):
         response = await self.invoke(
-            {"get-pr": {"version": version, "pkgarch": pkgarch, "checksum": checksum}}
+            {"get-pr": {"version": version, "pkgarch": pkgarch, "checksum": checksum, "history": history}}
         )
         if response:
             return response["value"]
diff --git a/lib/prserv/db.py b/lib/prserv/db.py
index 8305238f7a..d41b781622 100644
--- a/lib/prserv/db.py
+++ b/lib/prserv/db.py
@@ -163,7 +163,7 @@  class PRTable(object):
 
         self.dirty = True
 
-    def _get_value(self, version, pkgarch, checksum):
+    def _get_value(self, version, pkgarch, checksum, history):
 
         max_value = self.find_max_value(version, pkgarch)
 
@@ -177,7 +177,11 @@  class PRTable(object):
             # version, pkgarch found but not checksum. Create a new value from the maximum one
             return increase_revision(max_value)
 
-        if self.nohist:
+        if history:
+            # "history" mode: we found an existing value. We can return it
+            # whether it's the maximum one or not.
+            return value
+        else:
             # "no-history" mode: only return a value if that's the maximum one for
             # the version and architecture, otherwise create a new one.
             # This means that the value cannot decrement.
@@ -185,13 +189,9 @@  class PRTable(object):
                 return value
             else:
                 return increase_revision(max_value)
-        else:
-            # "hist" mode: we found an existing value. We can return it
-            # whether it's the maximum one or not.
-            return value
 
-    def get_value(self, version, pkgarch, checksum):
-        value = self._get_value(version, pkgarch, checksum)
+    def get_value(self, version, pkgarch, checksum, history):
+        value = self._get_value(version, pkgarch, checksum, history)
         self.store_value(version, pkgarch, checksum, value)
         return value
 
diff --git a/lib/prserv/serv.py b/lib/prserv/serv.py
index 9e07a34445..f692d050b8 100644
--- a/lib/prserv/serv.py
+++ b/lib/prserv/serv.py
@@ -76,9 +76,10 @@  class PRServerClient(bb.asyncrpc.AsyncServerConnection):
         version = request["version"]
         pkgarch = request["pkgarch"]
         checksum = request["checksum"]
+        history = request["history"]
 
         if self.upstream_client is None:
-            value = self.server.table.get_value(version, pkgarch, checksum)
+            value = self.server.table.get_value(version, pkgarch, checksum, history)
             return {"value": value}
 
         # We have an upstream server.
@@ -105,7 +106,7 @@  class PRServerClient(bb.asyncrpc.AsyncServerConnection):
             # The package is not known upstream, must be a local-only package
             # Let's compute the PR number using the local-only method
 
-            value = self.server.table.get_value(version, pkgarch, checksum)
+            value = self.server.table.get_value(version, pkgarch, checksum, history)
             return {"value": value}
 
         # The package is known upstream, let's ask the upstream server