Generate contents.xml flash files for Axiom #142
Conversation
Signed-off-by: Loïc Minier <[email protected]>
Test Results 3 files ±0 7 suites ±0 19m 11s ⏱️ ±0s For more details on these failures, see this check. Results for commit ce624fe. ± Comparison against base commit 89cba35. |
Test jobs for commit ce624fe |
|
The tests actually pass, it's just that I had to give back the RB1 boot test due to foundriesio/linux-qcom#2 |
|
What's the rationale for making these changes, please? Perhaps that could go into the commit messages? |
Ah yes, I was reluctant to put details because I wasn't sure if I could mention this publicly, but actually I think there is no harm in doing that. While today we test our images with boards in a LAVA instance, the regular device testing infra inside Qualcomm is Axiom. Axiom expects this metadata file called contents.xml that describes the flashing procedure. This PR leverages ptool's newly gained ability to generate these files, and is making similar adjustments to the filename/pathnames as in rawprogram0.xml for regular/LAVA flashing. I did put the name in the PR's description, and can change the commit message. I'd like to avoid retriggering a build just for that, so I'll wait until review is complete to update if that makes sense. |
| while read line; do | ||
| case "$line" in | ||
| "--disk "*) | ||
| disk_type="$(echo "$line" | sed -n 's/.*--type=\([^ ]*\).*/\1/p')" |
There was a problem hiding this comment.
I'm struggling to review this parsing. Could I suggest using getopt(1) to parse these lines instead? It would less error-prone to find edge case bugs, and I think (but am not certain) it'd look cleaner here then, too.
It might also be worth considering if you've stretched shell a little further that it can sensibly go and switch to Python here instead. The task is nicely abstracted here already I think - something like adjust_partitions_conf.py might work well.
Provided we're testing the various result for various edge cases (I think we are?), then I think it's OK to leave it as-is if you don't think it's worth it though.
There was a problem hiding this comment.
Reading you, I first thought that getopt would be a good idea to simplify this, but trying to use it I realized that since I have to reconstruct the line in some cases (when changing filename), it gets a bit ugly and I think sed does a better job.
Here's what I could do to make parsing (by other humans :)) easier:
- add more comments with examples of the lines we're parsing (e.g. from https://github.com/qualcomm-linux/qcom-ptool/blob/main/platforms/qcs6490-rb3gen2/partitions.conf)
- extract values such as "disk_type" and name with something more idiomatic like:
name=$(echo "$line" | grep -oP '(?<=--name=)[^ ]+')
What do you think?
I agree it's getting a bit of a stretch in shell, but I'm also have a bias to keep the logic in a single place without jumping between files as when branching to external scripts. Perhaps we can indeed address this directly in ptool, or in debos actions?
There was a problem hiding this comment.
it gets a bit ugly and I think sed does a better job.
Fair enough! I appreciate there are trade-offs.
Perhaps we can indeed address this directly in ptool, or in debos actions?
I haven't spent the time to understand this aspect of ptool and how we're using it in detail. It does seem odd to have to parse and transform something in the form of ptool's input, but from an example file provided upstream, and this suggests that we have a use case that upstream ptool could implement a better interface there for us. That sounds like a upstream feature request to me, and in the meantime I think what you have is fine then. Maybe it's worth leaving an upstream issue open.
There was a problem hiding this comment.
I've added comments with sample inputs and more detailed steps
| -t "$contents" \ | ||
| -o contents.xml | ||
| # fix file_name and file_path; qcom-ptool doesn't generate them | ||
| # properly |
There was a problem hiding this comment.
Do we need a bug against qcom-ptool for this?
There was a problem hiding this comment.
Yeah; however, as I can't test this myself, I'm not sure how these pathnames are interpreted; in ptool itself, there are a bunch of partitions.conf and these don't need relative pathnames, so not sure if the changes would be acceptable there.
There was a problem hiding this comment.
Similar to above, perhaps we should explain our use case in a ptool upstream issue, perhaps combined with the previous case, since it seems to me that we shouldn't be having to parse and transform this file in the first place from an architectural perspective.
There was a problem hiding this comment.
There was a problem hiding this comment.
This was merged in ptool; I've dropped the workaround in this branch
lool
changed the title
|
Let's wait a day or two to see if the qcom-ptool PR get merged or see some pushback... |
This will make it easier to add contents.xml support. Signed-off-by: Loïc Minier <[email protected]>
Infered from qcom-ptool data. Signed-off-by: Loïc Minier <[email protected]>
lool
force-pushed
the
gen-contents-xml
branch
from
ce624fe to
69e552f
Compare
Signed-off-by: Loïc Minier <[email protected]>
lool
force-pushed
the
gen-contents-xml
branch
from
69e552f to
6ab99f0
Compare
Test jobs for commit 6ab99f0 |
4 participants
Generate contents.xml flash files for Axiom: