[PATCH v2] binman: bintool: Add support for tool directories
Neha Malcom Francis
n-francis at ti.com
Fri Feb 24 12:31:44 CET 2023
Hi Simon
On 23/02/23 02:50, Simon Glass wrote:
> 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.
>
I did consider this. And this is very much feasible from my end (easier
also I assume!), but I am under the impression that this could benefit
tools that work this way. Not sure if this a possible use-case, haven't
tried this personally but an example would be West, a meta-tool by
Zephyr is completely python, so no 'make'. It is also used to sign boot
binaries [1].
> 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.
>
Hmm... agreed to see each script when listed, but feels very messy.
> 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...
>
Right.
>>
>>>> + 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.
>
I've finished up the ti-secure etype for which I use this functionality,
will post them right after this reply.
>>
>> - 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
[1] https://docs.zephyrproject.org/3.2.0/develop/west/sign.html
--
Thanking You
Neha Malcom Francis
More information about the U-Boot
mailing list