[1/3] Makefile/set_versions: Allow poky.yaml to be autogenerated

Use a script to generate the branch/tag information inside poky.yaml.

If the branch isn't a known release branch, include git magic to find
the closest matching release branch we know about.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 documentation/.gitignore                  |   1 +
 documentation/Makefile                    |   1 +
 documentation/{poky.yaml => poky.yaml.in} |   0
 documentation/set_versions.py             | 115 ++++++++++++++++++++++
 4 files changed, 117 insertions(+)
 rename documentation/{poky.yaml => poky.yaml.in} (100%)
 create mode 100755 documentation/set_versions.py

Hi Richard,

On 3/18/22 16:42, Richard Purdie wrote:
> Use a script to generate the branch/tag information inside poky.yaml.
> 
> If the branch isn't a known release branch, include git magic to find
> the closest matching release branch we know about.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>   documentation/.gitignore                  |   1 +
>   documentation/Makefile                    |   1 +
>   documentation/{poky.yaml => poky.yaml.in} |   0
>   documentation/set_versions.py             | 115 ++++++++++++++++++++++
>   4 files changed, 117 insertions(+)
>   rename documentation/{poky.yaml => poky.yaml.in} (100%)
>   create mode 100755 documentation/set_versions.py
> 
> diff --git a/documentation/.gitignore b/documentation/.gitignore
> index 35ead8af6..e5e2c1708 100644
> --- a/documentation/.gitignore
> +++ b/documentation/.gitignore
> @@ -1,5 +1,6 @@
>   _build/
>   Pipfile.lock
> +poky.yaml
>   .vscode/
>   */svg/*.png
>   */svg/*.pdf
> diff --git a/documentation/Makefile b/documentation/Makefile
> index f04f381bd..bec53399c 100644
> --- a/documentation/Makefile
> +++ b/documentation/Makefile
> @@ -57,4 +57,5 @@ all: html epub latexpdf
>   # Catch-all target: route all unknown targets to Sphinx using the new
>   # "make mode" option.  $(O) is meant as a shortcut for $(SPHINXOPTS).
>   %:
> +	$(SOURCEDIR)/set_versions.py
>   	@$(SPHINXBUILD) -M $@ "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O)
> diff --git a/documentation/poky.yaml b/documentation/poky.yaml.in
> similarity index 100%
> rename from documentation/poky.yaml
> rename to documentation/poky.yaml.in
> diff --git a/documentation/set_versions.py b/documentation/set_versions.py
> new file mode 100755
> index 000000000..266ccf464
> --- /dev/null
> +++ b/documentation/set_versions.py
> @@ -0,0 +1,115 @@
> +#!/usr/bin/env python3
> +#
> +# Add version information to poky.yaml based upon current git branch/tags
> +#
> +# Copyright Linux Foundation
> +# Author: Richard Purdie <richard.purdie@linuxfoundation.org>
> +#
> +# SPDX-License-Identifier: MIT
> +#
> +
> +
> +import subprocess
> +import collections
> +import sys
> +
> +devbranch = "kirkstone"
> +#devbranch = "langdale"
> +ltsseries = ["kirkstone", "dunfell"]
> +
> +release_series = collections.OrderedDict()
> +#release_series["langdale"] = "4.1"
> +release_series["kirkstone"] = "4.0"
> +release_series["honister"] = "3.4"
> +release_series["hardknott"] = "3.3"
> +release_series["gatesgarth"] = "3.2"
> +release_series["dunfell"] = "3.1"
> +
> +ourversion = None
> +ourseries = None
> +ourbranch = None
> +
> +# Test tags exist and inform the user to fetch if not
> +try:
> +    subprocess.run(["git", "show", "yocto-3.4.2"], capture_output=True, check=True)
> +except subprocess.CalledProcessError:
> +    sys.exit("Please run 'git fetch --tags' before building the documentation")
> +
> +# Try and figure out what we are
> +tags = subprocess.run(["git", "tag", "--points-at", "HEAD"], capture_output=True, text=True).stdout
> +for t in tags.split():
> +    if t.startswith("yocto-"):
> +        ourversion = t[6:]
> +
> +if ourversion:
> +    # We're a tagged release
> +    components = ourversion.split(".")
> +    baseversion = components[0] + "." + components[1]
> +    for i in release_series:
> +        if release_series[i] == baseversion:
> +            ourseries = i
> +            ourbranch = i
> +else:
> +    # We're floating on a branch
> +    branch = subprocess.run(["git", "branch", "--show-current"], capture_output=True, text=True).stdout.strip()

Are we safe here if we're not on a branch? e.g. a specific commit?

> +    ourbranch = branch
> +    if branch != "master" and branch not in release_series:
> +        possible_branches = []
> +        for b in release_series.keys():
> +            result = subprocess.run(["git", "show-ref", "heads/" + b], capture_output=True, text=True)
> +            if result.returncode == 0:
> +                possible_branches.append(b)
> +                continue
> +            result = subprocess.run(["git", "show-ref", "origin/" + b], capture_output=True, text=True)

Not everybody has the upstream remote named origin (I don't :) ).

What about using [git ,branch ,--remote, --list, '*/' + b] ? Note that 
the return code is always 0 even if nothing's found.

