Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow builds without a variant but with additional (c)make paramaters such as USER_C_MODULES #46

Open
Josverl opened this issue Nov 10, 2024 · 5 comments

Comments

@Josverl
Copy link
Contributor

Josverl commented Nov 10, 2024

currently it is not possible to specify common parameters to make / cmake such as

  • clean
  • USER_C_MODULES
    as they are incorrectly assumed to be the variant.
jos@josverl-sb5:~/micropython_2$ mpbuild build ESP32_GENERIC_S3 USER_C_MODULES=/home/jos/micropython_2/ulab/code/micropython.cmake all
Invalid variant

As a result of that it is not possible to use mpbuild to build a lot of common builds such as adding ulab or lvgl to a build.

perhaps -- can be used to indicate that the remaining params should be passed to (c)make rather than consumed by mpbuild
mpbuild build ESP32_GENERIC_S3 -- USER_C_MODULES=/home/jos/micropython_2/ulab/code/micropython.cmake all

@Josverl Josverl changed the title Allow builds withouth a variant but with additional (c)make paramaters such as USER_C_MODULES Allow builds without a variant but with additional (c)make paramaters such as USER_C_MODULES Nov 10, 2024
@mattytrentini
Copy link
Owner

One workaround is to provide a blank variant:

$ mpbuild build ESP32_GENERIC_S3 "" USER_C_MODULES=/home/jos/micropython_2/ulab/code/micropython.cmake all

@Josverl
Copy link
Contributor Author

Josverl commented Nov 11, 2024

Nice for now, and one for the docs 😎

@mattytrentini
Copy link
Owner

There are two other related thought bubbles:

  1. I've been considering changing the way boards and variants are specified - by using slashes. So in this case it would be ESP32_GENERIC_S3 or ESP32_GENERIC_S3/SPIRAM_OCT and the port could optionally be added esp32/ESP32_GENERIC_S3/SPIRAM_OCT. That way it's only one required argument. Specifying the port would allow tab completion to be constrained.

I'm not 100% happy with the syntax but it does make parsing of the line clearer. What do you think?

  1. I'm not entirely satisfied with the list of additional options just passed to the make files. Instead, I'm considering adding explicit options to specify those parameters. Either very explicitly:
$ mpbuild build ESP32_GENERIC_S3 --user-c-modules=/home/jos/micro*

Or:

$ mpbuild build ESP32_GENERIC_S3 --additional="USER_C_MODULES=/home/jos/micro*"

Any opinions? I should probably carve these out into specific discussions...

@Josverl
Copy link
Contributor Author

Josverl commented Nov 11, 2024

To be honest, I don't really like it. There is a lot of documentation that uses just the port board and variants separated by spaces.
Similarly, for the C make arguments, they are generally documented just as you enter them on the command line after make.
I would not like that to need to need to rebuild them in a slightly different mpbuild syntax , especially if they are somewhat complex and contain quotes and such.

let me try to see if i can find a way to express that in a typer cli - I think that misses the option for variadic arguments : nargs.
but perhaps there is a workaround for that - its definitely come up before : fastapi/typer#110 (comment)

@Josverl
Copy link
Contributor Author

Josverl commented Nov 11, 2024

To be honest, I don't really like it. There is a lot of documentation that uses just the port board and variants separated by spaces.
Similarly, for the C make arguments, they are generally documented just as you enter them on the command line after make.
I would not like that to need to need to rebuild them in a slightly different mpbuild syntax , especially if they are somewhat complex and contain quotes and such.

Instead, I think it would be rather simple to just assume that there are three arguments provided after the build command.
They should. be [port] , board and [variant] in that sequence.
I have found no way for Typer/Click to handle that directly , but its not too complex with a helper function.

the problem is that Typer cannot handle a limited number or arguments for board & variant , and also handle a a variable number of arguments for the -- / --extra args that need to be passed.

Controlling the max length of the PBV list
If we were working directly with 'click' rather than through typer, there's an option for variadic arguments : nargs. You can set that to the maximum number of arguments that is accepted, in this case it would be 3. It seems that typer doesn't have that option and just consumes all remaining arguments.
I briefly looked in the source; typed does seem to use it (nargs) , but I've not found a way to directly provide that, that we can provide a custom error message for that.

Parsing extra/ additional args to make
This brings about the second part of the cli how to specify extra arguments to make or cmake.
I would just then keep the rest of the arguments in the exact change format as that you would use it after make. There's a lot of different options and constants that you can set on the make command line, and I don't think it would make sense to try and model them all as separate options.
I think the common format is to use -- However I've not yet found a way for type or to accept that as an option name.
(it is possible to add -- on the cmdline; and it just gets ignore in my testing.)

Ive build a number of different implementations for a quick PoC ,
and I think i like this one the best:

 Usage: mpb110.py build [OPTIONS] BOARD [VARIANT] [EXTRA]...                                                                                                                                                                         
                                                                                                                                                                                                                                     
 Build a MicroPython board. [PORT] BOARD [VARIANT].                                                                                                                                                                                  
                                                                                                                                                                                                                                     
