[PATCH v2] binman: Get futility by building it

[Simon Glass]

Hi Quentin,

On Tue, 13 Sept 2022 at 02:55, Quentin Schulz
<quentin.schulz at theobroma-systems.com> wrote:
>
> Hi Simon,
>
> On 9/12/22 15:35, Simon Glass wrote:
> > A binary download is not great, since it depends on libraries being
> > present in the system. Build futility from source instead.
> >
> > Note that this requires two patches to the source repo which are in
> > progress:
> >
> >     https://urldefense.proofpoint.com/v2/url?u=https-3A__issuetracker.google.com_issues_245993083-3Fpli-3D1&d=DwIDAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=aZRnLGuudyF3wf57Yf7GAac-g9Rgf3zQuq1O9GJPIdIhZ_w2XDHpPsZGFQAIMFT9&s=j0aTJGKOhRncvmyFptYpT_Y9Qb3U3CDUiqG2jO_7hAQ&e=
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v2:
> > - Pull from github instead, to avoid needing to login / gitcookies
> >
> >   tools/binman/bintool.py        | 17 +++++++++++++----
> >   tools/binman/btool/futility.py | 12 ++++++++----
> >   2 files changed, 21 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
> > index 032179a99de..52ec0030590 100644
> > --- a/tools/binman/bintool.py
> > +++ b/tools/binman/bintool.py
> > @@ -319,7 +319,8 @@ class Bintool:
> >               return result.stdout
> >
> >       @classmethod
> > -    def build_from_git(cls, git_repo, make_target, bintool_path):
> > +    def build_from_git(cls, git_repo, make_target, bintool_path, flags=None,
> > +                       branch=None):
> >           """Build a bintool from a git repo
> >
> >           This clones the repo in a temporary directory, builds it with 'make',
> > @@ -330,6 +331,8 @@ class Bintool:
> >               make_target (str): Target to pass to 'make' to build the tool
> >               bintool_path (str): Relative path of the tool in the repo, after
> >                   build is complete
> > +            flags (list of str): Flags or variables to pass to make, or None
> > +            branch (str): Branch to build, None for the default
> >
> >           Returns:
> >               tuple:
> > @@ -339,10 +342,16 @@ class Bintool:
> >           """
> >           tmpdir = tempfile.mkdtemp(prefix='binmanf.')
> >           print(f"- clone git repo '{git_repo}' to '{tmpdir}'")
> > -        tools.run('git', 'clone', '--depth', '1', git_repo, tmpdir)
> > +        args = ['git', 'clone', '--depth', '1']
> > +        if branch:
> > +            args += ['-b', branch]
>
> I don't like branches too much, a commit hash would probably be better
> for reproducibility, we would need a git checkout command though since
> git clone command does not allow for commit hashes AFAIR. Up to you I guess.

Well I plan land the required patch upstream so will be able to use
the master branch. It's a shame that Chromium git requires an account.

>
> > +        tools.run(*args, git_repo, tmpdir)
> >           print(f"- build target '{make_target}'")
> > -        tools.run('make', '-C', tmpdir, '-j', f'{multiprocessing.cpu_count()}',
> > -                  make_target)
> > +        cmd = ['make', '-C', tmpdir, '-j', f'{multiprocessing.cpu_count()}',
> > +               make_target]
> > +        if flags:
> > +            cmd += flags > +        tools.run(*cmd)
> >           fname = os.path.join(tmpdir, bintool_path)
> >           if not os.path.exists(fname):
> >               print(f"- File '{fname}' was not produced")
> > diff --git a/tools/binman/btool/futility.py b/tools/binman/btool/futility.py
> > index 75a05c2ac66..121a366830b 100644
> > --- a/tools/binman/btool/futility.py
> > +++ b/tools/binman/btool/futility.py
> > @@ -160,8 +160,12 @@ class Bintoolfutility(bintool.Bintool):
> >           Raises:
> >               Valuerror: Fetching could not be completed
> >           """
> > -        if method != bintool.FETCH_BIN:
> > +        if method != bintool.FETCH_BUILD:
> >               return None
> > -        fname, tmpdir = self.fetch_from_drive(
> > -            '1hdsInzsE4aJbmBeJ663kYgjOQyW1I-E0')
> > -        return fname, tmpdir
> > +        result = self.build_from_git(
> > +            'https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_sjg20_vboot-5Freference.git&d=DwIDAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=aZRnLGuudyF3wf57Yf7GAac-g9Rgf3zQuq1O9GJPIdIhZ_w2XDHpPsZGFQAIMFT9&s=4I4vh_QaOw3S9ET9XnKHOHUkf7Qu3SGMGfUKNe5Uxe4&e=  ',
> > +            'all',
> > +            'build/futility/futility',
> > +            flags=['USE_FLASHROM=0'],
> > +            branch='fut')
> > +        return result
>
> Seems to be doing the job, it fetches and builds the futility binary.
> Having some hard time figuring out how to test it produces a valid
> binary it seems most of the calls to futility in ftest.py are faked/mocked?

Yes I think they all are. It would be easy enough to have a test which
generates a real GBB and vblock and is skipped if futility is missing.

>
> Also, would be great to have the DOWNLOAD_DESTDIR part of toolpath as I
> found it quite surprising to run binman tool -f missing and still have
> the binary not appear when running binman tool --list. But that's not
> related to this commit :)

Yes, would you like to send a patch? The --toolpath arg is intended to
allow multiple paths to be provided, but I think it would be OK to use
it for what you say here. Perhap also support a BINMAN_TOOLPATH env
var?

Regards,
Simon