[PATCH v2] binman: bintool: Add support for tool directories
Simon Glass
sjg at chromium.org
Tue Feb 21 20:35:45 CET 2023
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' ?
> }
>
> # 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
> +
> + 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?
> + 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.
>
> 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)
Regards,
Simon
More information about the U-Boot
mailing list