On Tue, 2023-08-15 at 21:27 +0000, Peter Kjellerstedt wrote:
> > -----Original Message-----
> > From: [email protected]
> > <[email protected]> On Behalf Of
> > Richard Purdie
> > Sent: den 10 augusti 2023 18:53
> > To: openembedded-architecture
> > <[email protected]>
> > Subject: Re: [Openembedded-architecture] Workflow improvements -
> > incremental source updates?
> >
> > On Tue, 2023-08-08 at 10:10 +0100, Richard Purdie via
> > lists.openembedded.org wrote:
> > >
>
> I decided to give the related changes you have in master-next a try. I
> cherry-picked the following three commits:
>
> bitbake: fetch2: Add new srcrev fetcher API
> recipes/classes/scripts: Drop SRCPV usage in OE-Core
> base/package: Move source revision information from PV to PKGV
>
> I am of course aware that the above changes are not final, but I have
> some concerns.
Thanks, I'm happy someone else is looking at this and you raise some
issues I hadn't thought about as I've been too busy trying to even get
the test suites and devtool to work correctly with the changes.
>
> I tested building "iso-codes" and "xcursor-transparent-theme". They are
> both allarch recipes with few dependencies. The former has a SRCREV that
> matches PV, and the latter does not (and thus uses PV = "0.1.1+git${SRCPV}").
>
> Here are the names of the resulting RPM packages before the changes are
> applied:
>
> iso-codes-4.15.0-r0.4.noarch.rpm
> xcursor-transparent-theme-0.1.1+git0+23c8af5ba4-r0.0.noarch.rpm
>
> And here they are afterwards:
>
> iso-codes-4.15.0AUTOINC+69ba16daef-r0.0.noarch.rpm
> xcursor-transparent-theme-0.1.1+gitAUTOINC+23c8af5ba4-r0.0.noarch.rpm
I've definitely been focusing on the latter, to the expense of the
former.
> The first problem is of course that AUTOINC is not replaced as intended
> when a PR server is active. This is due to the code in package_get_auto_pr()
> that should update PRSERV_PV_AUTOINC not being triggered and thus it
> retains its default value "AUTOINC". The code isn't triggered because it
> looks for "AUTOINC" in PKGV, but it isn't there (yet) since ${SRCPV} is
> no longer used. The following patch solves it:
>
> diff --git a/meta/classes-global/package.bbclass
> b/meta/classes-global/package.bbclass
> index 7f55b123c4..bf824b0b83 100644
> --- a/meta/classes-global/package.bbclass
> +++ b/meta/classes-global/package.bbclass
> @@ -266,6 +266,9 @@ python package_get_auto_pr() {
> d.setVar("PRSERV_HOST", host)
>
> pkgv = d.getVar("PKGV")
> + srcpv = bb.fetch.get_pkgv_string(d)
> + if srcpv:
> + pkgv += srcpv
>
> # PR Server not active, handle AUTOINC
> if not d.getVar('PRSERV_HOST'):
Thanks, I'll have to look into the ordering of this. The original code
changes were in this function but that didn't work for other reasons.
> The second problem is that the value from bb.fetch.get_pkgv_string() is
> concatenated to the version without any separator. So if the AUTOINC
> part had been replaced as expected, the RPM name for the iso-codes
> package would have become iso-codes-4.15.00+69ba16daef-r0.0.noarch.rpm,
> which I doubt was intended... The easiest solution should be to make
> get_pkgv_string() return "+AUTOINC+<ID>" instead of the current
> "AUTOINC+<ID>".
That would then add a separator to strings where there currently isn't
one. The intent wasn't to add to fixed versions but I've over
simplified one of the patches.
> Third, I am not sure if it is a good idea to always include the SHA-1
> in the package name. Before it was typically only done when the SRCREV
> did not match PV. For most packages it is redundant. It also means that
> the PR server needs to keep track of the version for all Git recipes,
> not just the ones where ${SRCPV} is used today. However, I understand
> that it is probably not easily solvable to automatically detect when
> it is needed and when it is not. So it is either do it manually as
> today and risk missing to do it when it is needed, or always do it and
> have it be redundant in most cases.
I think we can probably come up with a heuristic like a "+" in PV
triggers the addition which should work in most cases.
> One thing that I would prefer is if there is a variable, similar to
> the old SRCPV, that contains "${@bb.fetch.get_pkgv_string(d)}" by
> default and is used from package_convert_pr_autoinc() instead of
> calling the method directly. The reason for that is to make it
> possible to use a different method to determine the value. E.g., in
> our case we have a variable called MAINTPV, which is similar to
> SRCPV. The reason for it is that it handles Gerrit references and
> expands them in a more suitable way than just a SHA-1. Thus I would
> like to be able to use its underlying function instead of
> bb.fetch.get_pkgv_string(d) to generate the string for PKGV.
I'm torn on that. I understand why you might want/need to, equally,
we're trying to get away from manual and hence error prone usage.
Putting this in place brings us back to that so I'm reluctant.
FWIW the patches clearly aren't ready to merge in their current form
though. Thanks again for the review, it is helpful and I'm a bit too
close to some pieces of it to see some of the issues.
Cheers,
Richard
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#1735):
https://lists.openembedded.org/g/openembedded-architecture/message/1735
Mute This Topic: https://lists.openembedded.org/mt/100618420/21656
Group Owner: [email protected]
Unsubscribe: https://lists.openembedded.org/g/openembedded-architecture/unsub
[[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
