Optional DTB #186
Optional DTB #186
Conversation
Add a config to select a list of target boards to build. The default is still to build for all possible boards. Signed-off-by: Loïc Minier <[email protected]>
Build a list of dtb files from dtbs.tar.gz and skip boards that use a dtb that isn't present. This allows listing all supported boards/dtbs, while building with kernels that don't support all boards. Missing dtbs/boards get automatically skipped. Signed-off-by: Loïc Minier <[email protected]>
Document new target_boards option for the flash recipe and deprecate the build_* options. Signed-off-by: Loïc Minier <[email protected]>
Option was removed some time ago. Signed-off-by: Loïc Minier <[email protected]>
Misc rewording from spellcheck. Signed-off-by: Loïc Minier <[email protected]>
Test jobs for commit 6883f28 |
There was a problem hiding this comment.
Some minor comments inline. I think I understand what you're doing here, but I'm not sure I completely grasp it. I understand what it means to not have a particular board supported by the upstream kernel, and therefore not appear in dtbs.tar.gz. But:
...this will allow listing boards supported only by qcom-next or mainline that are not supported with the current stable linux-qcom kernel
...what does "listing boards" mean?
| rm -f "${targets_file}" | ||
| case "{{ $target_boards }}" in | ||
| all) | ||
| # no override; build all possible boards (default) |
There was a problem hiding this comment.
You already have $boards defined, right? Could you make the later code simpler by unconditionally writing build/targets.txt, so the "all" magic is restricted to here, rather than needing to be understood elsewhere? I think that would avoid surprising semantics of when build/targets.txt doesn't exist.
There was a problem hiding this comment.
Let me try to repeat what I understand you're suggesting: have the go template logic at the top to generate a targets.txt table based on the data from $boards, then have a pure shell script reading through that table afterwards. Is that correct?
There was a problem hiding this comment.
I think that matches, yes, if by "at the top" you mean in the code area highlighted here?
I think you could write $board.name for $board in $boards to build/targets.txt in the area of code highlighted here in the case that $target_boards is set to "all". Then below the if [ -e "${targets_file}" ]; then can go away - it'd be unconditionally setting skip_board to true if $board.name (in the iteration below) isn't present in build/targets.txt.
This would take away the special handling when build/targets.txt isn't present, because it'd always be present, making the logic simpler and less surprising from my POV.
| "${QCOM_PTOOL}/ptool.py" -x ptool-partitions.xml | ||
| ) | ||
|
|
||
| if [ "${skip_board}" = false ]; then |
There was a problem hiding this comment.
Do the following lines need indenting, or is GitHub hiding that from me? Same question for the other if statements below.
There was a problem hiding this comment.
It does, but then I have to indent the whole file, becoming hard to read and review :)
happy to do that though!
There was a problem hiding this comment.
My opinion: especially because there are many matching if/fi close together, it's easier to read with the expected indentation and if someone is debugging in this area then your intent will be clearer.
Factoring some of these into functions might help with readability too, but that's another can of worms so maybe not worth it.
This is subjective of course so I'll leave it to you.
I mean listing them in our known boards in the long yaml table in flash.yaml |
| rm -f "${targets_file}" | ||
| case "{{ $target_boards }}" in | ||
| all) | ||
| # no override; build all possible boards (default) |
There was a problem hiding this comment.
I think that matches, yes, if by "at the top" you mean in the code area highlighted here?
I think you could write $board.name for $board in $boards to build/targets.txt in the area of code highlighted here in the case that $target_boards is set to "all". Then below the if [ -e "${targets_file}" ]; then can go away - it'd be unconditionally setting skip_board to true if $board.name (in the iteration below) isn't present in build/targets.txt.
This would take away the special handling when build/targets.txt isn't present, because it'd always be present, making the logic simpler and less surprising from my POV.
| "${QCOM_PTOOL}/ptool.py" -x ptool-partitions.xml | ||
| ) | ||
|
|
||
| if [ "${skip_board}" = false ]; then |
There was a problem hiding this comment.
My opinion: especially because there are many matching if/fi close together, it's easier to read with the expected indentation and if someone is debugging in this area then your intent will be clearer.
Factoring some of these into functions might help with readability too, but that's another can of worms so maybe not worth it.
This is subjective of course so I'll leave it to you.
|
Approved with inline changes clarified, I think? Everything is subjective so it's up to you if you want to change anything or not. I don't think I need to review again. |
2 participants
Make DTB optional as to allow kernels with less support than all known
boards – this will allow listing boards supported only by qcom-next or
mainline that are not supported with the current stable linux-qcom
kernel.
Update docs for new option and use this opportunity for cleanups.