╭─ Arguments 
│ *    board        TEXT        Board name [default: None] [required]                                                                                                                                                               
│      variant      [VARIANT]   Board variant [default: None]                                                                                                                                                                       
│      extra        [EXTRA]...  additional arguments to pass to (c)make [default: None]                                                                                                                                             
╰──────────────────────────────────────────────────────────────────────────────
╭─ Options ───────────────────────
│ --idf                      TEXT  esp32 port only: select IDF version to build with [default: v5.2.2]                                                                                                                             
│ --build-container          TEXT  Override the default build container [default: None]                                                                                                                                            
│ --help             -h            Show this message and exit.                                                                                                                                                                     
╰──────────────────────────────────────────────────────────────────────────────

Stand alone Poc repo : [https://github.com/Josverl/mpbuild_typer_test

It accepts

  • any sensible combination of Port, Board and Variant ( matched to the mpy-repo )
  • all other options
  • a -- , Actually it gets silently ignored
  • any other arguments ( not port/board/variant) are added ( prefixed) to the extra args ( adding clean will just work )
Test results

test_cli.py Testing: `mpbuild build stm32 PYBV11 DP` Building pbv=['stm32', 'PYBV11', 'DP'] Building port='stm32' board='PYBV11' variant='DP' extra=[] build_container=None idf='v5.2.2'

.Testing: mpbuild build stm32 PYBV11 DP clean
Building pbv=['stm32', 'PYBV11', 'DP', 'clean']
Building port='stm32' board='PYBV11' variant='DP'
extra=['clean']
build_container=None
idf='v5.2.2'

.Testing: mpbuild build stm32 PYBV11 DP CMAKE=foo BAR=1
Building pbv=['stm32', 'PYBV11', 'DP', 'CMAKE=foo', 'BAR=1']
Building port='stm32' board='PYBV11' variant='DP'
extra=['CMAKE=foo', 'BAR=1']
build_container=None
idf='v5.2.2'

.Testing: mpbuild build rp2 RPI_PICO
Building pbv=['rp2', 'RPI_PICO']
Building port='rp2' board='RPI_PICO' variant=''
extra=[]
build_container=None
idf='v5.2.2'

.Testing: mpbuild build rp2 RPI_PICO MAKE=1
Building pbv=['rp2', 'RPI_PICO', '', 'MAKE=1']
Building port='rp2' board='RPI_PICO' variant=''
extra=['MAKE=1']
build_container=None
idf='v5.2.2'

.Testing: mpbuild build rp2 RPI_PICO clean CMAKE_OPTON=1
Building pbv=['rp2', 'RPI_PICO', '', 'clean', 'CMAKE_OPTON=1']
Building port='rp2' board='RPI_PICO' variant=''
extra=['clean', 'CMAKE_OPTON=1']
build_container=None
idf='v5.2.2'

.Testing: mpbuild build RPI_PICO clean CMAKE_OPTON=1
Building pbv=['', 'RPI_PICO', '', 'clean', 'CMAKE_OPTON=1']
Building port='' board='RPI_PICO' variant=''
extra=['clean', 'CMAKE_OPTON=1']
build_container=None
idf='v5.2.2'

.Testing: mpbuild build RPI_PICO -- clean
Building pbv=['', 'RPI_PICO', '', 'clean']
Building port='' board='RPI_PICO' variant=''
extra=['clean']
build_container=None
idf='v5.2.2'

.Testing: mpbuild build ESP32_GENERIC OTA clean CMAKE_OPTON=1
Building pbv=['', 'ESP32_GENERIC', 'OTA', 'clean', 'CMAKE_OPTON=1']
Building port='' board='ESP32_GENERIC' variant='OTA'
extra=['clean', 'CMAKE_OPTON=1']
build_container=None
idf='v5.2.2'

.Testing: mpbuild build ESP32_GENERIC OTA -- clean
Building pbv=['', 'ESP32_GENERIC', 'OTA', 'clean']
Building port='' board='ESP32_GENERIC' variant='OTA'
extra=['clean']
build_container=None
idf='v5.2.2'

.Testing: mpbuild build RPI_PICO -- clean CMAKE_OPTON=1
Building pbv=['', 'RPI_PICO', '', 'clean', 'CMAKE_OPTON=1']
Building port='' board='RPI_PICO' variant=''
extra=['clean', 'CMAKE_OPTON=1']
build_container=None
idf='v5.2.2'

.Testing: mpbuild build --idf v9.8.7 RPI_PICO clean CMAKE_OPTON=1
Building pbv=['', 'RPI_PICO', '', 'clean', 'CMAKE_OPTON=1']
Building port='' board='RPI_PICO' variant=''
extra=['clean', 'CMAKE_OPTON=1']
build_container=None
idf='v9.8.7'

.Testing: mpbuild build --idf v9.8.7 -- RPI_PICO clean CMAKE_OPTON=1
Building pbv=['', 'RPI_PICO', '', 'clean', 'CMAKE_OPTON=1']
Building port='' board='RPI_PICO' variant=''
extra=['clean', 'CMAKE_OPTON=1']
build_container=None
idf='v9.8.7'

And I added a number of tests to verify the input
( would need to be adapted to mock when merged )

If you think that CLI UX is useful, let me know and I can create a PR against mpbuild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants