Message ID | 20240416171945.3799445-3-michael.opdenacker@bootlin.com |
---|---|
State | New |
Headers | show |
Series | Call for help for prserv selftests | expand |
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 >
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.
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 --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