[PATCH 5/5] binman: Make the tooldir configurable

[Quentin Schulz]

Hi Simon,

On 2/18/23 00:19, Simon Glass wrote:
> Add a command-line argument for setting the tooldir, so that the default
> can be overridden. Add this directory to the toolpath automatically.
> Create the directory if it does not already exist.
> 
> Put the default in the argument parser instead of the class, so that it
> is more obvious.
> 
> Update a few tests that expect the utility name to be provided without
> any path (e.g. 'futility'), so they can accept a path, e.g.
> /path/to/futility
> 
> Update the documentation and add a few tests.
> 
> Improve the help for --toolpath while we are here.
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> 
>   tools/binman/binman.rst      | 19 +++++++++++++++----
>   tools/binman/bintool.py      |  8 +++++++-
>   tools/binman/bintool_test.py | 11 ++++++++---
>   tools/binman/cmdline.py      |  6 +++++-
>   tools/binman/control.py      | 10 ++++++++--
>   tools/binman/ftest.py        | 21 +++++++++++++++++++--
>   6 files changed, 62 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
> index 29034da92f1..3b0a9c38d72 100644
> --- a/tools/binman/binman.rst
> +++ b/tools/binman/binman.rst
> @@ -1407,7 +1407,15 @@ You can also use `--fetch all` to fetch all tools or `--fetch <tool>` to fetch
>   a particular tool. Some tools are built from source code, in which case you will
>   need to have at least the `build-essential` and `git` packages installed.
>   
> -Tools are fetched into the `~/.binman-tools` directory.
> +Tools are fetched into the `~/.binman-tools` directory. This directory is
> +automatically added to the toolpath so there is no need to use `--toolpath` to
> +specify it. If you want to use these tools outside binman, you may want to
> +add this directory to your `PATH`. For example, if you use bash, add this to
> +the end of `.bashrc`::
> +
> +   PATH="$HOME/.binman-tools:$PATH"
> +
> +To select a custom directory, use the `--tooldir` option.
>   
>   Bintool Documentation
>   =====================
> @@ -1427,8 +1435,9 @@ Binman commands and arguments
>   
>   Usage::
>   
> -    binman [-h] [-B BUILD_DIR] [-D] [-H] [--toolpath TOOLPATH] [-T THREADS]
> -        [--test-section-timeout] [-v VERBOSITY] [-V]
> +    binman [-h] [-B BUILD_DIR] [-D] [--tooldir TOOLDIR] [-H]
> +        [--toolpath TOOLPATH] [-T THREADS] [--test-section-timeout]
> +        [-v VERBOSITY] [-V]
>           {build,bintool-docs,entry-docs,ls,extract,replace,test,tool} ...
>   
>   Binman provides the following commands:
> @@ -1453,11 +1462,13 @@ Options:
>   -D, --debug
>       Enabling debugging (provides a full traceback on error)
>   
> +--tooldir TOOLDIR     Set the directory to store tools
> +
>   -H, --full-help
>       Display the README file
>   
>   --toolpath TOOLPATH
> -    Add a path to the directories containing tools
> +    Add a path to the list of directories containing tools
>   
>   -T THREADS, --threads THREADS
>       Number of threads to use (0=single-thread). Note that -T0 is useful for
> diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
> index 6ca3d886200..bd6555a0aac 100644
> --- a/tools/binman/bintool.py
> +++ b/tools/binman/bintool.py
> @@ -52,7 +52,7 @@ class Bintool:
>       missing_list = []
>   
>       # Directory to store tools
> -    tooldir = os.path.join(os.getenv('HOME'), '.binman-tools')
> +    tooldir = None
>   

This default is an issue since None is not a valid path and will trigger 
a TypeError......

>       def __init__(self, name, desc, version_regex=None, version_args='-V'):
>           self.name = name
> @@ -113,6 +113,10 @@ class Bintool:
>           obj = cls(name)
>           return obj
>   
> +    @classmethod
> +    def set_tool_dir(cls, pathname):
> +        cls.tooldir = pathname
> +
>       def show(self):
>           """Show a line of information about a bintool"""
>           if self.is_present():
> @@ -210,6 +214,8 @@ class Bintool:
>           if result is not True:
>               fname, tmpdir = result
>               dest = os.path.join(self.tooldir, self.name)
> +            if not os.path.exists(self.tooldir):

..... here.

I can suggest "", which seems to make os.path.exists() happy, or check 
whether tooldir is None before checking if the path stored in the 
variable exists.

Also, nit: os.makedirs takes exist_ok argument to do the mkdir -p 
equivalent in Python AFAICT. (though "" does not make os.makedirs happy).

> +                os.makedirs(self.tooldir)

Thanks for following up on the discussion :)

Cheers,
Quentin