diff mbox series

[v5,1/4] oe/qa.py: add check_uri_srcrev() helper for SCM URI SRCREV validation

Message ID 20260602062940.3107241-2-saisneha196@gmail.com
State Changes Requested
Headers show
Series Add missing SRCREV check for SCM URIs | expand

Commit Message

Sai Sneha June 2, 2026, 6:29 a.m. UTC 
Add a shared helper function check_uri_srcrev() that checks whether
a single SCM URI has a valid SRCREV set in the datastore.

The function uses unexpanded getVar() calls (False flag) to safely
inspect SRCREV without triggering AUTOREV expansion or live network
fetches. It mirrors BitBake's internal SRCREV resolution fallback
chain exactly:
  SRCREV_<name>:pn-<recipe>, SRCREV_<name>, SRCREV:pn-<recipe>, SRCREV

Returns:
  - The revision string if a valid SRCREV is found
  - '' for non-SCM URIs or URIs with inline rev= or tag= parameter (skip)
  - None if SRCREV is missing or INVALID

This shared helper is used by base.bbclass, insane.bbclass and
yocto-check-layer.bbclass to avoid duplicating the same logic.

Reported-by: Yoann Congal <yoann.congal@smile.fr>
Fixes: https://bugzilla.yoctoproject.org/show_bug.cgi?id=16051
AI-Generated: Developed with assistance from Anthropic Claude
Signed-off-by: Sai Sneha <saisneha196@gmail.com>
---

Changes in v5:
- Add tag= to skip condition alongside rev= to fix selftest breakage
  (test_git_unpack_nonetwork) reported by Mathieu Dubois-Briand
- Update docstring to mention tag= parameter

Changes in v4:
- New patch: factored common SRCREV checking logic into shared helper
- Eliminates code duplication across base.bbclass, insane.bbclass
  and yocto-check-layer.bbclass
- Uses for-else pattern, f-strings, simplified name lookup
- Adds docstring explaining return values

Changes in v3:
- Added AI-Generated disclosure and Reported-by tag

Changes in v2:
- Initial public submission
 meta/lib/oe/qa.py | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Comments

Yoann Congal June 2, 2026, 8:21 a.m. UTC | #1 
Hello,

On Tue Jun 2, 2026 at 8:29 AM CEST, Sai Sneha wrote:
> Add a shared helper function check_uri_srcrev() that checks whether
> a single SCM URI has a valid SRCREV set in the datastore.
>
> The function uses unexpanded getVar() calls (False flag) to safely
> inspect SRCREV without triggering AUTOREV expansion or live network
> fetches. It mirrors BitBake's internal SRCREV resolution fallback
> chain exactly:
>   SRCREV_<name>:pn-<recipe>, SRCREV_<name>, SRCREV:pn-<recipe>, SRCREV
That sounds wrong. Bitbake uses the *expanded* value:
lib/bb/fetch2/__init__.py:
1256         srcrev = d.getVar(a)

It needs the expansion to handle every possible overrides (for example,
MACHINE or DISTRO, but those are not the only one)

>
> Returns:
>   - The revision string if a valid SRCREV is found
>   - '' for non-SCM URIs or URIs with inline rev= or tag= parameter (skip)
>   - None if SRCREV is missing or INVALID
>
> This shared helper is used by base.bbclass, insane.bbclass and
> yocto-check-layer.bbclass to avoid duplicating the same logic.
>
> Reported-by: Yoann Congal <yoann.congal@smile.fr>
> Fixes: https://bugzilla.yoctoproject.org/show_bug.cgi?id=16051
> AI-Generated: Developed with assistance from Anthropic Claude
> Signed-off-by: Sai Sneha <saisneha196@gmail.com>
> ---
>
> Changes in v5:
> - Add tag= to skip condition alongside rev= to fix selftest breakage
>   (test_git_unpack_nonetwork) reported by Mathieu Dubois-Briand
> - Update docstring to mention tag= parameter
>
> Changes in v4:
> - New patch: factored common SRCREV checking logic into shared helper
> - Eliminates code duplication across base.bbclass, insane.bbclass
>   and yocto-check-layer.bbclass
> - Uses for-else pattern, f-strings, simplified name lookup
> - Adds docstring explaining return values
>
> Changes in v3:
> - Added AI-Generated disclosure and Reported-by tag
>
> Changes in v2:
> - Initial public submission
>  meta/lib/oe/qa.py | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/meta/lib/oe/qa.py b/meta/lib/oe/qa.py
> index cd36cb5070..0987aaf2c4 100644
> --- a/meta/lib/oe/qa.py
> +++ b/meta/lib/oe/qa.py
> @@ -240,6 +240,36 @@ def check_upstream_status(fullpath):
>              else:
>                  return "Missing Upstream-Status in patch\n%s\nPlease add according to %s ." % (fullpath, guidelines)
>  
> +def check_uri_srcrev(pn, uri, d):
> +    """
> +    Check that a single SCM URI has a valid SRCREV set.
> +
> +    Returns the resolved revision string if valid (including '${AUTOREV}').
> +    Returns '' for non-SCM URIs or URIs with inline rev= parameter (skip).
> +    Returns None if SRCREV is missing or INVALID.
> +    """
> +    import bb.fetch2
> +    try:
> +        (scheme, _, _, _, _, params) = bb.fetch2.decodeurl(uri)
> +    except Exception:
> +        return ''
> +    if scheme not in ('git', 'gitsm', 'hg', 'svn'):
> +        return ''
> +    if params.get('rev', '') or params.get('tag', ''):
> +        return ''

