[2/3] cargo_common.bbclass: Handle Cargo.lock modifications for git dependencies

From: Frederic Martinsons <frederic.martinsons@gmail.com>

Now we use --frozen, Cargo.lock cannot be modified by cargo build.
These patched git dependencies requires that the git url is removed
from Cargo.lock.

Fixes #15104

Signed-off-by: Frederic Martinsons <frederic.martinsons@gmail.com>
---
 meta/classes-recipe/cargo_common.bbclass | 40 ++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

On Mon, 2023-07-31 at 11:44 +0200, Frederic Martinsons wrote:
> From: Frederic Martinsons <frederic.martinsons@gmail.com>
> 
> Now we use --frozen, Cargo.lock cannot be modified by cargo build.
> These patched git dependencies requires that the git url is removed
> from Cargo.lock.
> 
> Fixes #15104
> 
> Signed-off-by: Frederic Martinsons <frederic.martinsons@gmail.com>
> ---
>  meta/classes-recipe/cargo_common.bbclass | 40 ++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/meta/classes-recipe/cargo_common.bbclass b/meta/classes-recipe/cargo_common.bbclass
> index db54826ddb..01afb74640 100644
> --- a/meta/classes-recipe/cargo_common.bbclass
> +++ b/meta/classes-recipe/cargo_common.bbclass
> @@ -117,6 +117,8 @@ cargo_common_do_configure () {
>  }
>  
>  python cargo_common_do_patch_paths() {
> +    import shutil
> +
>      cargo_config = os.path.join(d.getVar("CARGO_HOME"), "config")
>      if not os.path.exists(cargo_config):
>          return
> @@ -146,6 +148,44 @@ python cargo_common_do_patch_paths() {
>              print('\n[patch."%s"]' % k, file=config)
>              for name in v:
>                  print(name, file=config)
> +
> +    if not patches:
> +        return
> +
> +    # Cargo.lock file is needed for to be sure that artifacts
> +    # downloaded by the fetch steps are those expected by the
> +    # project and that the possible patches are correctly applied.
> +    # Moreover since we do not want any modification
> +    # of this file (for reproducibility purpose), we prevent it by
> +    # using --frozen flag (in CARGO_BUILD_FLAGS) and raise a clear error
> +    # here is better than letting cargo tell (in case the file is missing)
> +    # "Cargo.lock should be modified but --frozen was given"
> +
> +    manifest_path = d.getVar("MANIFEST_PATH", True)
> +    lockfile = os.path.join(os.path.dirname(manifest_path), "Cargo.lock")
> +    if not os.path.exists(lockfile):
> +        bb.fatal(f"{lockfile} file doesn't exist")
> +
> +    # There are patched files and so Cargo.lock should be modified but we use
> +    # --frozen so let's handle that modifications here.
> +    #
> +    # Note that a "better" (more elegant ?) would have been to use cargo update for
> +    # patched packages:
> +    #  cargo update --offline -p package_1 -p package_2
> +    # But this is not possible since it requires that cargo local git db
> +    # to be populated and this is not the case as we fetch git repo ourself.
> +
> +    newlines = []
> +    with open(lockfile, "r") as f:
> +        for line in f.readlines():
> +            if not line.startswith("source = \"git"):
> +                newlines.append(line)
> +
> +    lockfile_mod = lockfile + ".new"
> +    with open(lockfile_mod, "w") as f:
> +        f.writelines(newlines)
> +
> +    shutil.move(lockfile_mod, lockfile)
>  }
>  do_configure[postfuncs] += "cargo_common_do_patch_paths"

This patch isn't "wrong" but there are some tips we've picked up over
the years of doing this which might help things in the future.

Some questions:

What happens if we run the cargo_common_do_patch_paths function
multiple times?
What happens if the user hits Ctrl+C at an inopportune moment?

In this case the function is mostly ok but it might easily change in
the future to something that isn't.

The best practise we've come to is the pattern of firstly copy X to
X.orig if X.orig doesn't exist. This ensures you always start from a
pristine backup. You can then directly open and write to X reading from
X.orig.

This ensures that both of the questions I ask above become non-issues.

I'm know I'm being a little picky by mentioning this but I do want to
raise awareness of the better pattern in general and make sure that
examples in the class code are good as people will copy them!

Cheers,

Richard

Thank you for the tips, will work on a v2 implementing that you suggested

On Tue, 1 Aug 2023 at 10:44, Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Mon, 2023-07-31 at 11:44 +0200, Frederic Martinsons wrote:
> > From: Frederic Martinsons <frederic.martinsons@gmail.com>
> >
> > Now we use --frozen, Cargo.lock cannot be modified by cargo build.
> > These patched git dependencies requires that the git url is removed
> > from Cargo.lock.
> >
> > Fixes #15104
> >
> > Signed-off-by: Frederic Martinsons <frederic.martinsons@gmail.com>
> > ---
> >  meta/classes-recipe/cargo_common.bbclass | 40 ++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/meta/classes-recipe/cargo_common.bbclass
> b/meta/classes-recipe/cargo_common.bbclass
> > index db54826ddb..01afb74640 100644
> > --- a/meta/classes-recipe/cargo_common.bbclass
> > +++ b/meta/classes-recipe/cargo_common.bbclass
> > @@ -117,6 +117,8 @@ cargo_common_do_configure () {
> >  }
> >
> >  python cargo_common_do_patch_paths() {
> > +    import shutil
> > +
> >      cargo_config = os.path.join(d.getVar("CARGO_HOME"), "config")
> >      if not os.path.exists(cargo_config):
> >          return
> > @@ -146,6 +148,44 @@ python cargo_common_do_patch_paths() {
> >              print('\n[patch."%s"]' % k, file=config)
> >              for name in v:
> >                  print(name, file=config)
> > +
> > +    if not patches:
> > +        return
> > +
> > +    # Cargo.lock file is needed for to be sure that artifacts
> > +    # downloaded by the fetch steps are those expected by the
> > +    # project and that the possible patches are correctly applied.
> > +    # Moreover since we do not want any modification
> > +    # of this file (for reproducibility purpose), we prevent it by
> > +    # using --frozen flag (in CARGO_BUILD_FLAGS) and raise a clear error
> > +    # here is better than letting cargo tell (in case the file is
> missing)
> > +    # "Cargo.lock should be modified but --frozen was given"
> > +
> > +    manifest_path = d.getVar("MANIFEST_PATH", True)
> > +    lockfile = os.path.join(os.path.dirname(manifest_path),
> "Cargo.lock")
> > +    if not os.path.exists(lockfile):
> > +        bb.fatal(f"{lockfile} file doesn't exist")
> > +
> > +    # There are patched files and so Cargo.lock should be modified but
> we use
> > +    # --frozen so let's handle that modifications here.
> > +    #
> > +    # Note that a "better" (more elegant ?) would have been to use
> cargo update for
> > +    # patched packages:
> > +    #  cargo update --offline -p package_1 -p package_2
> > +    # But this is not possible since it requires that cargo local git db
> > +    # to be populated and this is not the case as we fetch git repo
> ourself.
> > +
> > +    newlines = []
> > +    with open(lockfile, "r") as f:
> > +        for line in f.readlines():
> > +            if not line.startswith("source = \"git"):
> > +                newlines.append(line)
> > +
> > +    lockfile_mod = lockfile + ".new"
> > +    with open(lockfile_mod, "w") as f:
> > +        f.writelines(newlines)
> > +
> > +    shutil.move(lockfile_mod, lockfile)
> >  }
> >  do_configure[postfuncs] += "cargo_common_do_patch_paths"
>
> This patch isn't "wrong" but there are some tips we've picked up over
> the years of doing this which might help things in the future.
>
> Some questions:
>
> What happens if we run the cargo_common_do_patch_paths function
> multiple times?
> What happens if the user hits Ctrl+C at an inopportune moment?
>
> In this case the function is mostly ok but it might easily change in
> the future to something that isn't.
>
> The best practise we've come to is the pattern of firstly copy X to
> X.orig if X.orig doesn't exist. This ensures you always start from a
> pristine backup. You can then directly open and write to X reading from
> X.orig.
>
> This ensures that both of the questions I ask above become non-issues.
>
> I'm know I'm being a little picky by mentioning this but I do want to
> raise awareness of the better pattern in general and make sure that
> examples in the class code are good as people will copy them!
>
> Cheers,
>
> Richard
>
>
>
>
>
>
>
>
>
>