Just need to add all matches if one is found.

Also, could use:

git branch --all --list b --list '*/'+b

to replace the two subprocesses since it returns both local and remote 
branches.

> +            if result.returncode == 0:
> +                possible_branches.append("origin/" + b)
> +        nearestbranch = subprocess.run('git show-branch master ' + ' '.join(possible_branches) + ' | grep "*" | grep -v "$(git rev-parse --abbrev-ref HEAD)" | head -n1', shell=True, capture_output=True, text=True).stdout

Since yuou're using shell=True, I would at least pass the possible 
branch names each through shlex.quote.

I also find this command very complex. What I could suggest instead 
would be to iterate over git rev-list --count master..branch and find 
the one that is the closest this way?

Also not sure about the grep "*", are you really looking for lines with 
a * character in it?

> +        branch = nearestbranch.split('[')[1].split('~')[0]
> +        print("Nearest release branch esimtated to be %s" % branch)
> +    if branch == "master":
> +        ourseries = devbranch

This is a risky assumption if the user hasn't pulled upstream in a long 
time, but I guess there isn't really anything to do about it :)

> +    elif branch in release_series:
> +        ourseries = branch
> +    else:
> +        sys.exit("Unknown series for branch %s" % branch)
> +
> +    previoustags = subprocess.run(["git", "tag", "--merged", "HEAD"], capture_output=True, text=True).stdout
> +    previoustags = [t[6:] for t in previoustags.split() if t.startswith("yocto-" + release_series[ourseries])]
> +    futuretags = subprocess.run(["git", "tag", "--merged", ourbranch], capture_output=True, text=True).stdout
> +    futuretags = [t[6:] for t in futuretags.split() if t.startswith("yocto-" + release_series[ourseries])]
> +

I don't understand why "ourbranch" is used here, isn't it technically 
the same as HEAD since it's the result of git branch --show-current of HEAD?

Did you mean "branch" instead since it could be either the returned 
value of git branch --show-current OR nearestbranch?

> +    # Append .999 against the last known version
> +    if len(previoustags) != len(futuretags):
> +        ourversion = previoustags[-1] + ".999"
> +    else:
> +        ourversion = release_series[ourseries] + ".999"
> +
> +series = [k for k in release_series]

dict(release_series.keys()) ? but works either way I guess

