feat(flash): introduce override-partition-conf.sh#334
feat(flash): introduce override-partition-conf.sh#334gagath wants to merge 1 commit intoqualcomm-linux:mainfrom
Conversation
95f2def to
309c495
Compare
|
Thanks for starting that: that's exactly the direction I think we need to take the flash recipe towards! From the top of my head, we mainly need these features:
My gut feeling is that while one or multiple shell scripts can do this, it will reach the ceiling of readability and testability, so I'd recommend pivoting to python. Perhaps 1) is a standalone somewhat generic script, and 2) and 3) are another script for debos flash integration calling the first one with target boards and specific inputs? |
I have started to do this on a personal branch using JSON, but the flash recipe was too complex to introduce the standalone database in a single PR. I would prefer multiple, focused PRs that allow us to arrive there.
Yes. I want unit tests around the code and I like Python. However to keep this PR simple I keep bash. The script can be migrated to Python later, with the introduction of unit tests, etc. Since I see no major objections I will mark this as ready for review. Once merged we can continue to iterate towards our common goal. |
|
TBH I don't see the point of moving the shell code around and I would prefer we start a pull request with incremental commits switching to an external python script. |
|
Here's a sample PR with the commits converting from shell to python which Robie found easy to review :) #220 |
The immediate benefit is easier debug experience. I spent time figuring out I forgot to specify This shell error was also found and debugged by Adam in this commit: abickett@73dede4
While I do not know how much it took them to understand the error, the "shell error syntax" mentioned could have been prevented by this PR which would replace it by a simpler "empty cdt_filename argument" investigation. If this benefit is not seen as enough as-is, I can put Python rewrite commits on top. But this will prevent other people to be in this situation again while rewritten, hence my dual-PR proposal which I still maintain for contributors’ sake. |
In the flash recipe is a section that involves replacing some lines in the partition.conf file and uses stdin/stdout, which makes it a good candidate for extraction in order to reduce the complexity of the flash recipe. Since the for loop in the recipe makes the code duplicated into a huge script, this simplifies debugging by having the possibility to see what variables are passed to the script each time instead of trying to debug with a big line number that is almost impossible to interpret because of template for loops. This also allows to manually test the overrides against an arbitrary partition.conf file. The behavior around cdt_download and cdt_filename is preserved, using a shell condition instead of a template condition. A new message is printed in the case of an existing cdt partition but missing cdt_filename variable. Signed-off-by: Agathe Porte <agathe.porte@oss.qualcomm.com>
309c495 to
5f130a3
Compare
|
Rebased on top of main to include the recent changes for nvme and spinor support. |
In the flash recipe is a section that involves replacing some lines in the partition.conf file and uses stdin/stdout, which makes it a good candidate for extraction in order to reduce the complexity of the flash recipe.
Since the for loop in the recipe makes the code duplicated into a huge script, this simplifies debugging by having the possibility to see what variables are passed to the script each time instead of trying to debug with a big line number that is almost impossible to interpret because of template for loops.
This also allows to manually test the overrides against an arbitrary partition.conf file.
The behavior around cdt_download and cdt_filename is preserved, using a shell condition instead of a template condition. A new message is printed in the case of an existing cdt partition but missing cdt_filename variable.