Sorry but I'm not sure "skipping the check for tag=... SRC_URI" is the right fix.

A SRC_URI with tag=... but no SRCREV is a situation we want to avoid in
OE-core so the check must remain enabled by default.
How about disabling the check in the test instead?

> +    name = params.get('name', 'default')
> +    candidates = [
> +        f'SRCREV_{name}:pn-{pn}',
> +        f'SRCREV_{name}',
> +        f'SRCREV:pn-{pn}',
> +        'SRCREV',
> +    ]
> +    for candidate in candidates:
> +        rev = d.getVar(candidate, False)

This code will miss many overrides (e.g MACHINE or DISTRO)

> +        if rev and rev != 'INVALID':
> +            return rev
> +    return None
> +
>  if __name__ == "__main__":
>      import sys
>
Paul Barker June 2, 2026, 12:16 p.m. UTC | #2 
On Tue, 2026-06-02 at 10:21 +0200, Yoann Congal via
lists.openembedded.org wrote:
> Hello,
> 
> On Tue Jun 2, 2026 at 8:29 AM CEST, Sai Sneha wrote:
> > Add a shared helper function check_uri_srcrev() that checks whether
> > a single SCM URI has a valid SRCREV set in the datastore.
> > 
> > The function uses unexpanded getVar() calls (False flag) to safely
> > inspect SRCREV without triggering AUTOREV expansion or live network
> > fetches. It mirrors BitBake's internal SRCREV resolution fallback
> > chain exactly:
> >   SRCREV_<name>:pn-<recipe>, SRCREV_<name>, SRCREV:pn-<recipe>, SRCREV
> That sounds wrong. Bitbake uses the *expanded* value:
> lib/bb/fetch2/__init__.py:
> 1256         srcrev = d.getVar(a)
> 
> It needs the expansion to handle every possible overrides (for example,
> MACHINE or DISTRO, but those are not the only one)

I don't think we should be handling overrides here at all, that's the
job of the bitbake parser.

