diff mbox series

[v3] binman: bintool: Add support for tool directories

Message ID 20230224115101.563729-1-n-francis@ti.com
State Deferred
Delegated to: Simon Glass
Headers show
Series [v3] binman: bintool: Add support for tool directories | expand

Commit Message

Neha Malcom Francis Feb. 24, 2023, 11:51 a.m. UTC
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 v3:
	- moved back to using DOWNLOAD_DIR as community is making
	  relevant changes
	- extended coverage for bintool_test.py
	- added function comment for new parameter

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        | 47 +++++++++++++++++++++++++++++-----
 tools/binman/bintool_test.py   | 43 +++++++++++++++++++++++++++++++
 tools/binman/btool/_testing.py |  4 +++
 tools/patman/tools.py          |  2 +-
 4 files changed, 88 insertions(+), 8 deletions(-)

Comments

Simon Glass March 11, 2023, 1:47 a.m. UTC | #1
Hi Neha,

On Fri, 24 Feb 2023 at 03:51, 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 v3:
>         - moved back to using DOWNLOAD_DIR as community is making
>           relevant changes
>         - extended coverage for bintool_test.py
>         - added function comment for new parameter
>
> 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        | 47 +++++++++++++++++++++++++++++-----
>  tools/binman/bintool_test.py   | 43 +++++++++++++++++++++++++++++++
>  tools/binman/btool/_testing.py |  4 +++
>  tools/patman/tools.py          |  2 +-
>  4 files changed, 88 insertions(+), 8 deletions(-)

I am OK with doing this but worried that it will be used for shell
scripts, which we are trying to avoid.

The code looks OK for now. Perhaps we can revisit this when we have a
use case? I also think we should have each tool individually shown in
the list, rather than having them be 'hidden' behind a btool.

Regards,
Simon
Neha Malcom Francis March 14, 2023, 3:41 a.m. UTC | #2
Hi Simon

On 11/03/23 07:17, Simon Glass wrote:
> Hi Neha,
> 
> On Fri, 24 Feb 2023 at 03:51, 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 v3:
>>          - moved back to using DOWNLOAD_DIR as community is making
>>            relevant changes
>>          - extended coverage for bintool_test.py
>>          - added function comment for new parameter
>>
>> 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        | 47 +++++++++++++++++++++++++++++-----
>>   tools/binman/bintool_test.py   | 43 +++++++++++++++++++++++++++++++
>>   tools/binman/btool/_testing.py |  4 +++
>>   tools/patman/tools.py          |  2 +-
>>   4 files changed, 88 insertions(+), 8 deletions(-)
> 
> I am OK with doing this but worried that it will be used for shell
> scripts, which we are trying to avoid.
> 
> The code looks OK for now. Perhaps we can revisit this when we have a
> use case? I also think we should have each tool individually shown in
> the list, rather than having them be 'hidden' behind a btool.
> 

I get the intention, let me know your reply to our thread [1] and we can 
work on it from there.

> Regards,
> Simon

[1] 
https://patchwork.ozlabs.org/project/uboot/patch/20230224120340.587786-1-n-francis@ti.com/
Simon Glass March 15, 2023, 6:48 p.m. UTC | #3
Hi Neha,

On Mon, 13 Mar 2023 at 21:41, Neha Malcom Francis <n-francis@ti.com> wrote:
>
> Hi Simon
>
> On 11/03/23 07:17, Simon Glass wrote:
> > Hi Neha,
> >
> > On Fri, 24 Feb 2023 at 03:51, 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 v3:
> >>          - moved back to using DOWNLOAD_DIR as community is making
> >>            relevant changes
> >>          - extended coverage for bintool_test.py
> >>          - added function comment for new parameter
> >>
> >> 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        | 47 +++++++++++++++++++++++++++++-----
> >>   tools/binman/bintool_test.py   | 43 +++++++++++++++++++++++++++++++
> >>   tools/binman/btool/_testing.py |  4 +++
> >>   tools/patman/tools.py          |  2 +-
> >>   4 files changed, 88 insertions(+), 8 deletions(-)
> >
> > I am OK with doing this but worried that it will be used for shell
> > scripts, which we are trying to avoid.
> >
> > The code looks OK for now. Perhaps we can revisit this when we have a
> > use case? I also think we should have each tool individually shown in
> > the list, rather than having them be 'hidden' behind a btool.
> >
>
> I get the intention, let me know your reply to our thread [1] and we can
> work on it from there.

