* [PATCH v2] binman: bintool: Add support for tool directories
@ 2023-02-17 11:46 Neha Malcom Francis
2023-02-21 19:35 ` Simon Glass
0 siblings, 1 reply; 5+ messages in thread
From: Neha Malcom Francis @ 2023-02-17 11:46 UTC (permalink / raw)
To: u-boot, sjg, alpernebiyasak, vigneshr, rogerq; +Cc: n-francis, afd, trini
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@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
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'
}
# 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):
"""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):
"""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
+
+ 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)
+ 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
def run(name, *args, **kwargs):
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] binman: bintool: Add support for tool directories
2023-02-17 11:46 [PATCH v2] binman: bintool: Add support for tool directories Neha Malcom Francis
@ 2023-02-21 19:35 ` Simon Glass
2023-02-22 4:30 ` Neha Malcom Francis
0 siblings, 1 reply; 5+ messages in thread
From: Simon Glass @ 2023-02-21 19:35 UTC (permalink / raw)
To: Neha Malcom Francis; +Cc: u-boot, alpernebiyasak, vigneshr, rogerq, afd, trini
Hi Neha,
On Fri, 17 Feb 2023 at 04:46, Neha Malcom Francis <n-francis@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@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] binman: bintool: Add support for tool directories
2023-02-21 19:35 ` Simon Glass
@ 2023-02-22 4:30 ` Neha Malcom Francis
2023-02-22 21:20 ` Simon Glass
0 siblings, 1 reply; 5+ messages in thread
From: Neha Malcom Francis @ 2023-02-22 4:30 UTC (permalink / raw)
To: Simon Glass; +Cc: u-boot, alpernebiyasak, vigneshr, rogerq, afd, trini
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@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@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.
>> + 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.
- 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.
>>
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] binman: bintool: Add support for tool directories
2023-02-22 4:30 ` Neha Malcom Francis
@ 2023-02-22 21:20 ` Simon Glass
2023-02-24 11:31 ` Neha Malcom Francis
0 siblings, 1 reply; 5+ messages in thread
From: Simon Glass @ 2023-02-22 21:20 UTC (permalink / raw)
To: Neha Malcom Francis; +Cc: u-boot, alpernebiyasak, vigneshr, rogerq, afd, trini
Hi Neha,
On Tue, 21 Feb 2023 at 21:30, Neha Malcom Francis <n-francis@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@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@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] binman: bintool: Add support for tool directories
2023-02-22 21:20 ` Simon Glass
@ 2023-02-24 11:31 ` Neha Malcom Francis
0 siblings, 0 replies; 5+ messages in thread
From: Neha Malcom Francis @ 2023-02-24 11:31 UTC (permalink / raw)
To: Simon Glass; +Cc: u-boot, alpernebiyasak, vigneshr, rogerq, afd, trini
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@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@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@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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-02-24 11:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-17 11:46 [PATCH v2] binman: bintool: Add support for tool directories Neha Malcom Francis
2023-02-21 19:35 ` Simon Glass
2023-02-22 4:30 ` Neha Malcom Francis
2023-02-22 21:20 ` Simon Glass
2023-02-24 11:31 ` Neha Malcom Francis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.