[PATCH v2] binman: bintool: Add support for tool directories
Simon Glass
sjg at chromium.org
Wed Feb 22 22:20:08 CET 2023
Hi Neha,
On Tue, 21 Feb 2023 at 21:30, Neha Malcom Francis <n-francis at ti.com> wrote:
>
> Hi Simon
>
> On 22/02/23 01:05, Simon Glass wrote:
> > Hi Neha,
> >
> > On Fri, 17 Feb 2023 at 04:46, Neha Malcom Francis <n-francis at ti.com> wrote:
> >>
> >> Currently, bintool supports external compilable tools as single
> >> executable files. Adding support for git repos that can be used to run
> >> non-compilable scripting tools that cannot otherwise be present in
> >> binman.
> >>
> >> Signed-off-by: Neha Malcom Francis <n-francis at ti.com>
> >> ---
> >> Changes in v2:
> >> - added parameter to obtain path to download the directory
> >> optionally, enables flexibility to avoid using
> >> DOWNLOAD_DESTDIR
> >> - added test to bintool_test.py
> >> - s/FETCH_NO_BUILD/FETCH_SOURCE
> >> - code reformatting
> >
> > This looks better but I see have some questions and nits.
> >
> >>
> >> tools/binman/bintool.py | 45 ++++++++++++++++++++++++++++------
> >> tools/binman/bintool_test.py | 22 +++++++++++++++++
> >> tools/binman/btool/_testing.py | 5 ++++
> >> tools/patman/tools.py | 2 +-
> >> 4 files changed, 66 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
> >> index 8fda13ff01..04c951fa0b 100644
> >> --- a/tools/binman/bintool.py
> >> +++ b/tools/binman/bintool.py
> >> @@ -32,12 +32,13 @@ FORMAT = '%-16.16s %-12.12s %-26.26s %s'
> >> modules = {}
> >>
> >> # Possible ways of fetching a tool (FETCH_COUNT is number of ways)
> >> -FETCH_ANY, FETCH_BIN, FETCH_BUILD, FETCH_COUNT = range(4)
> >> +FETCH_ANY, FETCH_BIN, FETCH_BUILD, FETCH_SOURCE, FETCH_COUNT = range(5)
> >>
> >> FETCH_NAMES = {
> >> FETCH_ANY: 'any method',
> >> FETCH_BIN: 'binary download',
> >> - FETCH_BUILD: 'build from source'
> >> + FETCH_BUILD: 'build from source',
> >> + FETCH_SOURCE: 'download source without building'
> >
> > Would this be a script? Should we say 'download script without building' ?
> >
>
> Addressed this in a further below comment.
>
> >> }
> >>
> >> # Status of tool fetching
> >> @@ -201,12 +202,13 @@ class Bintool:
> >> print(f'- trying method: {FETCH_NAMES[try_method]}')
> >> result = try_fetch(try_method)
> >> if result:
> >> + method = try_method
> >> break
> >> else:
> >> result = try_fetch(method)
> >> if not result:
> >> return FAIL
> >> - if result is not True:
> >> + if result is not True and method != FETCH_SOURCE:
> >> fname, tmpdir = result
> >> dest = os.path.join(DOWNLOAD_DESTDIR, self.name)
> >> print(f"- writing to '{dest}'")
> >> @@ -261,7 +263,7 @@ class Bintool:
> >> show_status(col.RED, 'Failures', status[FAIL])
> >> return not status[FAIL]
> >>
> >> - def run_cmd_result(self, *args, binary=False, raise_on_error=True):
> >> + def run_cmd_result(self, *args, binary=False, raise_on_error=True, add_name=True):
> >
> > Please update function comment for new param
> >
> >> """Run the bintool using command-line arguments
> >>
> >> Args:
> >> @@ -278,7 +280,10 @@ class Bintool:
> >> if self.name in self.missing_list:
> >> return None
> >> name = os.path.expanduser(self.name) # Expand paths containing ~
> >> - all_args = (name,) + args
> >> + if add_name:
> >> + all_args = (name,) + args
> >> + else:
> >> + all_args = args
> >> env = tools.get_env_with_path()
> >> tout.detail(f"bintool: {' '.join(all_args)}")
> >> result = command.run_pipe(
> >> @@ -304,7 +309,7 @@ class Bintool:
> >> tout.debug(result.stderr)
> >> return result
> >>
> >> - def run_cmd(self, *args, binary=False):
> >> + def run_cmd(self, *args, binary=False, add_name=True):
> >
> > Please update function comment for new param
> >
> >> """Run the bintool using command-line arguments
> >>
> >> Args:
> >> @@ -315,7 +320,7 @@ class Bintool:
> >> Returns:
> >> str or bytes: Resulting stdout from the bintool
> >> """
> >> - result = self.run_cmd_result(*args, binary=binary)
> >> + result = self.run_cmd_result(*args, binary=binary, add_name=add_name)
> >> if result:
> >> return result.stdout
> >>
> >> @@ -354,6 +359,32 @@ class Bintool:
> >> return None
> >> return fname, tmpdir
> >>
> >> + @classmethod
> >> + def fetch_from_git(cls, git_repo, name, toolpath=DOWNLOAD_DESTDIR):
> >> + """Fetch a bintool git repo
> >> +
> >> + This clones the repo and returns
> >> +
> >> + Args:
> >> + git_repo (str): URL of git repo
> >> + name (str): Bintool name assigned as tool directory name
> >
> > missing toolpath arg
> >
>
> Will make the above changes
> >> +
> >> + Returns:
> >> + str: Directory of fetched repo
> >> + or None on error
> >> + """
> >> + dir = os.path.join(toolpath, name)
> >> + if os.path.exists(dir):
> >> + print(f"- Repo {dir} already exists")
> >> + return None
> >> + os.mkdir(dir)
> >> + print(f"- clone git repo '{git_repo}' to '{dir}'")
> >> + tools.run('git', 'clone', '--depth', '1', git_repo, dir)
> >
> > doesn't this download directly into the download directory? What if
> > there are other files in the git repo...they will all end up in there,
> > right? Can we instead specify the filename that we want?
> >
> > Also, if it is a directory, how does the tool actually get used?
> >
>
> Right, I wanted to give flexibility to pick up different files and
> scripts within the same repo, which is why it's FETCH_SOURCE and not
> FETCH_GIT. The way I see it, right now the way you can pick up and build
> your tool is limited. My use case would be a direct example of this,
> will try posting it this week based on this patch so you could get an
> idea. Here there are multiple scripts that are run based on the
> properties picked up by the etype.
>
> This could be done by just picking up only that filename, but that would
> involve cloning/downloading the source multiple times. This patch avoids
> doing that.
So are these individual scripts going to appear in the download dir,
or will they be in a subdir of that, with the same name as the repo?
Effectively you are grouping scripts together. Would it instead be
possible to have one top-level script which calls the others, based on
a subcommand passed in, a bit like futility? [1] The existing gbb and
vblock entry types use that one utility.
As to the performance, I don't think it matters to clone the repo
multiple times. I actually prefer seeing each individual tool in the
'binman tool -l' list, rather than one of the items in the list being
a placeholder for the group.
There is still the question of whether we would want to put the tools
in a subdir to group them...not sure I like that idea much. But if we
end up needing a Python tool, it may well have a subdir with the
actual code in it, spread over multiple files...
>
> >> + if not os.path.exists(dir):
> >> + print(f"- Repo '{dir}' was not produced")
> >> + return None
> >> + return dir
> >> +
> >> @classmethod
> >> def fetch_from_url(cls, url):
> >> """Fetch a bintool from a URL
> >> diff --git a/tools/binman/bintool_test.py b/tools/binman/bintool_test.py
> >> index 7efb8391db..d95b0b7025 100644
> >> --- a/tools/binman/bintool_test.py
> >> +++ b/tools/binman/bintool_test.py
> >> @@ -258,6 +258,28 @@ class TestBintool(unittest.TestCase):
> >> fname = os.path.join(self._indir, '_testing')
> >> return fname if write_file else self.fname, stdout.getvalue()
> >>
> >> + def check_fetch_source_method(self, write_file):
> >> + """Check output from fetching using SOURCE method
> >> +
> >> + Args:
> >> + write_file (bool): True to write to output directory
> >> +
> >> + Returns:
> >> + tuple:
> >> + str: Filename of written file (or missing 'make' output)
> >> + str: Contents of stdout
> >> + """
> >> +
> >> + btest = Bintool.create('_testing')
> >> + col = terminal.Color()
> >> + self.fname = None
> >> + with unittest.mock.patch.object(bintool, 'DOWNLOAD_DESTDIR',
> >> + self._indir):
> >> + with test_util.capture_sys_output() as (stdout, _):
> >> + btest.fetch_tool(bintool.FETCH_SOURCE, col, False)
> >> + fname = os.path.join(self._indir, '_testing')
> >> + return fname if write_file else self.fname, stdout.getvalue()
> >> +
> >> def test_build_method(self):
> >> """Test fetching using the build method"""
> >> fname, stdout = self.check_build_method(write_file=True)
> >> diff --git a/tools/binman/btool/_testing.py b/tools/binman/btool/_testing.py
> >> index 4005e8a8a5..3863966011 100644
> >> --- a/tools/binman/btool/_testing.py
> >> +++ b/tools/binman/btool/_testing.py
> >> @@ -5,6 +5,8 @@
> >>
> >> This is not a real bintool, just one used for testing"""
> >>
> >> +import tempfile
> >> +
> >> from binman import bintool
> >>
> >> # pylint: disable=C0103
> >> @@ -33,4 +35,7 @@ class Bintool_testing(bintool.Bintool):
> >> return self.fetch_from_drive('junk')
> >> if method == bintool.FETCH_BUILD:
> >> return self.build_from_git('url', 'target', 'pathname')
> >> + tmpdir = tempfile.mkdtemp(prefix='binmanf.')
> >> + if method == bintool.FETCH_SOURCE:
> >> + return self.fetch_from_git('giturl', 'mygit', tmpdir)
> >> return None
> >> diff --git a/tools/patman/tools.py b/tools/patman/tools.py
> >> index 2ac814d476..b69a651eab 100644
> >> --- a/tools/patman/tools.py
> >> +++ b/tools/patman/tools.py
> >> @@ -397,7 +397,7 @@ def tool_find(name):
> >> paths += tool_search_paths
> >> for path in paths:
> >> fname = os.path.join(path, name)
> >> - if os.path.isfile(fname) and os.access(fname, os.X_OK):
> >> + if (os.path.isfile(fname) or os.path.isdir(fname)) and os.access(fname, os.X_OK):
> >> return fname
> >
> > I don't understand here how a directory can be a tool...normally it is
> > an executable.
> >
>
> Right... hope above comments answer the use case I'm targetting.
> Regarding tool normally being an executable, agreed. But I couldn't find
> another place that could fit in and house this functionality which I
> think other developers will need in future. We could either:
>
> - change the definition of a bintool to not necessarily being an
> executable but any external source needed to help package an image.
Can you give an example of such a source? I am obviously fine with the
idea of having executable scripts as opposed to just ELF files. But I
think I need a bit more info to understand what is going on here.
>
> - pick up only the filename (script) we want and return but with
> implication of same git repo getting cloned multiple times if it
> contains more than 1 filename that gets used.
So far as I understand the use case, this is my preference.
>
> >>
> >> def run(name, *args, **kwargs):
> >> --
> >> 2.34.1
> >>
> >
> > Also please note that there are a few lines in bintool.py without test
> > coverage (binman test -T)
> >
> I'll add coverage.
>
> > Regards,
> > Simon
>
> --
> Thanking You
> Neha Malcom Francis
[1] https://manpages.ubuntu.com/manpages/focal/man1/futility.1.html
Regards,
Simon
More information about the U-Boot
mailing list