OK I replied to that.

- Simon

> [1]
> https://patchwork.ozlabs.org/project/uboot/patch/20230224120340.587786-1-n-francis@ti.com/
>
> --
> Thanking You
> Neha Malcom Francis
diff mbox series

Patch

diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
index 8fda13ff01..33f563c46f 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:
@@ -270,6 +272,7 @@  class Bintool:
             binary (bool): True to return output as bytes instead of str
             raise_on_error (bool): True to raise a ValueError exception if the
                 tool returns a non-zero return code
+            add_name (bool): True to add bintool name to the beginning of command
 
         Returns:
             CommandResult: Resulting output from the bintool, or None if the
@@ -278,7 +281,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,18 +310,19 @@  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:
             args (list of str): Arguments to provide, in addition to the bintool
                 name
             binary (bool): True to return output as bytes instead of str
+            add_name (bool): True to add bintool name to the beginning of command
 
         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 +361,32 @@  class Bintool:
             return None
         return fname, tmpdir
 
+    @classmethod
+    def fetch_from_git(cls, git_repo, name):
+        """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(DOWNLOAD_DESTDIR, name)
+        if os.path.exists(dir):
+            print(f"- {dir} repo 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 len(os.listdir(dir)) == 0:
+            print(f"- '{dir}' repo is empty")
+            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..e391744289 100644
--- a/tools/binman/bintool_test.py
+++ b/tools/binman/bintool_test.py
@@ -258,6 +258,36 @@  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 directory created
+                str: Contents of stdout
+        """
+        def fake_run(*cmd):
+            if cmd[0] == 'git':
+                # See Bintool.fetch_from_git()
+                tmpdir = cmd[5]
+                self.fname = os.path.join(tmpdir, 'gitfile')
+                if write_file:
+                    tools.write_file(self.fname, b'hello')
+
+        btest = Bintool.create('_testing')
+        col = terminal.Color()
+        self.dirname = None
+        with unittest.mock.patch.object(bintool, 'DOWNLOAD_DESTDIR',
+                                        self._indir):
+            with unittest.mock.patch.object(tools, 'run', side_effect=fake_run):
+                with test_util.capture_sys_output() as (stdout, _):
+                    btest.fetch_tool(bintool.FETCH_SOURCE, col, False)
+        dirname = os.path.join(self._indir, 'mygit')
+        return self.fname if write_file else dirname, stdout.getvalue()
+
     def test_build_method(self):
         """Test fetching using the build method"""
         fname, stdout = self.check_build_method(write_file=True)
@@ -270,6 +300,17 @@  class TestBintool(unittest.TestCase):
         self.assertFalse(os.path.exists(fname))
         self.assertIn(f"File '{fname}' was not produced", stdout)
 
+    def test_fetch_source_method(self):
+        """Test fetching using the source method"""
+        dirname, stdout = self.check_fetch_source_method(write_file=True)
+        self.assertTrue(os.path.exists(dirname))
+
+    def test_fetch_source_method_fail(self):
+        """Test fetching using the source method when directory is empty"""
+        dirname, stdout = self.check_fetch_source_method(write_file=False)
+        self.assertEqual(len(os.listdir(dirname)), 0)
+        self.assertIn(f"'{dirname}' repo is empty", stdout)
+
     def test_install(self):
         """Test fetching using the install method"""
         btest = Bintool.create('_testing')
@@ -347,6 +388,8 @@  class TestBintool(unittest.TestCase):
         btool = Bintool.create('_testing')
         result = btool.run_cmd_result('fred')
         self.assertIsNone(result)
+        result = btool.run_cmd_result('daphne', add_name=False)
+        self.assertIsNone(result)
 
 
 if __name__ == "__main__":
diff --git a/tools/binman/btool/_testing.py b/tools/binman/btool/_testing.py
index 4005e8a8a5..b291d35c58 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,6 @@  class Bintool_testing(bintool.Bintool):
             return self.fetch_from_drive('junk')
         if method == bintool.FETCH_BUILD:
             return self.build_from_git('url', 'target', 'pathname')
+        if method == bintool.FETCH_SOURCE:
+            return self.fetch_from_git('giturl', 'mygit')
         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):