> +previousseries = series[series.index(ourseries)+1:]
> +lastlts = [k for k in previousseries if k in ltsseries]
> +
> +print("Version calculated to be %s" % ourversion)
> +print("Release series calculated to be %s" % ourseries)
> +
> +replacements = {
> +    "DISTRO" : ourversion,
> +    "DISTRO_NAME_NO_CAP" : ourseries,
> +    "DISTRO_NAME" : ourseries.capitalize(),
> +    "DISTRO_NAME_NO_CAP_MINUS_ONE" : previousseries[0],
> +    "DISTRO_NAME_NO_CAP_LTS" : lastlts[0],
> +    "YOCTO_DOC_VERSION" : ourversion,
> +    "DISTRO_REL_TAG" : "yocto-" + ourversion,
> +}
> +
> +with open("poky.yaml.in", "r") as r, open("poky.yaml", "w") as w:
> +    lines = r.readlines()
> +    for line in lines:
> +        data = line.split(":")
> +        k = data[0].strip()
> +        if k in replacements:
> +            w.write("%s : \"%s\"\n" % (k, replacements[k]))
> +        else:
> +            w.write(line)
> +

We could just do a yaml.safe_load(poky.yaml.in) and update the dict 
returned by the function and then do a yaml.dump() to poky.yaml. At 
least we'd be sure that it's valid YAML syntax and that the manual 
replacement logic hasn't missed a corner case.

Now, I said on IRC that I'd initially thought about having this put into 
yocto-vars.py to modify the poky.yaml right before it's used by Sphinx.

Two issues:
- slower sphinx builds
- opaque values before use by Sphinx

One benefit:
- automatically done

But I'd say it's just me nitpicking :)

Cheers,
Quentin

On Mon, 2022-03-21 at 16:49 +0100, Quentin Schulz wrote:
> Hi Richard,
> 
> On 3/18/22 16:42, Richard Purdie wrote:
> > 
> > +if ourversion:
> > +    # We're a tagged release
> > +    components = ourversion.split(".")
> > +    baseversion = components[0] + "." + components[1]
> > +    for i in release_series:
> > +        if release_series[i] == baseversion:
> > +            ourseries = i
> > +            ourbranch = i
> > +else:
> > +    # We're floating on a branch
> > +    branch = subprocess.run(["git", "branch", "--show-current"], capture_output=True, text=True).stdout.strip()
> 
> Are we safe here if we're not on a branch? e.g. a specific commit?

I think the code below should catch that case?

