Hi Neha, On Tue, 21 Feb 2023 at 21:30, Neha Malcom Francis <n-fran...@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-fran...@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-fran...@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