Best regards,
Paul Barker June 2, 2026, 12:23 p.m. UTC | #3 
On Tue, 2026-06-02 at 11:59 +0530, Sai Sneha via lists.openembedded.org
wrote:
> Add a shared helper function check_uri_srcrev() that checks whether
> a single SCM URI has a valid SRCREV set in the datastore.
> 
> The function uses unexpanded getVar() calls (False flag) to safely
> inspect SRCREV without triggering AUTOREV expansion or live network
> fetches. It mirrors BitBake's internal SRCREV resolution fallback
> chain exactly:
>   SRCREV_<name>:pn-<recipe>, SRCREV_<name>, SRCREV:pn-<recipe>, SRCREV
> 
> Returns:
>   - The revision string if a valid SRCREV is found
>   - '' for non-SCM URIs or URIs with inline rev= or tag= parameter (skip)
>   - None if SRCREV is missing or INVALID
> 
> This shared helper is used by base.bbclass, insane.bbclass and
> yocto-check-layer.bbclass to avoid duplicating the same logic.
> 
> Reported-by: Yoann Congal <yoann.congal@smile.fr>
> Fixes: https://bugzilla.yoctoproject.org/show_bug.cgi?id=16051
> AI-Generated: Developed with assistance from Anthropic Claude
> Signed-off-by: Sai Sneha <saisneha196@gmail.com>
> ---
> 
> Changes in v5:
> - Add tag= to skip condition alongside rev= to fix selftest breakage
>   (test_git_unpack_nonetwork) reported by Mathieu Dubois-Briand
> - Update docstring to mention tag= parameter
> 
> Changes in v4:
> - New patch: factored common SRCREV checking logic into shared helper
> - Eliminates code duplication across base.bbclass, insane.bbclass
>   and yocto-check-layer.bbclass
> - Uses for-else pattern, f-strings, simplified name lookup
> - Adds docstring explaining return values
> 
> Changes in v3:
> - Added AI-Generated disclosure and Reported-by tag
> 
> Changes in v2:
> - Initial public submission
>  meta/lib/oe/qa.py | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/meta/lib/oe/qa.py b/meta/lib/oe/qa.py
> index cd36cb5070..0987aaf2c4 100644
> --- a/meta/lib/oe/qa.py
> +++ b/meta/lib/oe/qa.py
> @@ -240,6 +240,36 @@ def check_upstream_status(fullpath):
>              else:
>                  return "Missing Upstream-Status in patch\n%s\nPlease add according to %s ." % (fullpath, guidelines)
>  
> +def check_uri_srcrev(pn, uri, d):
> +    """
> +    Check that a single SCM URI has a valid SRCREV set.
> +
> +    Returns the resolved revision string if valid (including '${AUTOREV}').
> +    Returns '' for non-SCM URIs or URIs with inline rev= parameter (skip).
> +    Returns None if SRCREV is missing or INVALID.
> +    """
> +    import bb.fetch2
> +    try:
> +        (scheme, _, _, _, _, params) = bb.fetch2.decodeurl(uri)
> +    except Exception:
> +        return ''
> +    if scheme not in ('git', 'gitsm', 'hg', 'svn'):
> +        return ''

Each fetcher class has a supports_srcrev() method we can call instead of
trying to hard-code a list here.

> +    if params.get('rev', '') or params.get('tag', ''):
> +        return ''
> +    name = params.get('name', 'default')
> +    candidates = [
> +        f'SRCREV_{name}:pn-{pn}',
> +        f'SRCREV_{name}',
> +        f'SRCREV:pn-{pn}',
> +        'SRCREV',
> +    ]
> +    for candidate in candidates:
> +        rev = d.getVar(candidate, False)
> +        if rev and rev != 'INVALID':
> +            return rev
> +    return None

If there is no 'name' param, this code will look for SRCREV_default
before SRCREV, which is not correct. Off the top of my head, I think
this should be looking at `SRCREV_${name}` if name is set, else
`SRCREV`. You should check how the existing fetcher code determines the
SRCREV value to use.

Please also drop overrides handling as I mentioned in the other email I
just sent.
diff mbox series

Patch

diff --git a/meta/lib/oe/qa.py b/meta/lib/oe/qa.py
index cd36cb5070..0987aaf2c4 100644
--- a/meta/lib/oe/qa.py
+++ b/meta/lib/oe/qa.py
@@ -240,6 +240,36 @@  def check_upstream_status(fullpath):
             else:
                 return "Missing Upstream-Status in patch\n%s\nPlease add according to %s ." % (fullpath, guidelines)
 
+def check_uri_srcrev(pn, uri, d):
+    """
+    Check that a single SCM URI has a valid SRCREV set.
+
+    Returns the resolved revision string if valid (including '${AUTOREV}').
+    Returns '' for non-SCM URIs or URIs with inline rev= parameter (skip).
+    Returns None if SRCREV is missing or INVALID.
+    """
+    import bb.fetch2
+    try:
+        (scheme, _, _, _, _, params) = bb.fetch2.decodeurl(uri)
+    except Exception:
+        return ''
+    if scheme not in ('git', 'gitsm', 'hg', 'svn'):
+        return ''
+    if params.get('rev', '') or params.get('tag', ''):
+        return ''
+    name = params.get('name', 'default')
+    candidates = [
+        f'SRCREV_{name}:pn-{pn}',
+        f'SRCREV_{name}',
+        f'SRCREV:pn-{pn}',
+        'SRCREV',
+    ]
+    for candidate in candidates:
+        rev = d.getVar(candidate, False)
+        if rev and rev != 'INVALID':
+            return rev
+    return None
+
 if __name__ == "__main__":
     import sys