Re: [RFC PATCH v1 00/21] Modernize U-Boot shell

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 6478 bytes --]

On Fri, Dec 31, 2021 at 05:13:06PM +0100, Francis Laniel wrote:

> Hi.
> 
> First I hope you are fine and the same for your relatives.
> 
> During 2021 summer, Sean Anderson wrote a contribution to add a new shell, based
> on LIL, to U-Boot [1][2].
> While one of the goals of this contribution was to address the fact actual
> U-Boot shell, which is based on Busybox hush, is old there was a discussion
> about adding a new shell versus updating the actual one [3][4].
> 
> So, in this series, with Harald Seiler, we updated the actual U-Boot shell to
> reflect what is currently in Busybox source code.
> Basically, this contribution is about taking a snapshot of Busybox shell/hush.c
> file (as it exists in commit 37460f5da) and adapt it to suit U-Boot needs.
> 
> This contribution was written to be as backward-compatible as possible to avoid
> breaking the existing.
> So, the 2021 hush flavor offers the same as the actual, that is to say:
> 1. Variable expansion.
> 2. Instruction lists (;, && and ||).
> 3. If, then and else.
> 4. Loops (for, while and until).
> No new features offered by Busybox hush were implemented (e.g. functions).
> 
> In terms of testing, new unit tests were added to ut to ensure the new behavior
> is the same as the old one and it does not add regression.
> Nonetheless, if old behavior was buggy and fixed upstream, the fix is then added
> to U-Boot [5].
> In sandbox, all of these tests pass smoothly:
> 2021> printenv board
> board=sandbox
> 2021> ut hush
> Running 20 hush tests
> ...
> Failures: 0
> Thanks to the effort of Harald Seiler, I was successful booting a board:
> 2021> printenv board_rev
> board_rev=iMX8MP
> 2021> boot
> ...
> root@iMX8MPboard:~# fw_printenv board_rev
> board_rev=iMX8MP
> 
> I marked this contribution as RFC to indeed collect your opinion.
> My goal is not to change suddenly actual shell to this one, we clearly need a
> transition period to think about it.
> I think it is better to see this contribution as a proof of concept which shows
> it is possible to update the actual shell.
> 
> If you want to review it - your review will really be appreciated - here are
> some information regarding the commits:
> * commits marked as "test:" deal with unit tests.
> * commit "cli: Add Busybox upstream hush.c file." copies Busybox shell/hush.c
> into U-Boot tree, this explain why this commit contains around 12000 additions.
> * commit "cli: Port Busybox 2021 hush to U-Boot." modifies previously added file
> to permit us to use this as new shell.
> The really good idea of #include'ing Busybox code into a wrapper file to define
> some particular functions while minimizing modifications to upstream code comes
> from Harald Seiler.
> * Other commits focus on enabling features we need (e.g. if).
> 
> Francis Laniel (21):
>   test: Add framework to test hush behavior.
>   test: hush: Test hush if/else.
>   test/py: hush_if_test: Remove the test file.
>   test: hush: Test hush variable expansion.
>   test: hush: Test hush commands list.
>   test: hush: Test hush loops.
>   cli: Add Busybox upstream hush.c file.
>   cli: Port Busybox 2021 hush to U-Boot.
>   cli: Add choice for hush parser.
>   cli: Add HUSH_2021_PARSER to hush parser choice .
>   cli: Enables using hush 2021 parser as command line parser.
>   cli: hush_2021: Enable variables expansion for hush 2021.
>   cli: hush_2021: Add functions to be called from run_command().
>   cli: Modify run_command() to add hush 2021 as parser.
>   test: hush: Fix instructions list tests for hush 2021.
>   test: hush: Fix variable expansion tests for hush 2021.
>   cli: hush_2021: Enable using \< and \> as string compare operators.
>   cli: hush_2021: Enable if keyword.
>   test: hush: Fix if tests for hush 2021.
>   cli: hush_2021: Enable loops.
>   test: hush: Fix loop tests for hush 2021.
> 
>  cmd/Kconfig                        |    22 +
>  common/Makefile                    |     3 +-
>  common/cli.c                       |    27 +-
>  common/cli_hush_2021.c             |   309 +
>  common/cli_hush_2021_upstream.c    | 12791 +++++++++++++++++++++++++++
>  include/cli_hush.h                 |     8 +
>  include/test/hush.h                |    15 +
>  include/test/suites.h              |     1 +
>  test/Makefile                      |     3 +
>  test/cmd_ut.c                      |     6 +
>  test/hush/Makefile                 |    10 +
>  test/hush/cmd_ut_hush.c            |    20 +
>  test/hush/dollar.c                 |   242 +
>  test/hush/if.c                     |   351 +
>  test/hush/list.c                   |   127 +
>  test/hush/loop.c                   |    84 +
>  test/py/tests/test_hush_if_test.py |   184 -
>  17 files changed, 14015 insertions(+), 188 deletions(-)
>  create mode 100644 common/cli_hush_2021.c
>  create mode 100644 common/cli_hush_2021_upstream.c
>  create mode 100644 include/test/hush.h
>  create mode 100644 test/hush/Makefile
>  create mode 100644 test/hush/cmd_ut_hush.c
>  create mode 100644 test/hush/dollar.c
>  create mode 100644 test/hush/if.c
>  create mode 100644 test/hush/list.c
>  create mode 100644 test/hush/loop.c
>  delete mode 100644 test/py/tests/test_hush_if_test.py
> 
> 
> Best regards, thank you in advance for your reviews and happy new year!

Sorry for the extremely delayed response here.  I'm glad you did this.
The first thing I see is that there's still some fail to build problems
to sort out:
https://source.denx.de/u-boot/u-boot/-/commits/TEST/RFC-modernize-u-boot-shell
and doing a world build locally shows a few more also fall out so please
give https://u-boot.readthedocs.io/en/latest/develop/ci_testing.html a
read on how to get at least an Azure run for the next iteration.

When enabled by default there's a few unused function warnings (I
enabled the option for all boards for a test) that also need to be
addressed.

Then the next step would be to get all of the current tests to pass, for
me I get:
FAILED test/py/tests/test_md.py::test_md_repeat - AssertionError: assert '00200040: ' in ''
FAILED test/py/tests/test_ut.py::test_ut[ut_hush_hush_test_simple_dollar] - Exception: Bad p...
FAILED test/py/tests/test_ut.py::test_ut[ut_lib_lib_test_hush_echo] - Exception: Bad pattern...
after dropping the forced SYS_PROMPT.

Again, thanks for doing this and I do look forward to the next posting.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]