> 
> > +    ourbranch = branch
> > +    if branch != "master" and branch not in release_series:
> > +        possible_branches = []
> > +        for b in release_series.keys():
> > +            result = subprocess.run(["git", "show-ref", "heads/" + b], capture_output=True, text=True)
> > +            if result.returncode == 0:
> > +                possible_branches.append(b)
> > +                continue
> > +            result = subprocess.run(["git", "show-ref", "origin/" + b], capture_output=True, text=True)
> 
> Not everybody has the upstream remote named origin (I don't :) ).
> 
> What about using [git ,branch ,--remote, --list, '*/' + b] ? Note that 
> the return code is always 0 even if nothing's found.
> 
> Just need to add all matches if one is found.
> 
> Also, could use:
> 
> git branch --all --list b --list '*/'+b
> 
> to replace the two subprocesses since it returns both local and remote 
> branches.

The code changed a bit in the newer version but the problem with origin is
probably still there :(. Not entirely sure what to do about that...

> > +            if result.returncode == 0:
> > +                possible_branches.append("origin/" + b)
> > +        nearestbranch = subprocess.run('git show-branch master ' + ' '.join(possible_branches) + ' | grep "*" | grep -v "$(git rev-parse --abbrev-ref HEAD)" | head -n1', shell=True, capture_output=True, text=True).stdout
> 
> Since yuou're using shell=True, I would at least pass the possible 
> branch names each through shlex.quote.
> 
> I also find this command very complex. What I could suggest instead 
> would be to iterate over git rev-list --count master..branch and find 
> the one that is the closest this way?
> 
> Also not sure about the grep "*", are you really looking for lines with 
> a * character in it?

I changed this in a later patch basically to do a count of "git log" between the
two branches so that piece should be addressed later in the series. Michael had
queued this already so I just left the history.

> 
> > +        branch = nearestbranch.split('[')[1].split('~')[0]
> > +        print("Nearest release branch esimtated to be %s" % branch)
> > +    if branch == "master":
> > +        ourseries = devbranch
> 
> This is a risky assumption if the user hasn't pulled upstream in a long 
> time, but I guess there isn't really anything to do about it :)

The impact would mostly be to local versioning in the document so probably not a
huge deal?

> 
> > +    elif branch in release_series:
> > +        ourseries = branch
> > +    else:
> > +        sys.exit("Unknown series for branch %s" % branch)
> > +
> > +    previoustags = subprocess.run(["git", "tag", "--merged", "HEAD"], capture_output=True, text=True).stdout
> > +    previoustags = [t[6:] for t in previoustags.split() if t.startswith("yocto-" + release_series[ourseries])]
> > +    futuretags = subprocess.run(["git", "tag", "--merged", ourbranch], capture_output=True, text=True).stdout
> > +    futuretags = [t[6:] for t in futuretags.split() if t.startswith("yocto-" + release_series[ourseries])]
> > +
> 
> I don't understand why "ourbranch" is used here, isn't it technically 
> the same as HEAD since it's the result of git branch --show-current of HEAD?
> 
> Did you mean "branch" instead since it could be either the returned 
> value of git branch --show-current OR nearestbranch?

I did mean ourbranch here since you could have a tag yocto-3.4.1 and a branch of
honister with a later 3.4.2 tag too. This then triggers the 3.4.1.999 version
(i.e. an increment on 3.4.1) instead of 3.4.999.

> > +    # Append .999 against the last known version
> > +    if len(previoustags) != len(futuretags):
> > +        ourversion = previoustags[-1] + ".999"
> > +    else:
> > +        ourversion = release_series[ourseries] + ".999"
> > +
> > +series = [k for k in release_series]
> 
> dict(release_series.keys()) ? but works either way I guess

The OrderedDict didn't quite work the way I'd expect in that regard which is why
I did this :/.

> 
> > +previousseries = series[series.index(ourseries)+1:]
> > +lastlts = [k for k in previousseries if k in ltsseries]
> > +
> > +print("Version calculated to be %s" % ourversion)
> > +print("Release series calculated to be %s" % ourseries)
> > +
> > +replacements = {
> > +    "DISTRO" : ourversion,
> > +    "DISTRO_NAME_NO_CAP" : ourseries,
> > +    "DISTRO_NAME" : ourseries.capitalize(),
> > +    "DISTRO_NAME_NO_CAP_MINUS_ONE" : previousseries[0],
> > +    "DISTRO_NAME_NO_CAP_LTS" : lastlts[0],
> > +    "YOCTO_DOC_VERSION" : ourversion,
> > +    "DISTRO_REL_TAG" : "yocto-" + ourversion,
> > +}
> > +
> > +with open("poky.yaml.in", "r") as r, open("poky.yaml", "w") as w:
> > +    lines = r.readlines()
> > +    for line in lines:
> > +        data = line.split(":")
> > +        k = data[0].strip()
> > +        if k in replacements:
> > +            w.write("%s : \"%s\"\n" % (k, replacements[k]))
> > +        else:
> > +            w.write(line)
> > +
> 
> We could just do a yaml.safe_load(poky.yaml.in) and update the dict 
> returned by the function and then do a yaml.dump() to poky.yaml. At 
> least we'd be sure that it's valid YAML syntax and that the manual 
> replacement logic hasn't missed a corner case.

I saw this the other way, I only wanted to change two very specific things and
not influence anything else, so it was more of a search and replace operation
with no other knowledge of the context (yaml). It also avoided the need for a
pyyaml dependency which I realise is silly given we're about to use sphinx which
needs it.

> Now, I said on IRC that I'd initially thought about having this put into 
> yocto-vars.py to modify the poky.yaml right before it's used by Sphinx.
> 
> Two issues:
> - slower sphinx builds
> - opaque values before use by Sphinx
> 
> One benefit:
> - automatically done
> 
> But I'd say it's just me nitpicking :)

Something to ponder in a later discussion...

Cheers,

Richard